Re: [PATCH v2 bpf-next 2/4] bpf: handle bpf_user_pt_regs_t typedef explicitly for PTR_TO_CTX global arg

[Eduard Zingerman <eddyz87@gmail.com>]

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.

> 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'.

> 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.