btf: Add support for bpf_core_type_matches()

[dylandreimerink]

This PR adds support for the latest edition to CO-RE. The bpf_core_type_matches() function allows you to check if a given type matches. This is a stricter check than the normal compatibility check.

An example use case for this feature is to implement fallback code for cases where kernel types changed over time, such as the block_rq_insert tracepoint. The tracepoint lost an argument in the v5.11 kernel, so this feature can be used to handle the change in provided context in a CO-RE manner.

For reference, this is the patch series on which the implementation is based: https://lore.kernel.org/bpf/20220628160127.607834-1-deso@posteo.net/

Fixes: #1360

[alban]

I tested this PR in inspektor-gadget/inspektor-gadget#2542 and it seems to work for me.

cc @eiffel-fl

[dylandreimerink]

Undrafting the PR since it made it through CI. @alban I expect a bit of a delay on the review since Lorenz is currently on PTO until the 13th.

[eiffel-fl]

Hi!

Thank you for working on it! This is highly appreciated!
I will test it later, for now I have some comments.

Best regards.

[lmb]

Left some nits, looks pretty good. My main concern is that it's pretty difficult to understand the code flow, especially since there are some co-recursive functions in there. For me, behindPtr and level is evidence that we don't have a good abstraction for iterating the graph. Would it be possible to split the algorithm in a way that we can use something like modifyGraphPreorder? That is, express it as a series of checks against an iterator instead of calling functions recursively.

[lmb]

Also see #896 for a similar (but not exact due to ignoring names) use case.

[lmb]

Would it be possible to split the algorithm in a way that we can use something like modifyGraphPreorder? That is, express it as a series of checks against an iterator instead of calling functions recursively.

Ignore this idea, it is a bad one. I tried to come up with something along these lines while on the train, it ends up being a mess. I'll put up a PoC in a bit which maybe makes this all a bit easier to understand.

Re: matching what libbpf does: it's good to check that we have the same semantics (mostly) but I think we shouldn't do a 1:1 conversion of their logic.

[dylandreimerink]

Did an initial pass over the comments.

Re: matching what libbpf does: it's good to check that we have the same semantics (mostly) but I think we shouldn't do a 1:1 conversion of their logic.

I agree, we want the same behavior, not the same implementation. But that can be challenging, I will double check the commit messages, but in a lot of cases we didn't get a good explanation of why they made certain choices.

[lmb]

@dylandreimerink PTAL #1383 where I rewrite coreAreTypesCompatible to use recursion, which ends up being a lot simpler. Can we use the same approach here?

• A single self recursive function which traverses the types
• Helpers which do checks on individual types if necessary (but never recurse)

I think we should also drop all depth, level, etc. checks. They aren't particularly useful in Go: they only really make a difference with cyclical types, and we can handle those differently.

[lmb]

Thanks for rewriting this as a recursive function! Left a bunch of comments, most of which are just details. Some higher level questions:

First, we're introducing a bunch of quadratic behaviour when comparing enums and composite members. Does it make sense to break these into two steps?

1. build a map[string]Member (or similar)
2. iterate once over localType

Enums are probably not that problematic, but I'm pretty sure that structs can get hundreds of members long.

Second, I'm really fuzzy about this whole behindPtr business. Seems like there are two special cases.

1. A local Pointer{Fwd} matches target Pointer{Struct/Union}, so that in eBPF we can stub out whole parts of a struct which we don't care about. Why do we also need to support the inverse, where a local Pointer{Struct} matches Pointer{Fwd}? In the first case we know that user code can't access anything in the target struct / union (because there is no type definition). In the second case that isn't true at all?
2. A struct / union member "behind a pointer" is compared differently. Why? See my comment below.

[lmb]

Please re-request review when you are ready.

[lmb]

LGTM!