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.
> > + 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.
> [...]
--
Kartikeya
