On Tue, Jul 05, 2022 at 09:16:27PM -0700, Andrii Nakryiko wrote:
> On Tue, Jul 5, 2022 at 2:07 PM Daniel Müller <deso@xxxxxxxxxx> wrote:
> >
> > On Tue, Jun 28, 2022 at 04:01:17PM +0000, Daniel Müller wrote:
> > > This patch set proposes the addition of a new way for performing type queries to
> > > BPF. It introduces the "type matches" relation, similar to what is already
> > > present with "type exists" (in the form of bpf_core_type_exists).
> > >
> > > "type exists" performs fairly superficial checking, mostly concerned with
> > > whether a type exists in the kernel and is of the same kind (enum/struct/...).
> > > Notably, compatibility checks for members of composite types is lacking.
> > >
> > > The newly introduced "type matches" (bpf_core_type_matches) fills this gap in
> > > that it performs stricter checks: compatibility of members and existence of
> > > similarly named enum variants is checked as well. E.g., given these definitions:
> > >
> > > struct task_struct___og { int pid; int tgid; };
> > >
> > > struct task_struct___foo { int foo; }
> > >
> > > 'task_struct___og' would "match" the kernel type 'task_struct', because the
> > > members match up, while 'task_struct___foo' would not match, because the
> > > kernel's 'task_struct' has no member named 'foo'.
> > >
> > > More precisely, the "type match" relation is defined as follows (copied from
> > > source):
> > > - modifiers and typedefs are stripped (and, hence, effectively ignored)
> > > - generally speaking types need to be of same kind (struct vs. struct, union
> > > vs. union, etc.)
> > > - exceptions are struct/union behind a pointer which could also match a
> > > forward declaration of a struct or union, respectively, and enum vs.
> > > enum64 (see below)
> > > Then, depending on type:
> > > - integers:
> > > - match if size and signedness match
> > > - arrays & pointers:
> > > - target types are recursively matched
> > > - structs & unions:
> > > - local members need to exist in target with the same name
> > > - for each member we recursively check match unless it is already behind a
> > > pointer, in which case we only check matching names and compatible kind
> > > - enums:
> > > - local variants have to have a match in target by symbolic name (but not
> > > numeric value)
> > > - size has to match (but enum may match enum64 and vice versa)
> > > - function pointers:
> > > - number and position of arguments in local type has to match target
> > > - for each argument and the return value we recursively check match
> > >
> > > Enabling this feature requires a new relocation to be made known to the
> > > compiler. This is being taken care of for LLVM as part of
> > > https://reviews.llvm.org/D126838.
> >
> > To give an update here, LLVM changes have been merged and, to the best of my
> > knowledge, are being used by BPF CI (tests that failed earlier are now passing).
> >
>
> I did a few small changes and combined patches 4-6 together (because
> they add the same functionality to both libbpf and kernel
> simultaneously, there were compilation warnings about non-static
> functions not having a proper prototype defined). But I've split out
> the bpf_core_type_matches() macro in bpf_core_read.h into a separate
> patch. I also dropped patch #3 as it wasn't needed anymore.
>
> Please see comments I left for two further follow ups.
Sounds good. Will address your comments soon. Thanks for merging!
Daniel
