Re: [PATCH bpf-next v2 04/15] bpf: Allow storing referenced kptr in map

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: bpf@vger.kernel.org, "Alexei Starovoitov" <ast@kernel.org>,
	"Andrii Nakryiko" <andrii@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>
Subject: Re: [PATCH bpf-next v2 04/15] bpf: Allow storing referenced kptr in map
Date: Sun, 20 Mar 2022 03:13:03 +0530	[thread overview]
Message-ID: <20220319214303.n3aogzbz3fjaohcy@apollo> (raw)
In-Reply-To: <20220319212320.oos7tao6mybsjt6i@ast-mbp.dhcp.thefacebook.com>

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