Re: [PATCH bpf-next v2 1/5] bpf: Implement file local storage

["Serge E. Hallyn" <serge@hallyn.com>]

On Mon, Aug 30, 2021 at 10:47:19AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Mon, Aug 30, 2021 at 09:53:46AM IST, Serge E. Hallyn wrote:
> > On Thu, Aug 26, 2021 at 07:09:09PM +0530, Kumar Kartikeya Dwivedi wrote:
> > > +static struct bpf_local_storage_data *
> > > +file_storage_lookup(struct file *file, struct bpf_map *map, bool cacheit_lockit)
> > > +{
> > > +	struct bpf_local_storage *file_storage;
> > > +	struct bpf_local_storage_map *smap;
> > > +	struct bpf_storage_blob *bsb;
> > > +
> > > +	bsb = bpf_file(file);
> > > +	if (!bsb)
> > > +		return NULL;
> > > +
> > > +	file_storage = rcu_dereference(bsb->storage);
> >
> > It's possible that I am (and the docs are) behind the times, or (very likely)
> > I'm missing something else, but Documentation/RCU/whatisRCU.rst says that
> > rcu_dereference result is only valid within a rcu read-side critical section.
> >
> > Here it doesn't seem like you're in a rcu_read_unlock at all.  Will the
> > callers (bpf_map_ops->map_lookup_elem) be called that way?
> >
> 
> This function will either be called from the BPF program, which is run under RCU
> protection, or from bpf_map_* bpf command, which also has rcu_read_lock
> protection (see map_copy_value, bpf_map_update_value in kernel/bpf/syscall.c
> called from map_lookup_elem, map_update_elem) when calling the map_ops.

Thanks.  That was my guess, but wanted to make sure.

(I've made a note to study map_copy_value and bpf_map_update_value, thanks)

> > > +	if (!file_storage)
> > > +		return NULL;
> > > +
> > > +	smap = (struct bpf_local_storage_map *)map;
> > > +	return bpf_local_storage_lookup(file_storage, smap, cacheit_lockit);
> > > +}
> > > +
> > > +void bpf_file_storage_free(struct file *file)
> > > +{
> > > +	struct bpf_local_storage *local_storage;
> > > +	struct bpf_local_storage_elem *selem;
> > > +	bool free_file_storage = false;
> > > +	struct bpf_storage_blob *bsb;
> > > +	struct hlist_node *n;
> > > +
> > > +	bsb = bpf_file(file);
> > > +	if (!bsb)
> > > +		return;
> > > +
> > > +	rcu_read_lock();
> > > +
> > > +	local_storage = rcu_dereference(bsb->storage);
> >
> > Here you've called rcu_read_lock, but you use the result of it,
> > 'local_storage', after dropping the rcu_read_unlock, which whatisRCU.rst
> > explicitly calls out as a bug.
> >
> 
> It is only used without rcu_read_lock protection in one place, in the branch
> that depends on 'free_file_storage', at which point we are responsible for
> freeing the local_storage after unlinking the last storage element from its
> list and resetting the owner.

Makes sense.  Both of these seem worth a brief comment in the code,
but I'll leave it to you in case you think it's so obvious it'll
just be needless clutter.

-serge