* [PATCH net] wireguard: use DEV_STATS_INC()
@ 2023-11-16 10:02 Eric Dumazet
2023-11-17 1:34 ` Hangbin Liu
0 siblings, 1 reply; 3+ messages in thread
From: Eric Dumazet @ 2023-11-16 10:02 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: netdev, eric.dumazet, Eric Dumazet, syzbot, Jason A . Donenfeld
wg_xmit() can be called concurrently, KCSAN reported [1]
some device stats updates can be lost.
Use DEV_STATS_INC() for this unlikely case.
[1]
BUG: KCSAN: data-race in wg_xmit / wg_xmit
read-write to 0xffff888104239160 of 8 bytes by task 1375 on cpu 0:
wg_xmit+0x60f/0x680 drivers/net/wireguard/device.c:231
__netdev_start_xmit include/linux/netdevice.h:4918 [inline]
netdev_start_xmit include/linux/netdevice.h:4932 [inline]
xmit_one net/core/dev.c:3543 [inline]
dev_hard_start_xmit+0x11b/0x3f0 net/core/dev.c:3559
...
read-write to 0xffff888104239160 of 8 bytes by task 1378 on cpu 1:
wg_xmit+0x60f/0x680 drivers/net/wireguard/device.c:231
__netdev_start_xmit include/linux/netdevice.h:4918 [inline]
netdev_start_xmit include/linux/netdevice.h:4932 [inline]
xmit_one net/core/dev.c:3543 [inline]
dev_hard_start_xmit+0x11b/0x3f0 net/core/dev.c:3559
...
Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
---
drivers/net/wireguard/device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
index 258dcc1039216f311a223fd348295d4b5e03a3ed..deb9636b0ecf8f47e832a0b07e9e049ba19bdf16 100644
--- a/drivers/net/wireguard/device.c
+++ b/drivers/net/wireguard/device.c
@@ -210,7 +210,7 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb, struct net_device *dev)
*/
while (skb_queue_len(&peer->staged_packet_queue) > MAX_STAGED_PACKETS) {
dev_kfree_skb(__skb_dequeue(&peer->staged_packet_queue));
- ++dev->stats.tx_dropped;
+ DEV_STATS_INC(dev, tx_dropped);
}
skb_queue_splice_tail(&packets, &peer->staged_packet_queue);
spin_unlock_bh(&peer->staged_packet_queue.lock);
@@ -228,7 +228,7 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb, struct net_device *dev)
else if (skb->protocol == htons(ETH_P_IPV6))
icmpv6_ndo_send(skb, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, 0);
err:
- ++dev->stats.tx_errors;
+ DEV_STATS_INC(dev, tx_errors);
kfree_skb(skb);
return ret;
}
--
2.43.0.rc0.421.g78406f8d94-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net] wireguard: use DEV_STATS_INC()
2023-11-16 10:02 [PATCH net] wireguard: use DEV_STATS_INC() Eric Dumazet
@ 2023-11-17 1:34 ` Hangbin Liu
2023-11-17 14:08 ` Eric Dumazet
0 siblings, 1 reply; 3+ messages in thread
From: Hangbin Liu @ 2023-11-17 1:34 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jason A . Donenfeld
On Thu, Nov 16, 2023 at 10:02:17AM +0000, Eric Dumazet wrote:
> wg_xmit() can be called concurrently, KCSAN reported [1]
> some device stats updates can be lost.
>
> Use DEV_STATS_INC() for this unlikely case.
>
> [1]
> BUG: KCSAN: data-race in wg_xmit / wg_xmit
>
> read-write to 0xffff888104239160 of 8 bytes by task 1375 on cpu 0:
> wg_xmit+0x60f/0x680 drivers/net/wireguard/device.c:231
> __netdev_start_xmit include/linux/netdevice.h:4918 [inline]
> netdev_start_xmit include/linux/netdevice.h:4932 [inline]
> xmit_one net/core/dev.c:3543 [inline]
> dev_hard_start_xmit+0x11b/0x3f0 net/core/dev.c:3559
> ...
>
> read-write to 0xffff888104239160 of 8 bytes by task 1378 on cpu 1:
> wg_xmit+0x60f/0x680 drivers/net/wireguard/device.c:231
> __netdev_start_xmit include/linux/netdevice.h:4918 [inline]
> netdev_start_xmit include/linux/netdevice.h:4932 [inline]
> xmit_one net/core/dev.c:3543 [inline]
> dev_hard_start_xmit+0x11b/0x3f0 net/core/dev.c:3559
> ...
>
> Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> drivers/net/wireguard/device.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
> index 258dcc1039216f311a223fd348295d4b5e03a3ed..deb9636b0ecf8f47e832a0b07e9e049ba19bdf16 100644
> --- a/drivers/net/wireguard/device.c
> +++ b/drivers/net/wireguard/device.c
> @@ -210,7 +210,7 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb, struct net_device *dev)
> */
> while (skb_queue_len(&peer->staged_packet_queue) > MAX_STAGED_PACKETS) {
> dev_kfree_skb(__skb_dequeue(&peer->staged_packet_queue));
> - ++dev->stats.tx_dropped;
> + DEV_STATS_INC(dev, tx_dropped);
> }
> skb_queue_splice_tail(&packets, &peer->staged_packet_queue);
> spin_unlock_bh(&peer->staged_packet_queue.lock);
> @@ -228,7 +228,7 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb, struct net_device *dev)
> else if (skb->protocol == htons(ETH_P_IPV6))
> icmpv6_ndo_send(skb, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, 0);
> err:
> - ++dev->stats.tx_errors;
> + DEV_STATS_INC(dev, tx_errors);
> kfree_skb(skb);
> return ret;
> }
> --
> 2.43.0.rc0.421.g78406f8d94-goog
>
Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
BTW, what about the receive part function wg_packet_consume_data_done(), do
we need to use DEV_STATS_INC()?
Thanks
Hangbin
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] wireguard: use DEV_STATS_INC()
2023-11-17 1:34 ` Hangbin Liu
@ 2023-11-17 14:08 ` Eric Dumazet
0 siblings, 0 replies; 3+ messages in thread
From: Eric Dumazet @ 2023-11-17 14:08 UTC (permalink / raw)
To: Hangbin Liu
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, netdev,
eric.dumazet, syzbot, Jason A . Donenfeld
On Fri, Nov 17, 2023 at 2:34 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Thu, Nov 16, 2023 at 10:02:17AM +0000, Eric Dumazet wrote:
> > wg_xmit() can be called concurrently, KCSAN reported [1]
> > some device stats updates can be lost.
> >
> > Use DEV_STATS_INC() for this unlikely case.
> >
> > [1]
> > BUG: KCSAN: data-race in wg_xmit / wg_xmit
> >
> > read-write to 0xffff888104239160 of 8 bytes by task 1375 on cpu 0:
> > wg_xmit+0x60f/0x680 drivers/net/wireguard/device.c:231
> > __netdev_start_xmit include/linux/netdevice.h:4918 [inline]
> > netdev_start_xmit include/linux/netdevice.h:4932 [inline]
> > xmit_one net/core/dev.c:3543 [inline]
> > dev_hard_start_xmit+0x11b/0x3f0 net/core/dev.c:3559
> > ...
> >
> > read-write to 0xffff888104239160 of 8 bytes by task 1378 on cpu 1:
> > wg_xmit+0x60f/0x680 drivers/net/wireguard/device.c:231
> > __netdev_start_xmit include/linux/netdevice.h:4918 [inline]
> > netdev_start_xmit include/linux/netdevice.h:4932 [inline]
> > xmit_one net/core/dev.c:3543 [inline]
> > dev_hard_start_xmit+0x11b/0x3f0 net/core/dev.c:3559
> > ...
> >
> > Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> > drivers/net/wireguard/device.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
> > index 258dcc1039216f311a223fd348295d4b5e03a3ed..deb9636b0ecf8f47e832a0b07e9e049ba19bdf16 100644
> > --- a/drivers/net/wireguard/device.c
> > +++ b/drivers/net/wireguard/device.c
> > @@ -210,7 +210,7 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb, struct net_device *dev)
> > */
> > while (skb_queue_len(&peer->staged_packet_queue) > MAX_STAGED_PACKETS) {
> > dev_kfree_skb(__skb_dequeue(&peer->staged_packet_queue));
> > - ++dev->stats.tx_dropped;
> > + DEV_STATS_INC(dev, tx_dropped);
> > }
> > skb_queue_splice_tail(&packets, &peer->staged_packet_queue);
> > spin_unlock_bh(&peer->staged_packet_queue.lock);
> > @@ -228,7 +228,7 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb, struct net_device *dev)
> > else if (skb->protocol == htons(ETH_P_IPV6))
> > icmpv6_ndo_send(skb, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, 0);
> > err:
> > - ++dev->stats.tx_errors;
> > + DEV_STATS_INC(dev, tx_errors);
> > kfree_skb(skb);
> > return ret;
> > }
> > --
> > 2.43.0.rc0.421.g78406f8d94-goog
> >
>
> Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
>
> BTW, what about the receive part function wg_packet_consume_data_done(), do
> we need to use DEV_STATS_INC()?
You are right, I will send a v2
(wg_packet_purge_staged_packets() also needs to be changed)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-17 14:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-16 10:02 [PATCH net] wireguard: use DEV_STATS_INC() Eric Dumazet
2023-11-17 1:34 ` Hangbin Liu
2023-11-17 14:08 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox