netdev - Re: [PATCH net] wireguard: use DEV_STATS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 17 Nov 2023 15:08:33 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	syzbot <syzkaller@...glegroups.com>, "Jason A . Donenfeld" <Jason@...c4.com>
Subject: Re: [PATCH net] wireguard: use DEV_STATS_INC()

On Fri, Nov 17, 2023 at 2:34 AM Hangbin Liu <liuhangbin@...il.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@...glegroups.com>
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > Cc: Jason A. Donenfeld <Jason@...c4.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@...il.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)