feat: more link inspection support

[secDre4mer]

Add support for more link inspection types, e.g. to query a uprobe's symbol.

[mmat11]

looks mostly good to me, left a few comments; can this be split in multiple commits?

[secDre4mer]

looks mostly good to me, left a few comments; can this be split in multiple commits?

Sure, I'll try. Any ideas on how the commits should be structured? E.g. one focused on uprobe, one focused on tracepoint, ...?

[mmat11]

looks mostly good to me, left a few comments; can this be split in multiple commits?

Sure, I'll try. Any ideas on how the commits should be structured? E.g. one focused on uprobe, one focused on tracepoint, ...?

I guess you could split code generation and the different link types in different commits

[secDre4mer]

Split is done.

[mmat11]

@secDre4mer sorry for the delay, I was on vacation; I ran CI and some tests are failing, could you please take a look?

[secDre4mer]

@secDre4mer sorry for the delay, I was on vacation; I ran CI and some tests are failing, could you please take a look?

Don't worry about the delay, I hope your vacation was nice!

The CI complains that some generated files are out of date, see here; however, running make locally, as suggested, does not change any generated files. Maybe it's due to differing kernel versions? I'm running 6.17.8 locally.

[mmat11]

@secDre4mer sorry for the delay, I was on vacation; I ran CI and some tests are failing, could you please take a look?

Don't worry about the delay, I hope your vacation was nice!

The CI complains that some generated files are out of date, see here; however, running make locally, as suggested, does not change any generated files. Maybe it's due to differing kernel versions? I'm running 6.17.8 locally.

hmm I think you would need to update CI_MAX_KERNEL_VERSION https://github.com/cilium/ebpf/blob/main/.github/workflows/ci.yml#L10 and run ./scripts/update-kernel-deps.sh

the script has been added after last time I tried to update kernel types so I am not sure this is the correct thing to do -- /cc @lmb

[secDre4mer]

@mmat11 Thanks for the hints, the solution turned out to be somewhat easier than expected; main was already on 6.16, so I only had to rebase the branch and re-run make update-external-deps.

Now there are some issues with connecting to git.kernel.org, but those look unrelated to the changes I've made.

[dylandreimerink]

Re-triggered the tests, seems like we got some legit failures now

[secDre4mer]

I'll take a look.

[secDre4mer]

@dylandreimerink Do you have any idea why the error with Netfilter / XDP occurs here? I'm a little stumped.
The direct source is clear: The kernel uses EBUSY to indicate that another eBPF program is already attached.
However, I don't get why this is the case. While both TestAttachNetfilter and TestNetfilterInfo use the same hook / priority, they are executed sequentially and each should remove their program / link at the end.
Also, this seems to happen quite reliably in the CI on kernel 6.12; but when I run 6.12 locally, I can't reproduce it.

[ti-mo]

@secDre4mer Sorry for letting this languish, would you be able to rebase and resolve conflicts? Can't see the failures you were mentioning.

@mmat11 We're a little short on review cycles over here (what's new) so I'd appreciate if could take another look. Thanks! 🙏

[secDre4mer]

@ti-mo sure, done.

[mmat11]

@mmat11 We're a little short on review cycles over here (what's new) so I'd appreciate if could take another look. Thanks! 🙏

I restarted the failing tests and the CI looks ok now, the code lgtm!

[dylandreimerink]

Sorry for letting this linger, like Timo mentioned, review cycles have been short. But better late than never.

Overall this looks good, 2 minor points of interest.

Thanks for your work on this.

[dylandreimerink]

I think this is in a good state now. Thank!

[ti-mo]

Sorry for the long wait, there's too much stuff to review. Haven't made it through the full PR, but I've suggested something that could help with maintainability.

[ti-mo]

Thanks for making the gentypes changes so quickly, they're exactly what I had in mind. Finally managed to get through it all. There are a few changes I'd like to make to the exported API.

Calling OpenExecutable on behalf of a user is a potential surprise, so the fact that those methods depend on the executable being where they were when the link was created needs to be well-documented, and the resulting error needs to be very clear that this is expected when doing cross-namespace stuff.

In one of my comments I've called for removing umi.Symbols(), but I'm not opposed to merging it as long as the UX is good. Thanks!

[secDre4mer]

Would you be up for removing kp.Address() and kp.Missed() below and exporting the fields instead? Now would be a good time to change this as we're touching surrounding functionality.
@ti-mo Gladly; I noticed that it was inconsistent when implementing the PR and wasn't sure how to work with it.

[secDre4mer]

To me, this method falls more into convenience territory, since the user can accomplish the same using exported APIs. We tend to not include stuff like this, it goes a bit beyond the information returned by the kernel.
I presume you have an application that does this kind of symbol resolution? Would you be willing to carry this feature locally?

Certainly. I understand your point that the Symbol() methods on uprobe / uprobemulti infos are possibly beyond the scope of this library, especially with the caveats (mount namespaces, potentially deleted / replaced files, ...)
Ultimately, it's your call on whether you want to include this. We could also separate this into a different merge request to keep the changes simpler / easier to discuss.
If it's not included, I'd like to keep the "look up a symbol for an offset" logic in Executable, though, since it is necessary to do this locally.

[ti-mo]

Ultimately, it's your call on whether you want to include this. [...] If it's not included, I'd like to keep the "look up a symbol for an offset" logic in Executable, though, since it is necessary to do this locally.

I've mulled it over for a bit, I'd prefer not to keep the convenience methods and go for only Executable.Symbol(); I think it's more universally useful.

Thanks!

[secDre4mer]

I've mulled it over for a bit, I'd prefer not to keep the convenience methods and go for only Executable.Symbol(); I think it's more universally useful.

Thanks!

Removed the convenience methods.

[ti-mo]

Looks good, thanks! One of the tests started failing, though.

[secDre4mer]

Looks good, thanks! One of the tests started failing, though.

Thanks, I removed one "is ok" check in the test and that caused older kernels to fail. Added an explicit skip on kernels <6.6 with a comment.