On Wed, Apr 24, 2024 at 11:36:51AM -0700, Alexei Starovoitov wrote:
[...]
> > The OBJ_RELEASE causes check_func_arg_reg_off() to fail to verify if
> > there's a nonzero offset. In reality, I _think_ we only need to check
> > for a nonzero offset for KF_RELEASE, and possibly KF_ACQUIRE.
>
> Why special case KF_RELEASE/ACQUIRE ?
> I think they're no different from kfuncs with KF_TRUSTED_ARGS.
> Should be safe to allow non-zero offset trusted arg in all cases.
Yeah, after thinking about this some more I agree with you. All we need
to do is verify that the object at the non-zero offset has a
ref_obj_id > 0 if being passed to KF_RELEASE. No different than at
offset 0. This will be a nice usability improvement. The offset=0
restriction really does seem pointless and arbitrary, unless I'm
completely missing something.
> > > We can allow off!=0 and it won't confuse btf_type_ids_nocast_alias.
> > >
> > > struct nf_conn___init {
> > > int another_field_at_off_zero;
> > > struct nf_conn ct;
> > > };
> > >
> > > will still trigger strict_type_match as expected.
> >
> > Yes, this should continue to just work, but I think we may also have to
> > be cognizant to not allow this type of pattern:
> >
> > struct some_other_type {
> > int field_at_off_zero;
> > struct nf_conn___init ct;
> > };
> >
> > In this case, we don't want to allow &other_type->ct to be passed to a
> > kfunc expecting a struct nf_conn. So we'd also have to compare the type
> > at the register offset to make sure it's not a nocast alias, not just
> > the type in the register itself. I'm not sure if this is a problem in
> > practice. I expect it isn't. struct nf_conn___init exists solely to
> > allow the struct nf_conn kfuncs to enforce calling semantics so that an
> > uninitialized struct nf_conn object can't be passed to specific kfuncs
> > that are expecting an initialized object. I don't see why we'd ever
> > embed a wrapper type like that inside of another type. But still
> > something to be cognizant of.
>
> Agree that it's not a problem now and I wouldn't proactively
> complicate the verifier. __init types are in the kernel code and it
> gets code reviewed. So 'struct some_other_type' won't happen out of
> nowhere.
Makes sense
Attachment:
signature.asc
Description: PGP signature
