bpf: Link the first element after its storage is published

bpf_local_storage_alloc() links the first element of a new storage to the
bucket of its map, and then publishes the storage in the owner with
cmpxchg().  If another cpu has been faster, the element is taken out of
the bucket again and is freed together with the storage.

In between the iterator over a sk_storage map can find the element.  It
stays linked to a storage that points to the socket, but the socket knows
nothing of that storage: bpf_sk_storage_free() neither unlinks the element
nor waits for anybody who uses the storage.  So nothing tells the iterator
that the socket has gone away.

Three cpus that create the first storage of a new AF_UNIX socket at the
same time, and a fourth that reads the iterator, are enough: in 20 seconds
its program was shown some 4000 elements of storages that had lost.  If the
socket is closed in time, the program is handed a freed socket.

Link the element once the storage is published.  Both happens under the
lock of the bucket, which holds off whoever finds the element through the
owner and wants to unlink it.

Fixes: c69d2ddb2072 ("bpf: Using rcu_read_lock for bpf_sk_storage_map iterator")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Link: https://patch.msgid.link/20261017163745.3341787-17-alexei.starovoitov@gmail.com

1 files changed, 7 insertions, 3 deletions

diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
index 7721834e4bff..c08afb33d038 100644
--- a/kernel/bpf/bpf_local_storage.c
+++ b/kernel/bpf/bpf_local_storage.c
@@ -509,8 +509,6 @@ int bpf_local_storage_alloc(void *owner,
 	if (err)
 		goto uncharge;
 
-	bpf_selem_link_map_nolock(b, first_selem);
-
 	owner_storage_ptr =
 		(struct bpf_local_storage **)owner_storage(smap, owner);
 	/* Publish storage to the owner.
@@ -525,11 +523,17 @@ int bpf_local_storage_alloc(void *owner,
 	 */
 	prev_storage = cmpxchg(owner_storage_ptr, NULL, storage);
 	if (unlikely(prev_storage)) {
-		bpf_selem_unlink_map_nolock(first_selem);
 		raw_res_spin_unlock_irqrestore(&b->lock, flags);
 		err = -EAGAIN;
 		goto uncharge;
 	}
+	/*
+	 * Not before the storage is published: the owner does not wait for
+	 * users of a storage that it knows nothing of, so that nobody may find
+	 * such a storage through the map. b->lock holds off those who find
+	 * first_selem through the owner and want to unlink it.
+	 */
+	bpf_selem_link_map_nolock(b, first_selem);
 	raw_res_spin_unlock_irqrestore(&b->lock, flags);
 
 	return 0;