feat: add support for untyped extern __ksym variables

[patrickpichler]

It is now possible to use the __ksym attribute to load the address of the ksym for every use of the variable. It works by searching the name of any external ksym variable in the /proc/kallsyms file and patching any load operation on that external variable to load the address as an immediate value.

[dylandreimerink]

Thank you for the work! I have a few things to address though.

[dylandreimerink]

Thanks again for the work!

[ti-mo]

Thanks for working on this! This is not a trivial contribution, I commend you for taking on the challenge. 🙂 Most of my comments are around style and readability.

We also need to make sure we reject non-variable references, unless there's a bpf use case for that, of course. Thanks again!

[ti-mo]

Thanks! Just gave this a second review. Feel free to squash any amends into the first commit, I don't usually look at fixup commits separately. Re-request review when ready.

[ti-mo]

@patrickpichler Thank you for addressing all of my feedback so far. Something came up in #1584 that made me realize using fmt scanning is really quite slow for something that potentially happens hundreds of thousands of times on every ELF load. The result of the parsing operation also isn't cached, which isn't ideal.

Since we have some work to do on providing an efficient kallsyms parser, is it okay if we change this to a draft for now? The surrounding code is fine and doesn't need to change anymore, it just needs to use a common parser implementation with the kallsyms kmod logic.

[patrickpichler]

@ti-mo fine with me 👌
Not sure if you saw it, but dylan did a quick implementation of a pretty performant version for the kallsym scanning in one of the PR comments (might be useful)
#1578 (comment)

[patrickpichler]

@ti-mo after thinking some more about it, would it maybe be possible to still merge it? 😅
It is an opt-in change, meaning there will be no performance regression of existing users and I would kinda need the functionality for another project 😅

It would also make things easier to work on BTF support (otherwise I would need to layer PRs) 😅

If not, it is also no problem though 😄

[ti-mo]

@ti-mo after thinking some more about it, would it maybe be possible to still merge it? 😅 It is an opt-in change, meaning there will be no performance regression of existing users and I would kinda need the functionality for another project 😅

Agreed, no harm in merging as-is. Will make it somewhat easier to introduce the new reader and making sure it works for both use cases.

I've pushed a small fixup and will merge when tests are green. Thanks again for picking this up!

[dylandreimerink]

Something came up in #1584 that made me realize using fmt scanning is really quite slow ...

This is true, the scanner allocates a lot, which makes it slow. I mentioned this in #1578 (comment) as well. You can improve it with a more bespoke implementation, after which you are bottleneck-ed by the read syscall. But you only hit that path if you use ksyms.

... for something that potentially happens hundreds of thousands of times on every ELF load.

Patrick specifically changed the logic (on my request) so we collect all symbols for a whole collection and do this op once per ELF/Collection load.

The result of the parsing operation also isn't cached, which isn't ideal.

We also discussed the advantages and disadvantages of caching the parsed kallsyms in #1578 (comment). From my benchmarks, its not worth it. Unless you plan on loading multiple collections and can spare the RSS.

[ti-mo]

We also discussed the advantages and disadvantages of caching the parsed kallsyms in #1578 (comment). From my benchmarks, its not worth it. Unless you plan on loading more than 5 collections and can spare the RSS.

Yep, just read through some of the comments earlier. Shame I didn't see them before, I did the same implementation exercise for the other PR.

What could be a real improvement is having both a kallsyms parser that allocates minimally and a pull-through cache that only caches the symbols that were requested by the user.

I'd wager a guess that 99% of ELF loads over the lifetime of an application are the same objects over and over (looking at Cilium especially) for resources like containers that come and go. The set of symbols is going to be fairly static across all of those loads, since in the ksyms case these are baked into the variable definitions, and most people would be running something like bpf2go/prebuilt ELFs/CORE. If parsing becomes cheap, we can cache less aggressively, and we won't need to worry about ballooning memory consumption.

With this PR, we can technically refer to any symbol (not only tT), so with the current shotgun approach we'd need to cache everything, which is not realistic. I'll try and come up with a small parser that can be reused between kmods and ksyms.

[dylandreimerink]

What could be a real improvement is having both a kallsyms parser that allocates minimally and a pull-through cache that only caches the symbols that were requested by the user.

I'd wager a guess that 99% of ELF loads over the lifetime of an application are the same objects over and over (looking at Cilium especially) for resources like containers that come and go. The set of symbols is going to be fairly static across all of those loads, since in the ksyms case these are baked into the variable definitions, and most people would be running something like bpf2go/prebuilt ELFs/CORE. If parsing becomes cheap, we can cache less aggressively, and we won't need to worry about ballooning memory consumption.

Yea, I would also make that wager, good point.