On Fri, Apr 22, 2022 at 03:56:44AM IST, Alexei Starovoitov wrote:
> On Thu, Apr 21, 2022 at 12:36 PM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> > > > +
> > > > + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
> > > > + off_desc->kptr.btf, off_desc->kptr.btf_id))
> > > > + goto bad_type;
> > >
> > > Is full type comparison really needed?
> >
> > Yes.
> >
> > > reg->btf should be the same pointer as off_desc->kptr.btf
> > > and btf_id should match exactly.
> >
> > This is not true, it can be vmlinux or module BTF. But if you mean just
> > comparing the pointer and btf_id, we still need to handle reg->off.
> >
> > We want to support cases like:
> >
> > struct foo {
> > struct bar br;
> > struct baz bz;
> > };
> >
> > struct foo *v = func(); // PTR_TO_BTF_ID
> > map->foo = v; // reg->off is zero, btf and btf_id matches type.
> > map->bar = &v->br; // reg->off is still zero, but we need to walk and retry with
> > // first member type of struct after comparison fails.
> > map->baz = &v->bz; // reg->off is non-zero, so struct needs to be walked to
> > // match type.
> >
> > In the ref case, the argument's offset will always be 0, so third case is not
> > going to work, but in the unref case, we want to allow storing pointers to
> > structs embedded inside parent struct.
> >
> > Please let me know if I misunderstood what you meant.
>
> Makes sense.
> Please add this comment to the code.
>
I took a closer look at this, and I think we're missing one extra corner case
from the ones covered in 24d5bb806c7e, i.e. when reg->off is zero and struct is
walked to match type. This would be incorrect for release/kptr_ref case, even if
it is unlikely to occur in practice, it should be rejected by default. I
included a patch + selftest for this in v6, ptal.
--
Kartikeya
