* [PATCH net] veth: fix races around rq->rx_notify_masked
@ 2022-02-08 23:28 Eric Dumazet
2022-02-09 12:10 ` patchwork-bot+netdevbpf
2022-02-09 12:36 ` Toshiaki Makita
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2022-02-08 23:28 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski
Cc: netdev, Eric Dumazet, Eric Dumazet, Toshiaki Makita, syzbot
From: Eric Dumazet <edumazet@google.com>
veth being NETIF_F_LLTX enabled, we need to be more careful
whenever we read/write rq->rx_notify_masked.
BUG: KCSAN: data-race in veth_xmit / veth_xmit
write to 0xffff888133d9a9f8 of 1 bytes by task 23552 on cpu 0:
__veth_xdp_flush drivers/net/veth.c:269 [inline]
veth_xmit+0x307/0x470 drivers/net/veth.c:350
__netdev_start_xmit include/linux/netdevice.h:4683 [inline]
netdev_start_xmit include/linux/netdevice.h:4697 [inline]
xmit_one+0x105/0x2f0 net/core/dev.c:3473
dev_hard_start_xmit net/core/dev.c:3489 [inline]
__dev_queue_xmit+0x86d/0xf90 net/core/dev.c:4116
dev_queue_xmit+0x13/0x20 net/core/dev.c:4149
br_dev_queue_push_xmit+0x3ce/0x430 net/bridge/br_forward.c:53
NF_HOOK include/linux/netfilter.h:307 [inline]
br_forward_finish net/bridge/br_forward.c:66 [inline]
NF_HOOK include/linux/netfilter.h:307 [inline]
__br_forward+0x2e4/0x400 net/bridge/br_forward.c:115
br_flood+0x521/0x5c0 net/bridge/br_forward.c:242
br_dev_xmit+0x8b6/0x960
__netdev_start_xmit include/linux/netdevice.h:4683 [inline]
netdev_start_xmit include/linux/netdevice.h:4697 [inline]
xmit_one+0x105/0x2f0 net/core/dev.c:3473
dev_hard_start_xmit net/core/dev.c:3489 [inline]
__dev_queue_xmit+0x86d/0xf90 net/core/dev.c:4116
dev_queue_xmit+0x13/0x20 net/core/dev.c:4149
neigh_hh_output include/net/neighbour.h:525 [inline]
neigh_output include/net/neighbour.h:539 [inline]
ip_finish_output2+0x6f8/0xb70 net/ipv4/ip_output.c:228
ip_finish_output+0xfb/0x240 net/ipv4/ip_output.c:316
NF_HOOK_COND include/linux/netfilter.h:296 [inline]
ip_output+0xf3/0x1a0 net/ipv4/ip_output.c:430
dst_output include/net/dst.h:451 [inline]
ip_local_out net/ipv4/ip_output.c:126 [inline]
ip_send_skb+0x6e/0xe0 net/ipv4/ip_output.c:1570
udp_send_skb+0x641/0x880 net/ipv4/udp.c:967
udp_sendmsg+0x12ea/0x14c0 net/ipv4/udp.c:1254
inet_sendmsg+0x5f/0x80 net/ipv4/af_inet.c:819
sock_sendmsg_nosec net/socket.c:705 [inline]
sock_sendmsg net/socket.c:725 [inline]
____sys_sendmsg+0x39a/0x510 net/socket.c:2413
___sys_sendmsg net/socket.c:2467 [inline]
__sys_sendmmsg+0x267/0x4c0 net/socket.c:2553
__do_sys_sendmmsg net/socket.c:2582 [inline]
__se_sys_sendmmsg net/socket.c:2579 [inline]
__x64_sys_sendmmsg+0x53/0x60 net/socket.c:2579
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
read to 0xffff888133d9a9f8 of 1 bytes by task 23563 on cpu 1:
__veth_xdp_flush drivers/net/veth.c:268 [inline]
veth_xmit+0x2d6/0x470 drivers/net/veth.c:350
__netdev_start_xmit include/linux/netdevice.h:4683 [inline]
netdev_start_xmit include/linux/netdevice.h:4697 [inline]
xmit_one+0x105/0x2f0 net/core/dev.c:3473
dev_hard_start_xmit net/core/dev.c:3489 [inline]
__dev_queue_xmit+0x86d/0xf90 net/core/dev.c:4116
dev_queue_xmit+0x13/0x20 net/core/dev.c:4149
br_dev_queue_push_xmit+0x3ce/0x430 net/bridge/br_forward.c:53
NF_HOOK include/linux/netfilter.h:307 [inline]
br_forward_finish net/bridge/br_forward.c:66 [inline]
NF_HOOK include/linux/netfilter.h:307 [inline]
__br_forward+0x2e4/0x400 net/bridge/br_forward.c:115
br_flood+0x521/0x5c0 net/bridge/br_forward.c:242
br_dev_xmit+0x8b6/0x960
__netdev_start_xmit include/linux/netdevice.h:4683 [inline]
netdev_start_xmit include/linux/netdevice.h:4697 [inline]
xmit_one+0x105/0x2f0 net/core/dev.c:3473
dev_hard_start_xmit net/core/dev.c:3489 [inline]
__dev_queue_xmit+0x86d/0xf90 net/core/dev.c:4116
dev_queue_xmit+0x13/0x20 net/core/dev.c:4149
neigh_hh_output include/net/neighbour.h:525 [inline]
neigh_output include/net/neighbour.h:539 [inline]
ip_finish_output2+0x6f8/0xb70 net/ipv4/ip_output.c:228
ip_finish_output+0xfb/0x240 net/ipv4/ip_output.c:316
NF_HOOK_COND include/linux/netfilter.h:296 [inline]
ip_output+0xf3/0x1a0 net/ipv4/ip_output.c:430
dst_output include/net/dst.h:451 [inline]
ip_local_out net/ipv4/ip_output.c:126 [inline]
ip_send_skb+0x6e/0xe0 net/ipv4/ip_output.c:1570
udp_send_skb+0x641/0x880 net/ipv4/udp.c:967
udp_sendmsg+0x12ea/0x14c0 net/ipv4/udp.c:1254
inet_sendmsg+0x5f/0x80 net/ipv4/af_inet.c:819
sock_sendmsg_nosec net/socket.c:705 [inline]
sock_sendmsg net/socket.c:725 [inline]
____sys_sendmsg+0x39a/0x510 net/socket.c:2413
___sys_sendmsg net/socket.c:2467 [inline]
__sys_sendmmsg+0x267/0x4c0 net/socket.c:2553
__do_sys_sendmmsg net/socket.c:2582 [inline]
__se_sys_sendmmsg net/socket.c:2579 [inline]
__x64_sys_sendmmsg+0x53/0x60 net/socket.c:2579
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
value changed: 0x00 -> 0x01
Reported by Kernel Concurrency Sanitizer on:
CPU: 1 PID: 23563 Comm: syz-executor.5 Not tainted 5.17.0-rc2-syzkaller-00064-gc36c04c2e132 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Fixes: 948d4f214fde ("veth: Add driver XDP")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
drivers/net/veth.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 354a963075c5f15d194152ca9b38c59c66763e94..d29fb9759cc9557d62370f016fc160f3dbd16ce3 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -265,9 +265,10 @@ static void __veth_xdp_flush(struct veth_rq *rq)
{
/* Write ptr_ring before reading rx_notify_masked */
smp_mb();
- if (!rq->rx_notify_masked) {
- rq->rx_notify_masked = true;
- napi_schedule(&rq->xdp_napi);
+ if (!READ_ONCE(rq->rx_notify_masked) &&
+ napi_schedule_prep(&rq->xdp_napi)) {
+ WRITE_ONCE(rq->rx_notify_masked, true);
+ __napi_schedule(&rq->xdp_napi);
}
}
@@ -912,8 +913,10 @@ static int veth_poll(struct napi_struct *napi, int budget)
/* Write rx_notify_masked before reading ptr_ring */
smp_store_mb(rq->rx_notify_masked, false);
if (unlikely(!__ptr_ring_empty(&rq->xdp_ring))) {
- rq->rx_notify_masked = true;
- napi_schedule(&rq->xdp_napi);
+ if (napi_schedule_prep(&rq->xdp_napi)) {
+ WRITE_ONCE(rq->rx_notify_masked, true);
+ __napi_schedule(&rq->xdp_napi);
+ }
}
}
--
2.35.0.263.gb82422642f-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] veth: fix races around rq->rx_notify_masked
2022-02-08 23:28 [PATCH net] veth: fix races around rq->rx_notify_masked Eric Dumazet
@ 2022-02-09 12:10 ` patchwork-bot+netdevbpf
2022-02-09 12:36 ` Toshiaki Makita
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-02-09 12:10 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, kuba, netdev, edumazet, makita.toshiaki, syzkaller
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
On Tue, 8 Feb 2022 15:28:22 -0800 you wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> veth being NETIF_F_LLTX enabled, we need to be more careful
> whenever we read/write rq->rx_notify_masked.
>
> BUG: KCSAN: data-race in veth_xmit / veth_xmit
>
> [...]
Here is the summary with links:
- [net] veth: fix races around rq->rx_notify_masked
https://git.kernel.org/netdev/net/c/68468d8c4cd4
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] veth: fix races around rq->rx_notify_masked
2022-02-08 23:28 [PATCH net] veth: fix races around rq->rx_notify_masked Eric Dumazet
2022-02-09 12:10 ` patchwork-bot+netdevbpf
@ 2022-02-09 12:36 ` Toshiaki Makita
2022-02-10 16:57 ` Eric Dumazet
1 sibling, 1 reply; 5+ messages in thread
From: Toshiaki Makita @ 2022-02-09 12:36 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller, Jakub Kicinski
Cc: netdev, Eric Dumazet, Toshiaki Makita, syzbot
On 2022/02/09 8:28, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
Thank you for handling this case.
> veth being NETIF_F_LLTX enabled, we need to be more careful
> whenever we read/write rq->rx_notify_masked.
>
> BUG: KCSAN: data-race in veth_xmit / veth_xmit
>
> write to 0xffff888133d9a9f8 of 1 bytes by task 23552 on cpu 0:
> __veth_xdp_flush drivers/net/veth.c:269 [inline]
> veth_xmit+0x307/0x470 drivers/net/veth.c:350
> __netdev_start_xmit include/linux/netdevice.h:4683 [inline]
> netdev_start_xmit include/linux/netdevice.h:4697 [inline]
> xmit_one+0x105/0x2f0 net/core/dev.c:3473
> dev_hard_start_xmit net/core/dev.c:3489 [inline]
> __dev_queue_xmit+0x86d/0xf90 net/core/dev.c:4116
> dev_queue_xmit+0x13/0x20 net/core/dev.c:4149
> br_dev_queue_push_xmit+0x3ce/0x430 net/bridge/br_forward.c:53
> NF_HOOK include/linux/netfilter.h:307 [inline]
> br_forward_finish net/bridge/br_forward.c:66 [inline]
> NF_HOOK include/linux/netfilter.h:307 [inline]
> __br_forward+0x2e4/0x400 net/bridge/br_forward.c:115
> br_flood+0x521/0x5c0 net/bridge/br_forward.c:242
> br_dev_xmit+0x8b6/0x960
> __netdev_start_xmit include/linux/netdevice.h:4683 [inline]
> netdev_start_xmit include/linux/netdevice.h:4697 [inline]
> xmit_one+0x105/0x2f0 net/core/dev.c:3473
> dev_hard_start_xmit net/core/dev.c:3489 [inline]
> __dev_queue_xmit+0x86d/0xf90 net/core/dev.c:4116
> dev_queue_xmit+0x13/0x20 net/core/dev.c:4149
> neigh_hh_output include/net/neighbour.h:525 [inline]
> neigh_output include/net/neighbour.h:539 [inline]
> ip_finish_output2+0x6f8/0xb70 net/ipv4/ip_output.c:228
> ip_finish_output+0xfb/0x240 net/ipv4/ip_output.c:316
> NF_HOOK_COND include/linux/netfilter.h:296 [inline]
> ip_output+0xf3/0x1a0 net/ipv4/ip_output.c:430
> dst_output include/net/dst.h:451 [inline]
> ip_local_out net/ipv4/ip_output.c:126 [inline]
> ip_send_skb+0x6e/0xe0 net/ipv4/ip_output.c:1570
> udp_send_skb+0x641/0x880 net/ipv4/udp.c:967
> udp_sendmsg+0x12ea/0x14c0 net/ipv4/udp.c:1254
> inet_sendmsg+0x5f/0x80 net/ipv4/af_inet.c:819
> sock_sendmsg_nosec net/socket.c:705 [inline]
> sock_sendmsg net/socket.c:725 [inline]
> ____sys_sendmsg+0x39a/0x510 net/socket.c:2413
> ___sys_sendmsg net/socket.c:2467 [inline]
> __sys_sendmmsg+0x267/0x4c0 net/socket.c:2553
> __do_sys_sendmmsg net/socket.c:2582 [inline]
> __se_sys_sendmmsg net/socket.c:2579 [inline]
> __x64_sys_sendmmsg+0x53/0x60 net/socket.c:2579
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> read to 0xffff888133d9a9f8 of 1 bytes by task 23563 on cpu 1:
> __veth_xdp_flush drivers/net/veth.c:268 [inline]
> veth_xmit+0x2d6/0x470 drivers/net/veth.c:350
> __netdev_start_xmit include/linux/netdevice.h:4683 [inline]
> netdev_start_xmit include/linux/netdevice.h:4697 [inline]
> xmit_one+0x105/0x2f0 net/core/dev.c:3473
> dev_hard_start_xmit net/core/dev.c:3489 [inline]
> __dev_queue_xmit+0x86d/0xf90 net/core/dev.c:4116
> dev_queue_xmit+0x13/0x20 net/core/dev.c:4149
> br_dev_queue_push_xmit+0x3ce/0x430 net/bridge/br_forward.c:53
> NF_HOOK include/linux/netfilter.h:307 [inline]
> br_forward_finish net/bridge/br_forward.c:66 [inline]
> NF_HOOK include/linux/netfilter.h:307 [inline]
> __br_forward+0x2e4/0x400 net/bridge/br_forward.c:115
> br_flood+0x521/0x5c0 net/bridge/br_forward.c:242
> br_dev_xmit+0x8b6/0x960
> __netdev_start_xmit include/linux/netdevice.h:4683 [inline]
> netdev_start_xmit include/linux/netdevice.h:4697 [inline]
> xmit_one+0x105/0x2f0 net/core/dev.c:3473
> dev_hard_start_xmit net/core/dev.c:3489 [inline]
> __dev_queue_xmit+0x86d/0xf90 net/core/dev.c:4116
> dev_queue_xmit+0x13/0x20 net/core/dev.c:4149
> neigh_hh_output include/net/neighbour.h:525 [inline]
> neigh_output include/net/neighbour.h:539 [inline]
> ip_finish_output2+0x6f8/0xb70 net/ipv4/ip_output.c:228
> ip_finish_output+0xfb/0x240 net/ipv4/ip_output.c:316
> NF_HOOK_COND include/linux/netfilter.h:296 [inline]
> ip_output+0xf3/0x1a0 net/ipv4/ip_output.c:430
> dst_output include/net/dst.h:451 [inline]
> ip_local_out net/ipv4/ip_output.c:126 [inline]
> ip_send_skb+0x6e/0xe0 net/ipv4/ip_output.c:1570
> udp_send_skb+0x641/0x880 net/ipv4/udp.c:967
> udp_sendmsg+0x12ea/0x14c0 net/ipv4/udp.c:1254
> inet_sendmsg+0x5f/0x80 net/ipv4/af_inet.c:819
> sock_sendmsg_nosec net/socket.c:705 [inline]
> sock_sendmsg net/socket.c:725 [inline]
> ____sys_sendmsg+0x39a/0x510 net/socket.c:2413
> ___sys_sendmsg net/socket.c:2467 [inline]
> __sys_sendmmsg+0x267/0x4c0 net/socket.c:2553
> __do_sys_sendmmsg net/socket.c:2582 [inline]
> __se_sys_sendmmsg net/socket.c:2579 [inline]
> __x64_sys_sendmmsg+0x53/0x60 net/socket.c:2579
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> value changed: 0x00 -> 0x01
I'm not familiar with KCSAN.
Does this mean rx_notify_masked value is changed while another CPU is reading it?
If so, I'm not sure there is a problem with that.
At least we could call napi_schedule() twice, but that just causes one extra napi
poll due to NAPIF_STATE_MISSED, and it happens very rarely?
Toshiaki Makita
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] veth: fix races around rq->rx_notify_masked
2022-02-09 12:36 ` Toshiaki Makita
@ 2022-02-10 16:57 ` Eric Dumazet
2022-02-14 15:58 ` Toshiaki Makita
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2022-02-10 16:57 UTC (permalink / raw)
To: Toshiaki Makita
Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, netdev,
Toshiaki Makita, syzbot
On Wed, Feb 9, 2022 at 4:36 AM Toshiaki Makita
<toshiaki.makita1@gmail.com> wrote:
>
> On 2022/02/09 8:28, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
>
> Thank you for handling this case.
>
> > veth being NETIF_F_LLTX enabled, we need to be more careful
> > whenever we read/write rq->rx_notify_masked.
> >
> > BUG: KCSAN: data-race in veth_xmit / veth_xmit
> >
> > w
> > value changed: 0x00 -> 0x01
>
> I'm not familiar with KCSAN.
> Does this mean rx_notify_masked value is changed while another CPU is reading it?
>
Yes.
> If so, I'm not sure there is a problem with that.
This is a problem if not annotated properly.
> At least we could call napi_schedule() twice, but that just causes one extra napi
> poll due to NAPIF_STATE_MISSED, and it happens very rarely?
>
> Toshiaki Makita
The issue might be more problematic, a compiler might play bad games,
look for load and store tearing.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] veth: fix races around rq->rx_notify_masked
2022-02-10 16:57 ` Eric Dumazet
@ 2022-02-14 15:58 ` Toshiaki Makita
0 siblings, 0 replies; 5+ messages in thread
From: Toshiaki Makita @ 2022-02-14 15:58 UTC (permalink / raw)
To: Eric Dumazet
Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, netdev,
Toshiaki Makita, syzbot
On 2022/02/11 1:57, Eric Dumazet wrote:
> On Wed, Feb 9, 2022 at 4:36 AM Toshiaki Makita
> <toshiaki.makita1@gmail.com> wrote:
>>
>> On 2022/02/09 8:28, Eric Dumazet wrote:
>>> From: Eric Dumazet <edumazet@google.com>
>>
>> Thank you for handling this case.
>>
>>> veth being NETIF_F_LLTX enabled, we need to be more careful
>>> whenever we read/write rq->rx_notify_masked.
>>>
>>> BUG: KCSAN: data-race in veth_xmit / veth_xmit
>>>
>>> w
>>> value changed: 0x00 -> 0x01
>>
>> I'm not familiar with KCSAN.
>> Does this mean rx_notify_masked value is changed while another CPU is reading it?
>>
>
> Yes.
>
>> If so, I'm not sure there is a problem with that.
>
> This is a problem if not annotated properly.
>
>> At least we could call napi_schedule() twice, but that just causes one extra napi
>> poll due to NAPIF_STATE_MISSED, and it happens very rarely?
>
> The issue might be more problematic, a compiler might play bad games,
> look for load and store tearing.
Thank you.
I assume you mean problems like those listed in this page,
https://lwn.net/Articles/793253/
e.g. "invented stores", not exactly load/store tearing.
(since rx_notify_masked is 0/1 value and seems not able to be teared)
Now I understand that it's pretty hard to know what exact problem will happen as the
behavior is undefined and depends heavily on compiler implementation.
Thank you for the fix again!
Toshiaki Makita
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-14 15:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-08 23:28 [PATCH net] veth: fix races around rq->rx_notify_masked Eric Dumazet
2022-02-09 12:10 ` patchwork-bot+netdevbpf
2022-02-09 12:36 ` Toshiaki Makita
2022-02-10 16:57 ` Eric Dumazet
2022-02-14 15:58 ` Toshiaki Makita
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.