On Sun, Mar 20, 2022 at 03:13:03AM IST, Kumar Kartikeya Dwivedi wrote:
> On Sun, Mar 20, 2022 at 02:53:20AM IST, Alexei Starovoitov wrote:
> > On Sun, Mar 20, 2022 at 12:29:04AM +0530, Kumar Kartikeya Dwivedi wrote:
> > > > >
> > > > > if (is_release_function(func_id)) {
> > > > > - err = release_reference(env, meta.ref_obj_id);
> > > > > + err = -EINVAL;
> > > > > + if (meta.ref_obj_id)
> > > > > + err = release_reference(env, meta.ref_obj_id);
> > > > > + /* Only bpf_kptr_xchg is a release function that accepts a
> > > > > + * possibly NULL reg, hence meta.ref_obj_id can only be unset
> > > > > + * for it.
> > > >
> > > > Could you rephrase the comment? I'm not following what it's trying to convey.
> > > >
> > >
> > > All existing release helpers never take a NULL register, so their
> > > meta.ref_obj_id will never be unset, but bpf_kptr_xchg can, so it needs some
> > > special handling. In check_func_arg, when it jumps to skip_type_check label,
> > > reg->ref_obj_id won't be set for NULL value.
> >
> > I still don't follow.
> > What do you mean 'unset meta.ref_obj_id' ?
> > It's either set or not.
>
> By unset I meant it is the default (0).
>
> > meta->ref_obj_id will stay zero when arg == NULL.
> > Above 'if (meta.ref_obj_id)' makes sense.
> > But the code below with extra func_id check looks like defensive programming again.
> >
>
> Ok, so I'll just write it like:
>
> if (is_release_function(...) && meta.ref_obj_id) {
> err = release_reference(...);
> if (err)
> ...
> }
>
Ah, after reworking, bpf_sk_release(listen_sk) in verifier/ref_tracking.c is
failing, now I remember why I did it this way.
So meta.ref_obj_id may be 0 in many other cases, where reg->ref_obj_id is 0, not
just for the NULL register, so we need to special case the bpf_kptr_xchg. User
cannot pass any other reg with ref_obj_id == 0 to it because verifier will check
type to be PTR_TO_BTF_ID_OR_NULL, if register is not NULL.
> > > > > + */
> > > > > + else if (func_id == BPF_FUNC_kptr_xchg)
> > > > > + err = 0;
>
> --
> Kartikeya
--
Kartikeya
