| 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
| ||
|
Message-ID: <CAHmME9qvTUj4RUBo7AJXKhVSxvKbKxbKsnU0_rDfGSeXLNvb+Q@mail.gmail.com>
Date: Fri, 17 Nov 2023 15:28:16 +0100
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Eric Dumazet <edumazet@...gle.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>, Hangbin Liu <liuhangbin@...il.com>
Subject: Re: [PATCH v2 net] wireguard: use DEV_STATS_INC()
Hi Eric,
On Fri, Nov 17, 2023 at 3:17 PM Eric Dumazet <edumazet@...gle.com> 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
> ...
>
> v2: also change wg_packet_consume_data_done() (Hangbin Liu)
> and wg_packet_purge_staged_packets()
>
> 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>
> Cc: Hangbin Liu <liuhangbin@...il.com>
> ---
> drivers/net/wireguard/device.c | 4 ++--
> drivers/net/wireguard/receive.c | 12 ++++++------
> drivers/net/wireguard/send.c | 3 ++-
> 3 files changed, 10 insertions(+), 9 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;
> }
> diff --git a/drivers/net/wireguard/receive.c b/drivers/net/wireguard/receive.c
> index 0b3f0c843550957ee1fe3bed7185a7d990246c2b..a176653c88616b1bc871fe52fcea778b5e189f69 100644
> --- a/drivers/net/wireguard/receive.c
> +++ b/drivers/net/wireguard/receive.c
> @@ -416,20 +416,20 @@ static void wg_packet_consume_data_done(struct wg_peer *peer,
> net_dbg_skb_ratelimited("%s: Packet has unallowed src IP (%pISc) from peer %llu (%pISpfsc)\n",
> dev->name, skb, peer->internal_id,
> &peer->endpoint.addr);
> - ++dev->stats.rx_errors;
> - ++dev->stats.rx_frame_errors;
> + DEV_STATS_INC(dev, rx_errors);
> + DEV_STATS_INC(dev, rx_frame_errors);
> goto packet_processed;
> dishonest_packet_type:
> net_dbg_ratelimited("%s: Packet is neither ipv4 nor ipv6 from peer %llu (%pISpfsc)\n",
> dev->name, peer->internal_id, &peer->endpoint.addr);
> - ++dev->stats.rx_errors;
> - ++dev->stats.rx_frame_errors;
> + DEV_STATS_INC(dev, rx_errors);
> + DEV_STATS_INC(dev, rx_frame_errors);
> goto packet_processed;
> dishonest_packet_size:
> net_dbg_ratelimited("%s: Packet has incorrect size from peer %llu (%pISpfsc)\n",
> dev->name, peer->internal_id, &peer->endpoint.addr);
> - ++dev->stats.rx_errors;
> - ++dev->stats.rx_length_errors;
> + DEV_STATS_INC(dev, rx_errors);
> + DEV_STATS_INC(dev, rx_length_errors);
> goto packet_processed;
> packet_processed:
> dev_kfree_skb(skb);
> diff --git a/drivers/net/wireguard/send.c b/drivers/net/wireguard/send.c
> index 95c853b59e1dae1df8b4e5cbf4e3541e35806b82..0d48e0f4a1ba3e1f11825136a65de0867b204496 100644
> --- a/drivers/net/wireguard/send.c
> +++ b/drivers/net/wireguard/send.c
> @@ -333,7 +333,8 @@ static void wg_packet_create_data(struct wg_peer *peer, struct sk_buff *first)
> void wg_packet_purge_staged_packets(struct wg_peer *peer)
> {
> spin_lock_bh(&peer->staged_packet_queue.lock);
> - peer->device->dev->stats.tx_dropped += peer->staged_packet_queue.qlen;
> + DEV_STATS_ADD(peer->device->dev, tx_dropped,
> + peer->staged_packet_queue.qlen);
> __skb_queue_purge(&peer->staged_packet_queue);
> spin_unlock_bh(&peer->staged_packet_queue.lock);
> }
This is probably fine if you want to do it and feel strongly about it,
and you can take this directly into net/net-next with my:
Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
However, I recall evaluating the races here long ago and deliberately
deciding not to do anything about it. Sure KCSAN will complain, but
these stats being pixel perfect isn't really _that_ important and it
really doesn't seem worth it to have the performance hit of several
atomics on every packet. There's also peer->{r,t}x_bytes that should
probably be adjusted if you're going to change these. But again - is
it really worth it to do that? It just seems like such an unnecessary
performance hit.
So I think I'd prefer to _not_ fix this. But if you feel really
strongly about it, I'll be okay deferring to your judgement.
Jason
Powered by blists - more mailing lists