bpf, skmsg: fix NULL pointer dereference in sk_psock_skb_ingress_enqueue
Subsystems (4)
PATCH 1/1 bpf, skmsg: fix NULL pointer dereference in sk_psock_skb_ingress_enqueue
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in sk_psock_skb_ingress_enqueue() which syzbot reported [1].
[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueue
write to 0xffff88814b3278b8 of 8 bytes by task 10724 on cpu 1:
sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75
read to 0xffff88814b3278b8 of 8 bytes by task 10713 on cpu 0:
sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75
value changed: 0xffffffff83d7feb0 -> 0x0000000000000000
Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024
Prior to this, commit 4cd12c6065df ("bpf, sockmap: Fix NULL pointer dereference in sk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection of saved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it with sk_callback_lock read lock because the writer side in the sk_psock_drop() uses "write_lock_bh(&sk->sk_callback_lock);".
--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)
net/core/skmsg.c +2 -0
@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; }
Signature · 1 line
2.37.3
Quoted 52 lines · Jason Xing
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in
sk_psock_skb_ingress_enqueue()which syzbot reported [1].[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueuewrite to
0xffff88814b3278b8of 8 bytes by task 10724 on cpu 1:sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75read to
0xffff88814b3278b8of 8 bytes by task 10713 on cpu 0:sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75value changed:
0xffffffff83d7feb0->0x0000000000000000Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024
Quoted 6 lines
Prior to this, commit
4cd12c6065df("bpf, sockmap: Fix NULL pointer dereference insk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection ofsaved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it withsk_callback_lockread lock because the writer side in thesk_psock_drop()uses "write_lock_bh(&sk->sk_callback_lock);".
Should I use 'read_lock(&sk->sk_callback_lock);' in bpf_tcp_ingress() to protect sk_callback_lock field? If it's ok, I will do it in another patch.
Thanks,
Jason
Quoted 26 lines
Fixes 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")Reported-by [email protected]Signed-off-by Jason Xing <[email protected]>--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)net/core/skmsg.c +2
-1@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; } --2.37.3
Quoted 52 lines · Jason Xing
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in
sk_psock_skb_ingress_enqueue()which syzbot reported [1].[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueuewrite to
0xffff88814b3278b8of 8 bytes by task 10724 on cpu 1:sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75read to
0xffff88814b3278b8of 8 bytes by task 10713 on cpu 0:sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75value changed:
0xffffffff83d7feb0->0x0000000000000000Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024
Quoted 6 lines
Prior to this, commit
4cd12c6065df("bpf, sockmap: Fix NULL pointer dereference insk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection ofsaved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it withsk_callback_lockread lock because the writer side in thesk_psock_drop()uses "write_lock_bh(&sk->sk_callback_lock);".
Hello experts, any comments are welcome:) Did I miss something?
Thanks,
Jason
Quoted 26 lines
Fixes 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")Reported-by [email protected]Signed-off-by Jason Xing <[email protected]>--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)net/core/skmsg.c +2
-1@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; } --2.37.3
John Fastabend
maintainerQuoted 71 lines · Jason Xing
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in
sk_psock_skb_ingress_enqueue()which syzbot reported [1].[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueuewrite to
0xffff88814b3278b8of 8 bytes by task 10724 on cpu 1:sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75read to
0xffff88814b3278b8of 8 bytes by task 10713 on cpu 0:sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75value changed:
0xffffffff83d7feb0->0x0000000000000000Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024Prior to this, commit
4cd12c6065df("bpf, sockmap: Fix NULL pointer dereference insk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection ofsaved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it withsk_callback_lockread lock because the writer side in thesk_psock_drop()uses "write_lock_bh(&sk->sk_callback_lock);".Fixes 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")Reported-by [email protected]Signed-off-by Jason Xing <[email protected]>--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)net/core/skmsg.c +0
-0
net/core/skmsg.c static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, · line 559
Quoted 8 lines
net/core/skmsg.c +2
-0@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; }
The problem is the check and then usage presumably it is already set to NULL:
static inline void sk_psock_data_ready(struct sock *sk, struct sk_psock *psock)
{
if (psock->saved_data_ready)
psock->saved_data_ready(sk);I'm thinking we might be able to get away with just a READ_ONCE here with similar WRITE_ONCE on other side. Something like this,
sk_psock_data_ready(struct sock *sk, struct sk_psock *psock)
{
saved_data_ready = READ_ONCE(psock->saved_data_ready) if (saved_data_ready)
saved_data_ready(sk)
....And then in sk_psock_stop_verdict,
WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready); WRITE_ONCE(psock->saved_data_ready, NULL);
And because we don't actually release the sock until a RCU grace period we should be OK. The TCP stack manages to work correctly without wrapping tcp_data_ready in locks like this. But nice thing there is you don't change this callback on live sockets.
I think at least to keep backport simply above patch is ok, but lets move the read_lock_bh()/unlock_bh() into the sk_psock_data_ready() call and then we don't duplicate this error again. Does that make sense?
Thanks,
John
Hello John,
Quoted 85 lines · John Fastabend
Quoted 71 lines · Jason Xing
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in
sk_psock_skb_ingress_enqueue()which syzbot reported [1].[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueuewrite to
0xffff88814b3278b8of 8 bytes by task 10724 on cpu 1:sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75read to
0xffff88814b3278b8of 8 bytes by task 10713 on cpu 0:sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75value changed:
0xffffffff83d7feb0->0x0000000000000000Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024Prior to this, commit
4cd12c6065df("bpf, sockmap: Fix NULL pointer dereference insk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection ofsaved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it withsk_callback_lockread lock because the writer side in thesk_psock_drop()uses "write_lock_bh(&sk->sk_callback_lock);".Fixes 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")Reported-by [email protected]Signed-off-by Jason Xing <[email protected]>--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)net/core/skmsg.c +0
-0net/core/skmsg.c static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, · line 559
Quoted 8 lines
net/core/skmsg.c +2
-0@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; }The problem is the check and then usage presumably it is already set to NULL:
Quoted 4 lines
static inline void
sk_psock_data_ready(struct sock *sk, struct sk_psock *psock){ if (psock->saved_data_ready) psock->saved_data_ready(sk);
Yes.
Quoted 4 lines
I'm thinking we might be able to get away with just a
READ_ONCEhere with similarWRITE_ONCEon other side. Something like this,
The simple fix that popped into my mind at the beginning is the same as you: adding the READ_ONCE/WRITE_ONCE pair.
Quoted 14 lines
sk_psock_data_ready(struct sock *sk, struct sk_psock *psock) { saved_data_ready = READ_ONCE(psock->saved_data_ready)if (saved_data_ready) saved_data_ready(sk) ....And then in
sk_psock_stop_verdict,WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready); WRITE_ONCE(psock->saved_data_ready, NULL);
Quoted 8 lines
And because we don't actually release the sock until a RCU grace period we should be OK. The TCP stack manages to work correctly without wrapping
tcp_data_readyin locks like this. But nice thing there is you don't change this callback on live sockets.I think at least to keep backport simply above patch is ok, but lets move the read_lock_bh()/unlock_bh() into the
sk_psock_data_ready()call and then we don't duplicate this error again. Does that make sense?
Thanks for your quick response and such detailed analysis:)
It makes sense. I will move it into the sk_psock_data_ready(), then three callers for now and possible callers in future would never go wrong.
Quoted 4 lines
Thanks,
John
John Fastabend
maintainerQuoted 97 lines · Jason Xing
Hello John,
Quoted 85 lines · John Fastabend
Quoted 71 lines · Jason Xing
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in
sk_psock_skb_ingress_enqueue()which syzbot reported [1].[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueuewrite to
0xffff88814b3278b8of 8 bytes by task 10724 on cpu 1:sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75read to
0xffff88814b3278b8of 8 bytes by task 10713 on cpu 0:sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75value changed:
0xffffffff83d7feb0->0x0000000000000000Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024Prior to this, commit
4cd12c6065df("bpf, sockmap: Fix NULL pointer dereference insk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection ofsaved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it withsk_callback_lockread lock because the writer side in thesk_psock_drop()uses "write_lock_bh(&sk->sk_callback_lock);".Fixes 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")Reported-by [email protected]Signed-off-by Jason Xing <[email protected]>--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)net/core/skmsg.c +0
-0net/core/skmsg.c static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, · line 559
Quoted 8 lines
net/core/skmsg.c +2
-0@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; }The problem is the check and then usage presumably it is already set to NULL:
Quoted 4 lines
static inline void
sk_psock_data_ready(struct sock *sk, struct sk_psock *psock){ if (psock->saved_data_ready) psock->saved_data_ready(sk);Yes.
Quoted 2 lines
Quoted 5 lines
Quoted 2 lines
I'm thinking we might be able to get away with just a
READ_ONCEhere with similarWRITE_ONCEon other side. Something like this,The simple fix that popped into my mind at the beginning is the same as you: adding the READ_ONCE/WRITE_ONCE pair.
Let me know if you want to try doing a patch with the READ_ONCE/WRITE_ONCE we could push something like that through bpf-next I think. Just needs some extra thought and testing.
Quoted 102 lines · John Fastabend
Quoted 100 lines · Jason Xing
Hello John,
Quoted 85 lines · John Fastabend
Quoted 71 lines · Jason Xing
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in
sk_psock_skb_ingress_enqueue()which syzbot reported [1].[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueuewrite to
0xffff88814b3278b8of 8 bytes by task 10724 on cpu 1:sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75read to
0xffff88814b3278b8of 8 bytes by task 10713 on cpu 0:sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75value changed:
0xffffffff83d7feb0->0x0000000000000000Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024Prior to this, commit
4cd12c6065df("bpf, sockmap: Fix NULL pointer dereference insk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection ofsaved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it withsk_callback_lockread lock because the writer side in thesk_psock_drop()uses "write_lock_bh(&sk->sk_callback_lock);".Fixes 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")Reported-by [email protected]Signed-off-by Jason Xing <[email protected]>--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)net/core/skmsg.c +0
-0net/core/skmsg.c static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, · line 559
Quoted 8 lines
net/core/skmsg.c +2
-0@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; }The problem is the check and then usage presumably it is already set to NULL:
Quoted 4 lines
static inline void
sk_psock_data_ready(struct sock *sk, struct sk_psock *psock){ if (psock->saved_data_ready) psock->saved_data_ready(sk);Yes.
Quoted 4 lines
I'm thinking we might be able to get away with just a
READ_ONCEhere with similarWRITE_ONCEon other side. Something like this,
Quoted 6 lines
Quoted 2 lines
The simple fix that popped into my mind at the beginning is the same as you: adding the READ_ONCE/WRITE_ONCE pair.
Let me know if you want to try doing a patch with the READ_ONCE/WRITE_ONCE we could push something like that through bpf-next I think. Just needs some extra thought and testing.
Yes, I'm interested in it. Just a little bit worried that I cannot do it well. I will take some time to dig into it.
BTW, would this modification conflict with the current patch? The final solution you're thinking of is using the READ_ONCE/WRITE_ONCE pair?
Thanks,
Jason
John Fastabend
maintainerQuoted 110 lines · Jason Xing
Quoted 102 lines · John Fastabend
Quoted 100 lines · Jason Xing
Hello John,
Quoted 85 lines · John Fastabend
Quoted 71 lines · Jason Xing
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in
sk_psock_skb_ingress_enqueue()which syzbot reported [1].[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueuewrite to
0xffff88814b3278b8of 8 bytes by task 10724 on cpu 1:sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75read to
0xffff88814b3278b8of 8 bytes by task 10713 on cpu 0:sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75value changed:
0xffffffff83d7feb0->0x0000000000000000Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024Prior to this, commit
4cd12c6065df("bpf, sockmap: Fix NULL pointer dereference insk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection ofsaved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it withsk_callback_lockread lock because the writer side in thesk_psock_drop()uses "write_lock_bh(&sk->sk_callback_lock);".Fixes 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")Reported-by [email protected]Signed-off-by Jason Xing <[email protected]>--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)net/core/skmsg.c +0
-0net/core/skmsg.c static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, · line 559
Quoted 8 lines
net/core/skmsg.c +2
-0@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; }The problem is the check and then usage presumably it is already set to NULL:
Quoted 4 lines
static inline void
sk_psock_data_ready(struct sock *sk, struct sk_psock *psock){ if (psock->saved_data_ready) psock->saved_data_ready(sk);Yes.
Quoted 4 lines
I'm thinking we might be able to get away with just a
READ_ONCEhere with similarWRITE_ONCEon other side. Something like this,Quoted 6 lines
Quoted 2 lines
The simple fix that popped into my mind at the beginning is the same as you: adding the READ_ONCE/WRITE_ONCE pair.
Let me know if you want to try doing a patch with the READ_ONCE/WRITE_ONCE we could push something like that through bpf-next I think. Just needs some extra thought and testing.
Quoted 6 lines
Yes, I'm interested in it. Just a little bit worried that I cannot do it well. I will take some time to dig into it.
BTW, would this modification conflict with the current patch? The final solution you're thinking of is using the READ_ONCE/WRITE_ONCE pair?
Idea would be you can drop the read_lock/unlock once READ_ONCE/WRITE_ONCE and such are in place.
Quoted 3 lines
Thanks,
Jason
Quoted 115 lines · John Fastabend
Quoted 113 lines · Jason Xing
Quoted 102 lines · John Fastabend
Quoted 100 lines · Jason Xing
Hello John,
Quoted 85 lines · John Fastabend
Quoted 71 lines · Jason Xing
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in
sk_psock_skb_ingress_enqueue()which syzbot reported [1].[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueuewrite to
0xffff88814b3278b8of 8 bytes by task 10724 on cpu 1:sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75read to
0xffff88814b3278b8of 8 bytes by task 10713 on cpu 0:sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75value changed:
0xffffffff83d7feb0->0x0000000000000000Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024Prior to this, commit
4cd12c6065df("bpf, sockmap: Fix NULL pointer dereference insk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection ofsaved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it withsk_callback_lockread lock because the writer side in thesk_psock_drop()uses "write_lock_bh(&sk->sk_callback_lock);".Fixes 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")Reported-by [email protected]Signed-off-by Jason Xing <[email protected]>--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)net/core/skmsg.c +0
-0net/core/skmsg.c static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, · line 559
Quoted 8 lines
net/core/skmsg.c +2
-0@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; }The problem is the check and then usage presumably it is already set to NULL:
Quoted 4 lines
static inline void
sk_psock_data_ready(struct sock *sk, struct sk_psock *psock){ if (psock->saved_data_ready) psock->saved_data_ready(sk);Yes.
Quoted 4 lines
I'm thinking we might be able to get away with just a
READ_ONCEhere with similarWRITE_ONCEon other side. Something like this,Quoted 6 lines
Quoted 2 lines
The simple fix that popped into my mind at the beginning is the same as you: adding the READ_ONCE/WRITE_ONCE pair.
Let me know if you want to try doing a patch with the READ_ONCE/WRITE_ONCE we could push something like that through bpf-next I think. Just needs some extra thought and testing.
Yes, I'm interested in it. Just a little bit worried that I cannot do it well. I will take some time to dig into it.
Quoted 6 lines
Quoted 3 lines
BTW, would this modification conflict with the current patch? The final solution you're thinking of is using the READ_ONCE/WRITE_ONCE pair?
Idea would be you can drop the read_lock/unlock once READ_ONCE/WRITE_ONCE and such are in place.
Got it. Allow me to work on it. Lockless protection is surely better than rw lock. But can we let the current patch go into the tree first?
Thanks,
Jason
Quoted 6 lines
Quoted 4 lines
Thanks,
Jason
John Fastabend
maintainerQuoted 120 lines · Jason Xing
Quoted 119 lines · John Fastabend
Quoted 116 lines · Jason Xing
Quoted 102 lines · John Fastabend
Quoted 100 lines · Jason Xing
Hello John,
Quoted 85 lines · John Fastabend
Quoted 71 lines · Jason Xing
From: Jason Xing <[email protected]>
Fix NULL pointer data-races in
sk_psock_skb_ingress_enqueue()which syzbot reported [1].[1]
BUG: KCSAN: data-race in sk_psock_drop / sk_psock_skb_ingress_enqueuewrite to
0xffff88814b3278b8of 8 bytes by task 10724 on cpu 1:sk_psock_stop_verdict net/core/skmsg.c:1257 [inline] sk_psock_drop+0x13e/0x1f0 net/core/skmsg.c:843 sk_psock_put include/linux/skmsg.h:459 [inline] sock_map_close+0x1a7/0x260 net/core/sock_map.c:1648 unix_release+0x4b/0x80 net/unix/af_unix.c:1048 __sock_release net/socket.c:659 [inline] sock_close+0x68/0x150 net/socket.c:1421 __fput+0x2c1/0x660 fs/file_table.c:422 __fput_sync+0x44/0x60 fs/file_table.c:507 __do_sys_close fs/open.c:1556 [inline] __se_sys_close+0x101/0x1b0 fs/open.c:1541 __x64_sys_close+0x1f/0x30 fs/open.c:1541 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75read to
0xffff88814b3278b8of 8 bytes by task 10713 on cpu 0:sk_psock_data_ready include/linux/skmsg.h:464 [inline] sk_psock_skb_ingress_enqueue+0x32d/0x390 net/core/skmsg.c:555 sk_psock_skb_ingress_self+0x185/0x1e0 net/core/skmsg.c:606 sk_psock_verdict_apply net/core/skmsg.c:1008 [inline] sk_psock_verdict_recv+0x3e4/0x4a0 net/core/skmsg.c:1202 unix_read_skb net/unix/af_unix.c:2546 [inline] unix_stream_read_skb+0x9e/0xf0 net/unix/af_unix.c:2682 sk_psock_verdict_data_ready+0x77/0x220 net/core/skmsg.c:1223 unix_stream_sendmsg+0x527/0x860 net/unix/af_unix.c:2339 sock_sendmsg_nosec net/socket.c:730 [inline] __sock_sendmsg+0x140/0x180 net/socket.c:745 ____sys_sendmsg+0x312/0x410 net/socket.c:2584 ___sys_sendmsg net/socket.c:2638 [inline] __sys_sendmsg+0x1e9/0x280 net/socket.c:2667 __do_sys_sendmsg net/socket.c:2676 [inline] __se_sys_sendmsg net/socket.c:2674 [inline] __x64_sys_sendmsg+0x46/0x50 net/socket.c:2674 do_syscall_64+0xd3/0x1d0 entry_SYSCALL_64_after_hwframe+0x6d/0x75value changed:
0xffffffff83d7feb0->0x0000000000000000Reported by Kernel Concurrency Sanitizer on:
CPU: 0 PID: 10713 Comm: syz-executor.4 Tainted: G W 6.8.0-syzkaller-08951-gfe46a7dd189e #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/29/2024Prior to this, commit
4cd12c6065df("bpf, sockmap: Fix NULL pointer dereference insk_psock_verdict_data_ready()") fixed one NULL pointer similarly due to no protection ofsaved_data_ready. Here is another different caller causing the same issue because of the same reason. So we should protect it withsk_callback_lockread lock because the writer side in thesk_psock_drop()uses "write_lock_bh(&sk->sk_callback_lock);".Fixes 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")Reported-by [email protected]Signed-off-by Jason Xing <[email protected]>--- net/core/skmsg.c | 2 ++ 1 file changed, 2 insertions(+)net/core/skmsg.c +0
-0net/core/skmsg.c static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, · line 559
Quoted 8 lines
net/core/skmsg.c +2
-0@@ -552,7 +552,9 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff *skb, msg->skb = skb;sk_psock_queue_msg (psock, msg); +read_lock_bh (&sk->sk_callback_lock);sk_psock_data_ready (sk, psock); +read_unlock_bh (&sk->sk_callback_lock);return copied; }The problem is the check and then usage presumably it is already set to NULL:
Quoted 4 lines
static inline void
sk_psock_data_ready(struct sock *sk, struct sk_psock *psock){ if (psock->saved_data_ready) psock->saved_data_ready(sk);Yes.
Quoted 4 lines
I'm thinking we might be able to get away with just a
READ_ONCEhere with similarWRITE_ONCEon other side. Something like this,Quoted 6 lines
Quoted 2 lines
The simple fix that popped into my mind at the beginning is the same as you: adding the READ_ONCE/WRITE_ONCE pair.
Let me know if you want to try doing a patch with the READ_ONCE/WRITE_ONCE we could push something like that through bpf-next I think. Just needs some extra thought and testing.
Yes, I'm interested in it. Just a little bit worried that I cannot do it well. I will take some time to dig into it.
BTW, would this modification conflict with the current patch? The final solution you're thinking of is using the READ_ONCE/WRITE_ONCE pair?
Quoted 5 lines
Quoted 2 lines
Idea would be you can drop the read_lock/unlock once READ_ONCE/WRITE_ONCE and such are in place.
Got it. Allow me to work on it. Lockless protection is surely better than rw lock. But can we let the current patch go into the tree first?
Yes v2 fix has my Reviewed-by and should go in as a fix. The above improvement can go through bpf-next when its ready.