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)
...
}
> > > > + */
> > > > + else if (func_id == BPF_FUNC_kptr_xchg)
> > > > + err = 0;
--
Kartikeya
