[PATCH bpf-next 0/2] Fix psock update-side data races on sk->sk_prot and progs

[PATCH bpf-next 0/2] Fix psock update-side data races on sk->sk_prot and progs

[Jakub Sitnicki]

Follow up to the thread at [1]. See individual patches for details.

[1] https://lore.kernel.org/bpf/20261022054811.35008-1-phx0fer@gmail.com/

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
Jakub Sitnicki (2):
      selftests/bpf: Cover concurrent sockmap update and delete
      bpf, sockmap: Fix psock update-side data races on sk->sk_prot and progs

 net/core/sock_map.c                                | 22 ++++---
 .../selftests/bpf/prog_tests/sockmap_listen.c      | 77 ++++++++++++++++++++++
 2 files changed, 90 insertions(+), 9 deletions(-)

[PATCH bpf-next 1/2] selftests/bpf: Cover concurrent sockmap update and delete

[Jakub Sitnicki]

Add a sockmap_listen subtest where one thread keeps inserting a TCP
socket into a sockmap with an attached verdict program while another
thread deletes the same map entry.

Without any synchronization between the insert and delete paths, both
end up racing on psock teardown and the socket's proto callbacks,
which KCSAN reports as:

    ==================================================================
    BUG: KCSAN: data-race in sock_map_link / tcp_bpf_update_proto

    write (marked) to 0xffff88810b1f42a8 of 8 bytes by task 198 on cpu 0:
     tcp_bpf_update_proto+0x278/0x400
     sock_map_unref+0x1d3/0x240
     sock_map_delete_elem+0x7b/0xb0
     map_delete_elem+0x1e1/0x270
     __sys_bpf+0x71a/0x870
     __x64_sys_bpf+0x69/0x80
     x64_sys_call+0xf26/0x2550
     do_syscall_64+0xb8/0x2f0
     entry_SYSCALL_64_after_hwframe+0x76/0x7e

    read to 0xffff88810b1f42a8 of 8 bytes by task 194 on cpu 3:
     sock_map_link+0x146/0x5b0
     sock_map_update_common+0x74/0x2b0
     sock_map_update_elem_sys+0x246/0x2d0
     bpf_map_update_value+0x80/0x400
     map_update_elem+0x35c/0x3e0
     __sys_bpf+0x540/0x870
     __x64_sys_bpf+0x69/0x80
     x64_sys_call+0xf26/0x2550
     do_syscall_64+0xb8/0x2f0
     entry_SYSCALL_64_after_hwframe+0x76/0x7e

    value changed: 0xffffffff857540c0 -> 0xffffffff8468fde8

    Reported by Kernel Concurrency Sanitizer on:
    CPU: 3 UID: 0 PID: 194 Comm: test_progs Tainted: G           OE        7.3.0+ #12 PREEMPT(full)
    Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
    ==================================================================

Deleting an entry that is already gone is expected: sockmap reports
EINVAL and sockhash reports ENOENT. Re-inserting a socket whose psock
still holds the verdict program from a concurrent delete fails with
EBUSY. Tolerate these errnos; the test relies on KCSAN as the witness.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../selftests/bpf/prog_tests/sockmap_listen.c      | 77 ++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 235e1d56f044..dca1f0de92e0 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -646,6 +646,82 @@ static void test_race_insert_listen(struct test_sockmap_listen *skel __always_un
 	xclose(s);
 }
 
+struct update_delete_ctx {
+	unsigned int nr_iter;
+	unsigned int done;
+	int mapfd;
+};
+
+static void *delete_elem_thread(void *arg)
+{
+	struct update_delete_ctx *ctx = arg;
+	unsigned int i;
+
+	for (i = 0; i < ctx->nr_iter; i++) {
+		/* Expecting EINVAL (sockmap) or ENOENT (sockhash) when the
+		 * entry is already gone.
+		 */
+		if (bpf_map_delete_elem(ctx->mapfd, &(u32){0}) &&
+		    errno != EINVAL && errno != ENOENT) {
+			FAIL_ERRNO("map_delete");
+			break;
+		}
+	}
+
+	WRITE_ONCE(ctx->done, 1);
+	return NULL;
+}
+
+/* One thread keeps inserting a socket into a sockmap with an attached
+ * verdict program while another thread deletes the same map entry.
+ */
+static void test_update_delete_with_prog(struct test_sockmap_listen *skel,
+					 int family, int sotype, int mapfd)
+{
+	struct update_delete_ctx ctx = { 0 };
+	int err, s, verdict;
+	pthread_t t;
+	u64 value;
+
+	verdict = bpf_program__fd(skel->progs.prog_skb_verdict);
+	err = xbpf_prog_attach(verdict, mapfd, BPF_SK_SKB_VERDICT, 0);
+	if (err)
+		return;
+
+	s = xsocket(family, sotype, 0);
+	if (s < 0)
+		goto detach;
+
+	err = xlisten(s, 1);
+	if (err)
+		goto close;
+
+	ctx.mapfd = mapfd;
+	ctx.nr_iter = 10000;
+
+	err = xpthread_create(&t, NULL, delete_elem_thread, &ctx);
+	if (err)
+		goto close;
+
+	value = s;
+	while (!READ_ONCE(ctx.done)) {
+		/* Expecting EBUSY while a deleted socket's psock still
+		 * holds the verdict program.
+		 */
+		err = bpf_map_update_elem(mapfd, &(u32){0}, &value, BPF_ANY);
+		if (err && errno != EBUSY) {
+			FAIL_ERRNO("map_update");
+			break;
+		}
+	}
+
+	xpthread_join(t, NULL);
+close:
+	xclose(s);
+detach:
+	xbpf_prog_detach2(verdict, mapfd, BPF_SK_SKB_VERDICT);
+}
+
 static void zero_verdict_count(int mapfd)
 {
 	unsigned int zero = 0;
@@ -1289,6 +1365,7 @@ static void test_ops(struct test_sockmap_listen *skel, struct bpf_map *map,
 		TEST(test_destroy_orphan_child, SOCK_STREAM),
 		TEST(test_syn_recv_insert_delete, SOCK_STREAM),
 		TEST(test_race_insert_listen, SOCK_STREAM),
+		TEST(test_update_delete_with_prog, SOCK_STREAM),
 		/* child clone */
 		TEST(test_clone_after_delete, SOCK_STREAM),
 		TEST(test_accept_after_delete, SOCK_STREAM),

-- 
2.43.0

[PATCH bpf-next 2/2] bpf, sockmap: Fix psock update-side data races on sk->sk_prot and progs

[Jakub Sitnicki]

The update side of the sockmap psock lifecycle has several unsynchronized
accesses to sk->sk_prot and psock prog state that KCSAN flags when the
new update-vs-delete race selftest runs. The fix comes from Alexei [1]:

* sock_map_link() installed psock programs and called sock_map_init_proto()
  before taking sk->sk_callback_lock, racing a concurrent teardown's
  tcp_bpf_update_proto() write of sk->sk_prot. Take the write_lock early,
  before psock_set_prog() and sock_map_init_proto(), and drop the
  psock->saved_data_ready gating from prog detection (it is re-checked
  under the lock before stopping progs).

* sock_map_del_link() read psock->saved_data_ready outside the lock. Move
  the saved_data_ready check under write_lock_bh(&sk->sk_callback_lock)
  so prog teardown is serialized against the proto restore.

* sock_map_psock_get_checked() and sock_map_sk_is_suitable() did plain
  sk->sk_prot reads. Convert to READ_ONCE() to mark the intentional racy
  read against the WRITE_ONCE() swap in sock_replace_proto().

With these changes the sock_map_link/psock_set_prog side of the reported
KCSAN splats is silenced; the remaining teardown-vs-core-TCP pairings
are the separate read-side annotation problem.

[1] https://lore.kernel.org/bpf/DMBHB0TQOV1L.38MECMYFGUS5B@gmail.com

Suggested-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/sock_map.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 057e2f3c0265..efe49551caba 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -152,11 +152,11 @@ static void sock_map_del_link(struct sock *sk,
 			struct bpf_map *map = link->map;
 			struct sk_psock_progs *progs = sock_map_progs(map);
 
-			if (psock->saved_data_ready && progs->stream_parser)
+			if (progs->stream_parser)
 				strp_stop = true;
-			if (psock->saved_data_ready && progs->stream_verdict)
+			if (progs->stream_verdict)
 				verdict_stop = true;
-			if (psock->saved_data_ready && progs->skb_verdict)
+			if (progs->skb_verdict)
 				verdict_stop = true;
 			list_del(&link->list);
 			sk_psock_free_link(link);
@@ -164,8 +164,11 @@ static void sock_map_del_link(struct sock *sk,
 		}
 	}
 	spin_unlock_bh(&psock->link_lock);
-	if (strp_stop || verdict_stop) {
-		write_lock_bh(&sk->sk_callback_lock);
+	if (!strp_stop && !verdict_stop)
+		return;
+
+	write_lock_bh(&sk->sk_callback_lock);
+	if (psock->saved_data_ready) {
 		if (strp_stop)
 			sk_psock_stop_strp(sk, psock);
 		if (verdict_stop)
@@ -173,8 +176,8 @@ static void sock_map_del_link(struct sock *sk,
 
 		if (psock->psock_update_sk_prot)
 			psock->psock_update_sk_prot(sk, psock, false);
-		write_unlock_bh(&sk->sk_callback_lock);
 	}
+	write_unlock_bh(&sk->sk_callback_lock);
 }
 
 static void sock_map_unref(struct sock *sk, void *link_raw)
@@ -202,7 +205,7 @@ static struct sk_psock *sock_map_psock_get_checked(struct sock *sk)
 	rcu_read_lock();
 	psock = sk_psock(sk);
 	if (psock) {
-		if (sk->sk_prot->close != sock_map_close) {
+		if (READ_ONCE(sk->sk_prot)->close != sock_map_close) {
 			psock = ERR_PTR(-EBUSY);
 			goto out;
 		}
@@ -284,6 +287,7 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
 		}
 	}
 
+	write_lock_bh(&sk->sk_callback_lock);
 	if (msg_parser)
 		psock_set_prog(&psock->progs.msg_parser, msg_parser);
 	if (stream_parser)
@@ -298,11 +302,11 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
 	 */
 	ret = sock_map_init_proto(sk, psock);
 	if (ret < 0) {
+		write_unlock_bh(&sk->sk_callback_lock);
 		sk_psock_put(sk, psock);
 		goto out;
 	}
 
-	write_lock_bh(&sk->sk_callback_lock);
 	if (stream_parser && stream_verdict && !psock->saved_data_ready) {
 		if (sk_is_tcp(sk))
 			ret = sk_psock_init_strp(sk, psock);
@@ -536,7 +540,7 @@ static bool sock_map_redirect_allowed(const struct sock *sk)
 
 static bool sock_map_sk_is_suitable(const struct sock *sk)
 {
-	return !!sk->sk_prot->psock_update_sk_prot;
+	return !!READ_ONCE(sk->sk_prot)->psock_update_sk_prot;
 }
 
 static bool sock_map_sk_state_allowed(const struct sock *sk)

-- 
2.43.0

Re: [PATCH bpf-next 2/2] bpf, sockmap: Fix psock update-side data races on sk->sk_prot and progs

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3257 bytes --]

> bpf, sockmap: Fix psock update-side data races on sk->sk_prot and progs

TL;DR: Commit message only: the first bullet says the saved_data_ready gating
was dropped in sock_map_link(), but the diff drops it in sock_map_del_link(),
duplicating the second bullet.

>     bpf, sockmap: Fix psock update-side data races on sk->sk_prot and progs
>
>     The update side of the sockmap psock lifecycle has several unsynchronized
>     accesses to sk->sk_prot and psock prog state that KCSAN flags when the
>     new update-vs-delete race selftest runs. The fix comes from Alexei [1]:
>
>     * sock_map_link() installed psock programs and called sock_map_init_proto()
>       before taking sk->sk_callback_lock, racing a concurrent teardown's
>       tcp_bpf_update_proto() write of sk->sk_prot. Take the write_lock early,
>       before psock_set_prog() and sock_map_init_proto(), and drop the
>       psock->saved_data_ready gating from prog detection (it is re-checked
>       under the lock before stopping progs).
>
>     * sock_map_del_link() read psock->saved_data_ready outside the lock. Move
>       the saved_data_ready check under write_lock_bh(&sk->sk_callback_lock)
>       so prog teardown is serialized against the proto restore.
>
>     * sock_map_psock_get_checked() and sock_map_sk_is_suitable() did plain
>       sk->sk_prot reads. Convert to READ_ONCE() to mark the intentional racy
>       read against the WRITE_ONCE() swap in sock_replace_proto().
>
>     With these changes the sock_map_link/psock_set_prog side of the reported
>     KCSAN splats is silenced; the remaining teardown-vs-core-TCP pairings
>     are the separate read-side annotation problem.
>
>     [1] https://lore.kernel.org/bpf/DMBHB0TQOV1L.38MECMYFGUS5B@gmail.com
>
>     Suggested-by: Alexei Starovoitov <ast@kernel.org>
>     Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>

The first bullet says sock_map_link() drops the psock->saved_data_ready
gating from prog detection, which is "re-checked under the lock before
stopping progs".  Is that sentence describing the right function?

The diff doesn't change sock_map_link() that way.  Its only change is
moving write_lock_bh(&sk->sk_callback_lock) above the psock_set_prog()
calls, plus the matching unlock on the sock_map_init_proto() error path.
sock_map_link() still gates on !psock->saved_data_ready:

    if (stream_parser && stream_verdict && !psock->saved_data_ready) {

The gating that was dropped is in sock_map_del_link():

    -	if (psock->saved_data_ready && progs->stream_parser)
    +	if (progs->stream_parser)
     		strp_stop = true;
    ...
    +	write_lock_bh(&sk->sk_callback_lock);
    +	if (psock->saved_data_ready) {

The second bullet already covers the sock_map_del_link() lock and
saved_data_ready change, so the sentence in the first bullet looks both
misplaced and repeated.

Could it be moved to (or merged with) the sock_map_del_link() bullet, so
that a reader doesn't conclude the saved_data_ready checks were removed
from sock_map_link()?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41053761520