On Tue, Feb 13, 2024 at 9:08 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Tue, 2024-02-13 at 09:02 -0800, Andrii Nakryiko wrote:
> [...]
>
> > > t = btf_type_by_id(btf, t->type);
> > > - while (btf_type_is_modifier(t))
> > > - t = btf_type_by_id(btf, t->type);
> > > - if (!btf_type_is_struct(t)) {
> > > +
> > > + /* Skip modifiers, but stop if skipping of typedef would
> > > + * lead an anonymous type, e.g. like for s390:
> > > + *
> > > + * typedef struct { ... } user_pt_regs;
> > > + * typedef user_pt_regs bpf_user_pt_regs_t;
> > > + */
> > > + t = __btf_type_skip_qualifiers(btf, t);
> > > + while (btf_type_is_typedef(t)) {
> > > + const struct btf_type *t1;
> > > +
> > > + t1 = btf_type_by_id(btf, t->type);
> > > + t1 = __btf_type_skip_qualifiers(btf, t1);
> > > + tname = btf_name_by_offset(btf, t1->name_off);
> > > + if (!tname || tname[0] == '\0')
> > > + break;
> > > + t = t1;
> > > + }
> > > + if (!btf_type_is_struct(t) && !btf_type_is_typedef(t)) {
> >
> > and now we potentially are intermixing structs and typedefs and don't
> > really distinguish them later (but struct abc is not the same thing as
> > typedef abc), which is probably not what we want.
>
> Yes, need a condition in the end to check that 'ctx_type' and 't' have
> same kind.
Yeah, and then special case, for KPROBE that `struct
bpf_user_pt_regs_t` (not a typedef!) is also acceptable. Hopefully you
see why I'm saying that the early special case of `bpf_user_pt_regs_t`
typedef is much easier to reason about.
>
> > Also, we resolve typedef to its underlying type *or* typedef, right?
> > So if I have typedef A -> typedef B -> struct C, we won't get to
> > struct C, even if struct C is the expected correct context type for a
> > given program type (and it should work).
>
> For code above we would get to 'struct C'.
ha, right, it's "break if empty name", not the other way.
So basically in `typedef A -> typedef B -> struct C` we get `typedef
B` if C is anon, otherwise it's `struct C`. It will be a slightly
different behavior for bpf_user_pt_regs_t if the user typedef'ed it
(for whatever reason).
>
> > So in general, yes, I think this code could be changed to not ignore
> > typedefs and do the right thing, but we'd need to be very careful to
> > not allow unexpected things for all program types. Given only kprobes
> > define context as typedef, it's simpler and more reliable to special
> > case kprobe, IMO.
>
> Maybe, I do not insist.
Great.
