* Re: [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path
2026-10-21 15:56 [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path Dudu Lu
@ 2026-10-21 16:10 ` sashiko-bot
2026-10-21 16:37 ` bot+bpf-ci
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-21 16:10 UTC (permalink / raw)
To: Dudu Lu; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf, sockmap: incomplete data-race fix in sk_psock_start_verdict() and sock_map_unref()
--
commit 3aa9e245b4ee20f4b50e92e1de66b46757cb8a03
Author: Dudu Lu <phx0fer@gmail.com>
bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path
This commit annotates accesses to psock->saved_data_ready in
sk_psock_start_verdict() and sk_psock_stop_verdict() to address data races
with sock_map_link() and sock_map_unref().
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index df385a5a961e8..0bfb8c4e7f36d 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -1260,10 +1260,10 @@ static void sk_psock_verdict_data_ready(struct sock *sk)
>
> void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock)
> {
> - if (READ_ONCE(psock->saved_data_ready))
> + if (READ_ONCE(psock->saved_data_ready))
> return;
>
> - psock->saved_data_ready = sk->sk_data_ready;
> + WRITE_ONCE(psock->saved_data_ready, sk->sk_data_ready);
> WRITE_ONCE(sk->sk_data_ready, sk_psock_verdict_data_ready);
> WRITE_ONCE(sk->sk_write_space, sk_psock_write_space);
> }
> @@ -1273,9 +1273,9 @@ void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock)
> psock_set_prog(&psock->progs.stream_verdict, NULL);
> psock_set_prog(&psock->progs.skb_verdict, NULL);
>
> - if (!psock->saved_data_ready)
> + if (!READ_ONCE(psock->saved_data_ready))
> return;
>
> WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready);
> - psock->saved_data_ready = NULL;
> + WRITE_ONCE(psock->saved_data_ready, NULL);
> }
[Severity: Medium]
When a socket is concurrently added to and removed from sockmaps via
sock_map_update_elem() -> sock_map_link() and sock_map_delete_elem() ->
sock_map_unref() -> sock_map_del_link(), does this change actually resolve
the reported data races?
Every caller of sk_psock_start_verdict(), sk_psock_stop_verdict(),
sk_psock_start_strp(), and sk_psock_stop_strp() (namely sock_map_link() and
sock_map_del_link() in net/core/sock_map.c, and sk_psock_drop() in
net/core/skmsg.c) already holds write_lock_bh(&sk->sk_callback_lock).
Because all writes to psock->saved_data_ready are serialized by
sk->sk_callback_lock, the reads in sk_psock_start_verdict() and
sk_psock_stop_verdict() cannot race with a concurrent writer.
Meanwhile, sk_psock_stop_verdict() in net/core/skmsg.c still reads
psock->saved_data_ready with an unannotated plain load when restoring
sk->sk_data_ready:
if (!READ_ONCE(psock->saved_data_ready))
return;
WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready);
And the sibling functions sk_psock_start_strp() and sk_psock_stop_strp() in
net/core/skmsg.c, as well as sock_map_link() in net/core/sock_map.c, also
use plain loads and stores for psock->saved_data_ready:
net/core/skmsg.c:sk_psock_start_strp() {
if (psock->saved_data_ready)
return;
psock->saved_data_ready = sk->sk_data_ready;
...
}
More importantly, the unlocked reads of psock->saved_data_ready that race
with sk_psock_start_verdict() happen in sock_map_unref() ->
sock_map_del_link() in net/core/sock_map.c under
spin_lock_bh(&psock->link_lock), before write_lock_bh(&sk->sk_callback_lock)
is acquired:
net/core/sock_map.c:sock_map_del_link() {
...
spin_lock_bh(&psock->link_lock);
list_for_each_entry_safe(link, tmp, &psock->link, list) {
if (link->link_raw == link_raw) {
struct bpf_map *map = link->map;
struct sk_psock_progs *progs = sock_map_progs(map);
if (psock->saved_data_ready && progs->stream_parser)
strp_stop = true;
if (psock->saved_data_ready && progs->stream_verdict)
verdict_stop = true;
if (psock->saved_data_ready && progs->skb_verdict)
verdict_stop = true;
...
}
}
spin_unlock_bh(&psock->link_lock);
if (strp_stop || verdict_stop) {
write_lock_bh(&sk->sk_callback_lock);
if (strp_stop)
sk_psock_stop_strp(sk, psock);
if (verdict_stop)
sk_psock_stop_verdict(sk, psock);
...
write_unlock_bh(&sk->sk_callback_lock);
}
}
Because those reads in sock_map_del_link() remain unannotated plain loads
outside sk->sk_callback_lock, won't the data race between
sk_psock_start_verdict() and sock_map_unref() still trigger? In addition,
if sock_map_del_link() checks psock->saved_data_ready before taking
write_lock_bh(&sk->sk_callback_lock), can it observe psock->saved_data_ready
newly set by a concurrent sock_map_link() and then call
sk_psock_stop_verdict(), clearing psock->progs.stream_verdict and
psock->progs.skb_verdict and restoring sk->sk_data_ready for the newly
added link?
Finally, regarding the race between sock_map_link() and sock_map_unref()
mentioned in the commit description, doesn't that race occur in
net/core/sock_map.c where sock_map_link() -> sock_map_init_proto() writes
psock->psock_update_sk_prot without sk->sk_callback_lock while
sock_map_unref() -> sock_map_del_link() reads it under sk->sk_callback_lock:
net/core/sock_map.c:sock_map_init_proto() {
if (!sk->sk_prot->psock_update_sk_prot)
return -EINVAL;
psock->psock_update_sk_prot = sk->sk_prot->psock_update_sk_prot;
return sk->sk_prot->psock_update_sk_prot(sk, psock, false);
}
which is not touched by this patch?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261021155629.68813-1-phx0fer@gmail.com?part=1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path
2026-10-21 15:56 [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path Dudu Lu
2026-10-21 16:10 ` sashiko-bot
@ 2026-10-21 16:37 ` bot+bpf-ci
2026-10-21 16:45 ` Alexei Starovoitov
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: bot+bpf-ci @ 2026-10-21 16:37 UTC (permalink / raw)
To: phx0fer, john.fastabend, jakub, jiayuan.chen
Cc: netdev, bpf, ast, andrii, daniel, martin.lau, eddyz87,
yonghong.song, mason, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]
> bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path
TL;DR: Commit message only: the Fixes: tag may point at the wrong commit;
5a685cd94b21 may be the better target (medium confidence), though 2ef2b20cf4e0
may be deliberate for stable backporting. The code change itself is not
criticized.
> bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path
>
> sk_psock_start_verdict() and sk_psock_stop_verdict() access
> psock->saved_data_ready with plain loads and stores, while the sibling
> sk_psock_stop_strp() uses WRITE_ONCE for the same field. sock_map_link()
> / sock_map_unref() touch the same psock from another CPU without the
> lock, so KCSAN reports the race:
>
> BUG: KCSAN: data-race in sock_map_link / sock_map_unref
> BUG: KCSAN: data-race in sk_psock_start_verdict / sock_map_unref
>
> Use READ_ONCE() for the reads and WRITE_ONCE() for the writes, matching
> the strp sibling.
>
> Fixes: 2ef2b20cf4e0 ("net: annotate data-races around sk->sk_{data_ready,write_space}")
> Assisted-by: LLM
> Signed-off-by: Dudu Lu <phx0fer@gmail.com>
Is the Fixes: tag pointing at the right commit?
2ef2b20cf4e0 only annotated the sk->sk_data_ready and sk->sk_write_space
accesses, plus the saved_data_ready store in sk_psock_stop_strp(). Its
parent already had plain loads and stores of psock->saved_data_ready in
sk_psock_start_verdict() and sk_psock_stop_verdict(), so it left the verdict
path unannotated rather than introducing the accesses changed here.
The psock->saved_data_ready field, and the lockless reads of it that the
KCSAN reports are about, came from 5a685cd94b21 ("skmsg: Get rid of struct
sk_psock_parser"), which folded parser.saved_data_ready and parser.enabled
into psock->saved_data_ready. Should the tag be:
Fixes: 5a685cd94b21 ("skmsg: Get rid of struct sk_psock_parser")
The verdict start/stop code itself dates back to ef5659280eb1, but there the
unlocked reader tested parser.enabled rather than saved_data_ready, so this
attribution is a medium confidence suggestion. It is also possible that
2ef2b20cf4e0 was chosen deliberately, since the context lines of this patch
depend on it for stable backporting.
---
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/39631503837
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path
2026-10-21 15:56 [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path Dudu Lu
2026-10-21 16:10 ` sashiko-bot
2026-10-21 16:37 ` bot+bpf-ci
@ 2026-10-21 16:45 ` Alexei Starovoitov
2026-10-21 20:30 ` Jakub Sitnicki
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Alexei Starovoitov @ 2026-10-21 16:45 UTC (permalink / raw)
To: Dudu Lu, john.fastabend, jakub, jiayuan.chen; +Cc: netdev, bpf
On Wed, Oct 21, 2026 at 08:56 AM Dudu Lu <phx0fer@gmail.com> wrote:
> sk_psock_stop_strp() uses WRITE_ONCE for the same field. sock_map_link()
> / sock_map_unref() touch the same psock from another CPU without the
> lock, so KCSAN reports the race:
>
> BUG: KCSAN: data-race in sock_map_link / sock_map_unref
> BUG: KCSAN: data-race in sk_psock_start_verdict / sock_map_unref
sashiko is right. KCSAN will print both with this patch applied.
pw-bot: cr
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path
2026-10-21 15:56 [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path Dudu Lu
` (2 preceding siblings ...)
2026-10-21 16:45 ` Alexei Starovoitov
@ 2026-10-21 20:30 ` Jakub Sitnicki
2026-10-22 3:37 ` Jiayuan Chen
2026-10-22 5:48 ` [PATCH bpf v2] bpf, sockmap: annotate the psock teardown data races Dudu Lu
5 siblings, 0 replies; 10+ messages in thread
From: Jakub Sitnicki @ 2026-10-21 20:30 UTC (permalink / raw)
To: Dudu Lu; +Cc: john.fastabend, jiayuan.chen, netdev, bpf
On Wed, Oct 21, 2026 at 11:56 PM +08, Dudu Lu wrote:
> sk_psock_start_verdict() and sk_psock_stop_verdict() access
> psock->saved_data_ready with plain loads and stores, while the sibling
> sk_psock_stop_strp() uses WRITE_ONCE for the same field. sock_map_link()
> / sock_map_unref() touch the same psock from another CPU without the
> lock, so KCSAN reports the race:
>
> BUG: KCSAN: data-race in sock_map_link / sock_map_unref
> BUG: KCSAN: data-race in sk_psock_start_verdict / sock_map_unref
>
> Use READ_ONCE() for the reads and WRITE_ONCE() for the writes, matching
> the strp sibling.
>
> Fixes: 2ef2b20cf4e0 ("net: annotate data-races around sk->sk_{data_ready,write_space}")
> Assisted-by: LLM
> Signed-off-by: Dudu Lu <phx0fer@gmail.com>
> ---
> Tested base
> -----------
> Detected on Linux 7.3-rc4 (commit 93f51579e7df), arm64 QEMU guest.
> Re-checked against mainline f47d223627f8 (v7.3-341): the affected code
> is unchanged between the two, so the bug is still present there.
> Checked against linux-next next-20261021 (commit 9a482184d910): the
> affected code is unchanged there as well, so the bug is still present in
> linux-next too.
>
> Reproducer status
> -----------------
> A reproducer was written and executed on an arm64 QEMU guest (KCSAN); it
> reported the race. The reproducer source is not posted here per
> Documentation/process/security-bugs.rst; it is available on request.
>
> Triggering conditions
> ---------------------
> CAP_BPF / the ability to update sockmap and attach verdict programs.
>
> Notes
> -----
> Built and checkpatch-clean: this patch was applied on top of 7.3-rc4
> (commit 93f51579e7df) and a full arm64 kernel (defconfig) was built with
> it -- no new warnings or errors. scripts/checkpatch.pl reports 0 errors,
> 0 warnings.
>
> net/core/skmsg.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index df385a5a961e..0bfb8c4e7f36 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -1260,10 +1260,10 @@ static void sk_psock_verdict_data_ready(struct sock *sk)
>
> void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock)
> {
> - if (psock->saved_data_ready)
> + if (READ_ONCE(psock->saved_data_ready))
> return;
Okay, but if we're going down that route, now sk_psock_stop_strp() is
missing a READ_ONCE around psock->saved_data_ready, or rather we should
do it once and use it through the whole function:
void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
{
psock_set_prog(&psock->progs.stream_parser, NULL);
if (!psock->saved_data_ready)
return;
WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready);
WRITE_ONCE(psock->saved_data_ready, NULL);
strp_stop(&psock->strp);
}
>
> - psock->saved_data_ready = sk->sk_data_ready;
> + WRITE_ONCE(psock->saved_data_ready, sk->sk_data_ready);
I belive you also need to READ_ONCE(sk->sk_data_ready).
> WRITE_ONCE(sk->sk_data_ready, sk_psock_verdict_data_ready);
> WRITE_ONCE(sk->sk_write_space, sk_psock_write_space);
> }
> @@ -1273,9 +1273,9 @@ void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock)
> psock_set_prog(&psock->progs.stream_verdict, NULL);
> psock_set_prog(&psock->progs.skb_verdict, NULL);
>
> - if (!psock->saved_data_ready)
> + if (!READ_ONCE(psock->saved_data_ready))
> return;
>
> WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready);
> - psock->saved_data_ready = NULL;
> + WRITE_ONCE(psock->saved_data_ready, NULL);
> }
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path
2026-10-21 15:56 [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path Dudu Lu
` (3 preceding siblings ...)
2026-10-21 20:30 ` Jakub Sitnicki
@ 2026-10-22 3:37 ` Jiayuan Chen
2026-10-22 5:48 ` [PATCH bpf v2] bpf, sockmap: annotate the psock teardown data races Dudu Lu
5 siblings, 0 replies; 10+ messages in thread
From: Jiayuan Chen @ 2026-10-22 3:37 UTC (permalink / raw)
To: bpf
On 10/21/26 11:56 PM, Dudu Lu wrote:
> sk_psock_start_verdict() and sk_psock_stop_verdict() access
> psock->saved_data_ready with plain loads and stores, while the sibling
> sk_psock_stop_strp() uses WRITE_ONCE for the same field. sock_map_link()
> / sock_map_unref() touch the same psock from another CPU without the
> lock, so KCSAN reports the race:
>
> BUG: KCSAN: data-race in sock_map_link / sock_map_unref
> BUG: KCSAN: data-race in sk_psock_start_verdict / sock_map_unref
>
> Use READ_ONCE() for the reads and WRITE_ONCE() for the writes, matching
> the strp sibling.
>
> Fixes: 2ef2b20cf4e0 ("net: annotate data-races around sk->sk_{data_ready,write_space}")
I think it's just an incomplete fix which you shouldn't blame.
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH bpf v2] bpf, sockmap: annotate the psock teardown data races
2026-10-21 15:56 [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path Dudu Lu
` (4 preceding siblings ...)
2026-10-22 3:37 ` Jiayuan Chen
@ 2026-10-22 5:48 ` Dudu Lu
2026-10-22 15:40 ` Alexei Starovoitov
5 siblings, 1 reply; 10+ messages in thread
From: Dudu Lu @ 2026-10-22 5:48 UTC (permalink / raw)
To: john.fastabend, jakub, jiayuan.chen; +Cc: netdev, bpf
The sockmap link/unlink paths touch three psock fields from one CPU while
another CPU touches them under a different lock. A KCSAN run with two
SOCKMAPs sharing one socket reports all three:
BUG: KCSAN: data-race in sock_map_link / sock_map_unref
BUG: KCSAN: data-race in sock_map_link / unix_dgram_bpf_update_proto
BUG: KCSAN: data-race in sk_psock_start_verdict / sock_map_unref
- psock->psock_update_sk_prot: written in sock_map_init_proto() without
sk->sk_callback_lock, read in sock_map_del_link() under it.
- psock->saved_data_ready: written by sk_psock_start_verdict() under
sk->sk_callback_lock, read in sock_map_del_link() under
psock->link_lock -- a different lock instance.
- sk->sk_prot: replaced by sock_replace_proto() (via the protocol's
psock_update_sk_prot()) under sk->sk_callback_lock, read by
sock_map_link() / sock_map_sk_is_suitable() outside it.
Annotate every one of these accesses with READ_ONCE()/WRITE_ONCE(),
matching the existing convention for the same fields: sk->sk_data_ready
and sk->sk_write_space were already annotated this way, as were the
sk_psock_stop_strp() accesses to saved_data_ready. The indirect calls
read the pointer once into a local, so the guard and the call cannot see
different values.
No Fixes: tag: the three fields were introduced by three different
commits, so no single one is the origin. psock->saved_data_ready was
folded into struct sk_psock by commit 5a685cd94b21 ("skmsg: Get rid of
struct sk_psock_parser"); the psock->psock_update_sk_prot read in
sock_map_del_link() came with commit c0d95d3380ee ("bpf, sockmap:
Re-evaluate proto ops when psock is removed from sockmap"); the lockless
sk->sk_prot reads are older still.
Assisted-by: LLM
Signed-off-by: Dudu Lu <phx0fer@gmail.com>
---
v2:
- Drop the Fixes: tag; the three fields came from three different
commits, so explain the provenance in the message instead (bpf-ci).
- Read the function pointer once into a local before the guard and the
call, so the two cannot see different values: psock->saved_data_ready
in sk_psock_stop_strp() and sk_psock_stop_verdict(), and
psock->psock_update_sk_prot in sk_psock_restore_proto(),
sock_map_del_link() and sock_map_init_proto() (Jakub).
- READ_ONCE() the source sk->sk_data_ready in sk_psock_start_strp()
and sk_psock_start_verdict() (Jakub).
- Annotate the sock_map.c accesses that KCSAN actually reports:
sock_map_init_proto(), sock_map_del_link(), sock_map_link(),
sock_map_psock_get_checked() and sock_map_sk_is_suitable()
(Alexei).
Tested base
-----------
Detected on Linux 7.3-rc4 (commit 93f51579e7df), arm64 QEMU guest.
Re-checked against mainline f47d223627f8 (v7.3-341): the affected code
is unchanged between the two, so the bug is still present there.
Checked against linux-next next-20261021 (commit 9a482184d910): the
affected code is unchanged there as well, so the bug is still present in
linux-next too.
Reproducer status
-----------------
A reproducer was written and executed on an arm64 QEMU guest (KCSAN); it
reported the races above. The reproducer source is not posted here per
Documentation/process/security-bugs.rst; it is available on request.
Post-fix verification
---------------------
Tested: the patch was built for arm64 and booted in a QEMU guest with
KCSAN; the reproducer passes. Concretely, the reproducer was re-run on
the guest, once on the base tree and once with this patch applied. On
the base tree the sockmap-path races are five -- the three listed above,
plus
BUG: KCSAN: data-race in sock_map_update_elem_sys / unix_dgram_bpf_update_proto
BUG: KCSAN: data-race in sk_psock_stop_verdict / sock_map_unref
With this patch all five are gone and no new race is introduced. The map
operations complete identically in both runs (same op counts, no failures,
no KASAN memory errors), so the annotation does not change behaviour.
The reproducer also reports four unrelated races that this patch does not
touch and that fire the same way with or without it: three are the
socket-lock fast path (the 8-byte try_cmpxchg on sk_lock.combined against
the slock-protected sk_lock.owned write, by design), and one is a plain
sk->sk_prot read in generic release_sock(), which is a different scope
from the sockmap paths annotated here.
Triggering conditions
---------------------
CAP_BPF / the ability to update sockmap and attach verdict programs.
The two writers run on separate CPUs (the guest has 2), one cycling a
map with a verdict program, the other a bare map, on the same socket.
Notes
-----
Built and checkpatch-clean: this patch was applied on top of 7.3-rc4
(commit 93f51579e7df) and the affected objects were rebuilt for arm64
with the KCSAN config -- no new warnings or errors. scripts/checkpatch.pl
reports 0 errors, 1 warning (its 'BUG: KCSAN:' commit-message
heuristic, i.e. it is asking for a Fixes: tag, which we deliberately omit).
include/linux/skmsg.h | 7 +++++--
net/core/skmsg.c | 24 +++++++++++++++---------
net/core/sock_map.c | 34 +++++++++++++++++++++-------------
3 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index d5e35f24738d..cb17426f63be 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -488,8 +488,11 @@ static inline void sk_psock_cork_free(struct sk_psock *psock)
static inline void sk_psock_restore_proto(struct sock *sk,
struct sk_psock *psock)
{
- if (psock->psock_update_sk_prot)
- psock->psock_update_sk_prot(sk, psock, true);
+ int (*update_proto)(struct sock *sk, struct sk_psock *psock, bool restore);
+
+ update_proto = READ_ONCE(psock->psock_update_sk_prot);
+ if (update_proto)
+ update_proto(sk, psock, true);
}
static inline struct sk_psock *sk_psock_get(struct sock *sk)
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index df385a5a961e..54d832039f09 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -1167,22 +1167,25 @@ int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
{
- if (psock->saved_data_ready)
+ if (READ_ONCE(psock->saved_data_ready))
return;
- psock->saved_data_ready = sk->sk_data_ready;
+ WRITE_ONCE(psock->saved_data_ready, READ_ONCE(sk->sk_data_ready));
WRITE_ONCE(sk->sk_data_ready, sk_psock_strp_data_ready);
WRITE_ONCE(sk->sk_write_space, sk_psock_write_space);
}
void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
{
+ void (*saved_data_ready)(struct sock *sk);
+
psock_set_prog(&psock->progs.stream_parser, NULL);
- if (!psock->saved_data_ready)
+ saved_data_ready = READ_ONCE(psock->saved_data_ready);
+ if (!saved_data_ready)
return;
- WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready);
+ WRITE_ONCE(sk->sk_data_ready, saved_data_ready);
WRITE_ONCE(psock->saved_data_ready, NULL);
strp_stop(&psock->strp);
}
@@ -1260,22 +1263,25 @@ static void sk_psock_verdict_data_ready(struct sock *sk)
void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock)
{
- if (psock->saved_data_ready)
+ if (READ_ONCE(psock->saved_data_ready))
return;
- psock->saved_data_ready = sk->sk_data_ready;
+ WRITE_ONCE(psock->saved_data_ready, READ_ONCE(sk->sk_data_ready));
WRITE_ONCE(sk->sk_data_ready, sk_psock_verdict_data_ready);
WRITE_ONCE(sk->sk_write_space, sk_psock_write_space);
}
void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock)
{
+ void (*saved_data_ready)(struct sock *sk);
+
psock_set_prog(&psock->progs.stream_verdict, NULL);
psock_set_prog(&psock->progs.skb_verdict, NULL);
- if (!psock->saved_data_ready)
+ saved_data_ready = READ_ONCE(psock->saved_data_ready);
+ if (!saved_data_ready)
return;
- WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready);
- psock->saved_data_ready = NULL;
+ WRITE_ONCE(sk->sk_data_ready, saved_data_ready);
+ WRITE_ONCE(psock->saved_data_ready, NULL);
}
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 38df84284328..4dc69316ac17 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -145,6 +145,7 @@ static void sock_map_del_link(struct sock *sk,
{
bool strp_stop = false, verdict_stop = false;
struct sk_psock_link *link, *tmp;
+ int (*update_proto)(struct sock *sk, struct sk_psock *psock, bool restore);
spin_lock_bh(&psock->link_lock);
list_for_each_entry_safe(link, tmp, &psock->link, list) {
@@ -152,11 +153,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 (READ_ONCE(psock->saved_data_ready) && progs->stream_parser)
strp_stop = true;
- if (psock->saved_data_ready && progs->stream_verdict)
+ if (READ_ONCE(psock->saved_data_ready) && progs->stream_verdict)
verdict_stop = true;
- if (psock->saved_data_ready && progs->skb_verdict)
+ if (READ_ONCE(psock->saved_data_ready) && progs->skb_verdict)
verdict_stop = true;
list_del(&link->list);
sk_psock_free_link(link);
@@ -171,8 +172,9 @@ static void sock_map_del_link(struct sock *sk,
if (verdict_stop)
sk_psock_stop_verdict(sk, psock);
- if (psock->psock_update_sk_prot)
- psock->psock_update_sk_prot(sk, psock, false);
+ update_proto = READ_ONCE(psock->psock_update_sk_prot);
+ if (update_proto)
+ update_proto(sk, psock, false);
write_unlock_bh(&sk->sk_callback_lock);
}
}
@@ -189,10 +191,13 @@ static void sock_map_unref(struct sock *sk, void *link_raw)
static int sock_map_init_proto(struct sock *sk, struct sk_psock *psock)
{
- if (!sk->sk_prot->psock_update_sk_prot)
+ int (*update_proto)(struct sock *sk, struct sk_psock *psock, bool restore);
+
+ update_proto = READ_ONCE(sk->sk_prot)->psock_update_sk_prot;
+ if (!update_proto)
return -EINVAL;
- psock->psock_update_sk_prot = sk->sk_prot->psock_update_sk_prot;
- return sk->sk_prot->psock_update_sk_prot(sk, psock, false);
+ WRITE_ONCE(psock->psock_update_sk_prot, update_proto);
+ return update_proto(sk, psock, false);
}
static struct sk_psock *sock_map_psock_get_checked(struct sock *sk)
@@ -202,7 +207,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;
}
@@ -303,7 +308,8 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
}
write_lock_bh(&sk->sk_callback_lock);
- if (stream_parser && stream_verdict && !psock->saved_data_ready) {
+ if (stream_parser && stream_verdict &&
+ !READ_ONCE(psock->saved_data_ready)) {
if (sk_is_tcp(sk))
ret = sk_psock_init_strp(sk, psock);
else
@@ -314,9 +320,11 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
goto out;
}
sk_psock_start_strp(sk, psock);
- } else if (!stream_parser && stream_verdict && !psock->saved_data_ready) {
+ } else if (!stream_parser && stream_verdict &&
+ !READ_ONCE(psock->saved_data_ready)) {
sk_psock_start_verdict(sk,psock);
- } else if (!stream_verdict && skb_verdict && !psock->saved_data_ready) {
+ } else if (!stream_verdict && skb_verdict &&
+ !READ_ONCE(psock->saved_data_ready)) {
sk_psock_start_verdict(sk, psock);
}
write_unlock_bh(&sk->sk_callback_lock);
@@ -536,7 +544,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)
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH bpf v2] bpf, sockmap: annotate the psock teardown data races
2026-10-22 5:48 ` [PATCH bpf v2] bpf, sockmap: annotate the psock teardown data races Dudu Lu
@ 2026-10-22 15:40 ` Alexei Starovoitov
2026-10-23 18:49 ` Jakub Sitnicki
0 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2026-10-22 15:40 UTC (permalink / raw)
To: Dudu Lu, john.fastabend, jakub, jiayuan.chen; +Cc: netdev, bpf
On Wed, Oct 21, 2026 at 10:48 PM Dudu Lu <phx0fer@gmail.com> wrote:
> Annotate every one of these accesses with READ_ONCE()/WRITE_ONCE(),
> matching the existing convention for the same fields: sk->sk_data_ready
This makes KCSAN quiet and leaves the race where it is.
sock_map_link() runs under the sock lock. sock_map_delete_elem() doesn't
take it. sk_callback_lock is the only lock they share and sock_map_link()
takes it after psock_set_prog() and sock_map_init_proto().
So two tcp_bpf_update_proto(sk, psock, false) can run in parallel:
cpu0: sock_map_link() for a map with msg_parser sees
psock->progs.stream_verdict and picks TCP_BPF_TXRX
cpu1: sock_map_del_link() stops the verdict and stores TCP_BPF_TX
cpu0: stores TCP_BPF_TXRX
The socket is left with tcp_bpf_recvmsg_parser() and no verdict prog.
That's what commit c0d95d3380ee fixed.
Take the lock earlier. Then psock_update_sk_prot needs no annotation.
All accesses to saved_data_ready are under sk_callback_lock except the
three reads in sock_map_del_link(). Move that check under the lock instead
of annotating it. No need to touch skmsg.c and skmsg.h.
Only the two lockless reads of sk->sk_prot need READ_ONCE().
Something like this (compile tested only):
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 38df842..20fb605 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)
Pls run your reproducer on it, post the reproducer with the respin
and target bpf-next.
pw-bot: cr
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH bpf v2] bpf, sockmap: annotate the psock teardown data races
2026-10-22 15:40 ` Alexei Starovoitov
@ 2026-10-23 18:49 ` Jakub Sitnicki
2026-10-23 18:53 ` Alexei Starovoitov
0 siblings, 1 reply; 10+ messages in thread
From: Jakub Sitnicki @ 2026-10-23 18:49 UTC (permalink / raw)
To: Alexei Starovoitov, Dudu Lu; +Cc: john.fastabend, jiayuan.chen, netdev, bpf
On Thu, Oct 22, 2026 at 03:40 PM GMT, Alexei Starovoitov wrote:
> On Wed, Oct 21, 2026 at 10:48 PM Dudu Lu <phx0fer@gmail.com> wrote:
>> Annotate every one of these accesses with READ_ONCE()/WRITE_ONCE(),
>> matching the existing convention for the same fields: sk->sk_data_ready
>
> This makes KCSAN quiet and leaves the race where it is.
>
> sock_map_link() runs under the sock lock. sock_map_delete_elem() doesn't
> take it. sk_callback_lock is the only lock they share and sock_map_link()
> takes it after psock_set_prog() and sock_map_init_proto().
> So two tcp_bpf_update_proto(sk, psock, false) can run in parallel:
> cpu0: sock_map_link() for a map with msg_parser sees
> psock->progs.stream_verdict and picks TCP_BPF_TXRX
> cpu1: sock_map_del_link() stops the verdict and stores TCP_BPF_TX
> cpu0: stores TCP_BPF_TXRX
Right, so:
- map_update_elem syscall vs close(fd) - OK (lock_sock protected),
- map_update_elem syscall vs map_delete_elem syscall - RACY,
- map_update_elem syscall vs TCP RST (destroy) - RACY,
- map_update_elem from BPF vs close(fd) - RACY.
> The socket is left with tcp_bpf_recvmsg_parser() and no verdict prog.
> That's what commit c0d95d3380ee fixed.
> Take the lock earlier. Then psock_update_sk_prot needs no annotation.
>
> All accesses to saved_data_ready are under sk_callback_lock except the
> three reads in sock_map_del_link(). Move that check under the lock instead
> of annotating it. No need to touch skmsg.c and skmsg.h.
>
> Only the two lockless reads of sk->sk_prot need READ_ONCE().
>
> Something like this (compile tested only):
>
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 38df842..20fb605 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)
Code LGTM
^ permalink raw reply [flat|nested] 10+ messages in thread