kallsyms: change Modules caching strategy, cache address lookups#1590
kallsyms: change Modules caching strategy, cache address lookups#1590ti-mo merged 4 commits intocilium:mainfrom
Conversation
Since this function allows lookups to be performed in batches, caching lookup results becomes interesting, without having to resort to caching all of kallsyms. Signed-off-by: Timo Beckers <[email protected]>
| } | ||
| } | ||
| if len(mods) != 0 { | ||
| if err := kallsyms.AssignModules(mods); err != nil { |
There was a problem hiding this comment.
I tried this branch locally and it fails for me with an ambiguous error for kernel module load_elf_phdrs. If I understood the old code correctly, it didn't care about duplicate symbols in /proc/kallsyms.
There was a problem hiding this comment.
Correct, it would ignore kallsyms entries without modules, which is not correct. If a symbol is ambiguous between the core kernel and a module, we should not attach to it. The old code might've picked the address of the base symbol to attach to, but provided the BTF of the symbol in the kernel module.
There was a problem hiding this comment.
@patrickpichler Is this a breaking change for you, or were you just testing out ambiguous symbol handling during kmod attach?
There was a problem hiding this comment.
It is a breaking change, but I guess it was not right before due to ambiguity and anyway would have been a bug 😅
There was a problem hiding this comment.
Yes, if you'd actually make use of CO:RE things, build your BPF C program against headers of one kernel, deploy your app to another host, then be unlucky enough for some of the fields to have changed between versions, you would've likely hit a verifier error at runtime.
'Unfortunately', in most cases like load_elf_phdrs, it's actually the same function with the same signature and underlying types that gets included into multiple compilation units (by way of including a common header) and then gets merged into the same binary during linking of the kernel binary. Things would've kept working fine in your case since it's likely an exact duplicate, but you would run into funky cases where some code paths in the kernel invoke one version of the symbol, while other paths invoke the other, meaning you'd miss events since you're only attaching to one of the symbols.
Let's see if any other skeletons fall out of the ceiling when we ship this in a release, but this has been a bugbear for bpf since the beginning.
dylandreimerink
left a comment
dylandreimerink
left a comment
There was a problem hiding this comment.
This all makes sense. Very nice
|
@ti-mo Thanks, I tested this PR in Inspektor Gadget, and it uses between 8 and 10MB less memory than before. |
Renamed SymbolModule to Module and introduced AssignModules, a batch version of Module similar to the existing AssignAddresses. Along with this change, ambiguous symbols (with non-unique names) are now rejected to avoid sending the wrong BTF ID for a symbol for the kernel to use during verification. Similar to AssignAddresses, this API now only caches requested symbols instead of all of kallsyms. Some users found the permanent memory allocation burdensome and relied on flushing them after loading the programs they needed. btf.FlushKernelSpec now no longer calls FlushKernelModuleCache, and the latter has been removed due to the significantly lowered memory consumption. Signed-off-by: Timo Beckers <[email protected]>
The original implementation opened /proc/kallsyms and resolved all symbols during CollectionSpec loading. This creates issues like bpf2go not being able to generate its output when run unprivileged, since it can't properly resolve ksym addresses. /proc/kallsyms addresses are zero in this case. Also, some users load CollectionSpecs in non-linux environments like wasm, so we need the spec loading to remain hermetic. Probably a good idea to add some tests for this at some point, though not clear how at this time. Signed-off-by: Timo Beckers <[email protected]>
As mentioned in the code comments, scan all programs in the CollectionSpec to allow batching together kallsyms lookups, avoiding repeated scanning of /proc/kallsyms. Signed-off-by: Timo Beckers <[email protected]>
ti-mo
force-pushed
the
tb/kallsyms-caching
branch
from
daf2425 to
2e21f0b
Compare
4 participants
See individual commits for context.
At a high level:
newProgramWithOptionsonwards