btf: Add support for bpf_core_type_matches()#1366
Conversation
|
I tested this PR in inspektor-gadget/inspektor-gadget#2542 and it seems to work for me. cc @eiffel-fl |
|
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
left a comment
eiffel-fl
left a comment
There was a problem hiding this comment.
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 a comment
lmb
left a comment
There was a problem hiding this comment.
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.
|
Also see #896 for a similar (but not exact due to ignoring names) use case. |
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
force-pushed
the
feature/core-type-matches
branch
from
9997f25 to
7b38277
Compare
|
Did an initial pass over the comments.
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. |
dylandreimerink
force-pushed
the
feature/core-type-matches
branch
from
7b38277 to
6c82a36
Compare
|
@dylandreimerink PTAL #1383 where I rewrite coreAreTypesCompatible to use recursion, which ends up being a lot simpler. Can we use the same approach here?
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. |
dylandreimerink
force-pushed
the
feature/core-type-matches
branch
from
6c82a36 to
f9c4eb4
Compare
There was a problem hiding this comment.
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?
- build a map[string]Member (or similar)
- 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.
- A local
Pointer{Fwd}matches targetPointer{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 localPointer{Struct}matchesPointer{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? - A struct / union member "behind a pointer" is compared differently. Why? See my comment below.
dylandreimerink
force-pushed
the
feature/core-type-matches
branch
from
f9c4eb4 to
304ddbf
Compare
|
Please re-request review when you are ready. |
dylandreimerink
force-pushed
the
feature/core-type-matches
branch
4 times, most recently
from
eea76c3 to
86d6c4c
Compare
dylandreimerink
force-pushed
the
feature/core-type-matches
branch
2 times, most recently
from
1d1096b to
f3bfaa5
Compare
This commit 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. Signed-off-by: Dylan Reimerink <[email protected]> Co-authored-by: Lorenz Bauer <[email protected]>
dylandreimerink
force-pushed
the
feature/core-type-matches
branch
from
f3bfaa5 to
fb7f754
Compare
4 participants
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_inserttracepoint. 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/[email protected]/
Fixes: #1360