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:

quoted

+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.

quoted

+	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