netdev - Re: [PATCH v2 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 for Android: free password hash cracker in your pocket

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

Date: Mon, 20 Nov 2023 09:13:29 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: "Jason A. Donenfeld" <Jason@...c4.com>
Cc: patchwork-bot+netdevbpf@...nel.org, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, netdev@...r.kernel.org, eric.dumazet@...il.com, 
	syzkaller@...glegroups.com, liuhangbin@...il.com
Subject: Re: [PATCH v2 net] wireguard: use DEV_STATS_INC()

On Mon, Nov 20, 2023 at 8:56 AM Eric Dumazet <edumazet@...gle.com> wrote:
>

> Jason, I was in week end mode, so could not reply to your message.
>
> This fix is rather obvious to me. I do not want to spend too much time on it,
> and you gave an ACK if I am not mistaken.
>
> If you prefer not letting syzbot find other bugs in wireguard (because
> hitting this issue first),
> this is fine by me. We can ask syzbot team to not include wireguard in
> their kernels.

BTW, while cooking the patch I found that wireguard was incorrectly
using dev_kfree_skb() instead of kfree_skb().

dev_kfree_skb() is really a consume_skb(), which gives different drop
monitor signals

(perf record -a -e skb:kfree_skb)  vs (perf record -a -e skb:consume_skb)

I would suggest you take a look.

Ideally, using kfree_skb_reason(skb, reason) (for net-next tree) would
help future diagnostics.

For net tree I would suggest

diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
index 258dcc1039216f311a223fd348295d4b5e03a3ed..0b0e2a9fd14d14fb3c77004074e2b088364d332a
100644
--- a/drivers/net/wireguard/device.c
+++ b/drivers/net/wireguard/device.c
@@ -209,7 +209,7 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb,
struct net_device *dev)
         * we don't remove GSO segments that are in excess.
         */
        while (skb_queue_len(&peer->staged_packet_queue) > MAX_STAGED_PACKETS) {
-               dev_kfree_skb(__skb_dequeue(&peer->staged_packet_queue));
+               kfree_skb(__skb_dequeue(&peer->staged_packet_queue));
                ++dev->stats.tx_dropped;
        }
        skb_queue_splice_tail(&packets, &peer->staged_packet_queue);
diff --git a/drivers/net/wireguard/receive.c b/drivers/net/wireguard/receive.c
index 0b3f0c843550957ee1fe3bed7185a7d990246c2b..a9e76722b22cad65bda91f306fad11cdb6acff09
100644
--- a/drivers/net/wireguard/receive.c
+++ b/drivers/net/wireguard/receive.c
@@ -432,7 +432,7 @@ static void wg_packet_consume_data_done(struct
wg_peer *peer,
        ++dev->stats.rx_length_errors;
        goto packet_processed;
 packet_processed:
-       dev_kfree_skb(skb);
+       kfree_skb(skb);
 }

 int wg_packet_rx_poll(struct napi_struct *napi, int budget)
@@ -478,7 +478,7 @@ int wg_packet_rx_poll(struct napi_struct *napi, int budget)
                wg_noise_keypair_put(keypair, false);
                wg_peer_put(peer);
                if (unlikely(free))
-                       dev_kfree_skb(skb);
+                       kfree_skb(skb);

                if (++work_done >= budget)
                        break;
@@ -536,7 +536,7 @@ static void wg_packet_consume_data(struct
wg_device *wg, struct sk_buff *skb)
 err_keypair:
        rcu_read_unlock_bh();
        wg_peer_put(peer);
-       dev_kfree_skb(skb);
+       kfree_skb(skb);
 }

 void wg_packet_receive(struct wg_device *wg, struct sk_buff *skb)
@@ -582,5 +582,5 @@ void wg_packet_receive(struct wg_device *wg,
struct sk_buff *skb)
        return;

 err:
-       dev_kfree_skb(skb);
+       kfree_skb(skb);
 }
diff --git a/drivers/net/wireguard/socket.c b/drivers/net/wireguard/socket.c
index 0414d7a6ce74141cd2ca365bfd1da727691e27ec..7e773a2c8a8532caef06205f53e6b505dbe9ff57
100644
--- a/drivers/net/wireguard/socket.c
+++ b/drivers/net/wireguard/socket.c
@@ -178,7 +178,7 @@ int wg_socket_send_skb_to_peer(struct wg_peer
*peer, struct sk_buff *skb, u8 ds)
                ret = send6(peer->device, skb, &peer->endpoint, ds,
                            &peer->endpoint_cache);
        else
-               dev_kfree_skb(skb);
+               kfree_skb(skb);
        if (likely(!ret))
                peer->tx_bytes += skb_len;
        read_unlock_bh(&peer->endpoint_lock);