All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.