From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.zx2c4.com (lists.zx2c4.com [165.227.139.114]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C1949C79F89 for ; Mon, 7 Sep 2026 06:14:46 +0000 (UTC) Received: by lists.zx2c4.com (OpenSMTPD) with ESMTP id 5dd61ca1; Mon, 7 Sep 2026 06:14:44 +0000 (UTC) Received: from sea.source.kernel.org (sea.source.kernel.org [2600:3c0a:e001:78e:0:1991:8:25]) by lists.zx2c4.com (OpenSMTPD) with ESMTPS id beb9e3ee (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO) for ; Mon, 7 Sep 2026 06:14:42 +0000 (UTC) Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by sea.source.kernel.org (Postfix) with ESMTP id 8732441FE4; Mon, 7 Sep 2026 06:14:40 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CCA4B1F00A3A; Mon, 7 Sep 2026 06:14:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1788761680; bh=JZTKe9MJ+8Ne0cG3esO2PW7HkDItB6o5bu1JnsMRmlI=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=c9/ITiTQoZ6Qlw4geQCT+G4Suuaf8253KOUsEFb7F2SmRxb6U7fzBqToA2GuohsXI H/xUg/CtyQpn2KA9o4kJgzBW1MXPDmtUG2a7CstATKAnybGL42Ti/DNWk8/9GKU25q v9rxWpcPELC5M4k3Bpy+nHllrJ9bsuID0DNvdGlwwW3rCssx7YyJ46gDYDv6aET64y 9ni9j1IOzqq4W8R/i2Wf4izxHDslIYi8Fy0f5VeRv51Dq6oDrDdcsFVhgDLg1ZPAuO p6pQi3BNQZlU34NqFNXECU1LZ/06KQXRE0taVlrdN/l6VkM+sWJ2v3vAIJBnsxneRF hfGvEElztxAQw== Subject: Re: [PATCH net] wireguard: wait for per-peer crypto during removal From: netdev-bot+sashiko@kernel.org To: carges@cloudflare.com Cc: Jason@zx2c4.com, andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, wireguard@lists.zx2c4.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@cloudflare.com Date: Mon, 07 Sep 2026 06:14:39 +0000 Message-ID: <178876167936.219967.11338017664970925838@kernel.org> In-Reply-To: <20260902-fix-wg-peer-removal-v1-1-7a0190f5cdb1@cloudflare.com> References: <20260902-fix-wg-peer-removal-v1-1-7a0190f5cdb1@cloudflare.com> X-sashiko-severity: High Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit MIME-Version: 1.0 X-BeenThere: wireguard@lists.zx2c4.com X-Mailman-Version: 2.1.30rc1 Precedence: list List-Id: Development discussion of WireGuard List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: wireguard-bounces@lists.zx2c4.com Sender: "WireGuard" Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 1 · Medium: 0 · Low: 0 - [High] Teardown can strand decrypted packets in `peer->rx_queue`, permanently leaking the skbs and the peer. -- wireguard: wait for per-peer crypto during removal This replaces the two device-wide flush_workqueue(packet_crypt_wq) calls and the flush_workqueue(handshake_send_wq) call in peer_remove_after_dead() with a new per-peer packet_crypt_pending counter plus flush_work() on the peer's own transmit_packet_work and transmit_handshake_work. > diff --git a/drivers/net/wireguard/peer.c b/drivers/net/wireguard/peer.c > index 1cb502a932e0..f7a9c437b5b8 100644 > --- a/drivers/net/wireguard/peer.c > +++ b/drivers/net/wireguard/peer.c [ ... ] > @@ -105,28 +108,27 @@ static void peer_remove_after_dead(struct wg_peer *peer) > */ > wg_timers_stop(peer); > > - /* The transition between packet encryption/decryption queues isn't > - * guarded by is_dead, but each reference's life is strictly bounded by > - * two generations: once for parallel crypto and once for serial > - * ingestion, so we can simply flush twice, and be sure that we no > - * longer have references inside these queues. > + /* Lookup removal and is_dead prevent new packets from entering the > + * parallel crypto queues after synchronize_net() waits for pre-existing > + * submission paths. Drop the initial count and wait for existing packets > + * to finish scheduling their serial TX work or RX NAPI processing. > */ > + atomic_dec(&peer->packet_crypt_pending); > + wait_var_event(&peer->packet_crypt_pending, > + !atomic_read_acquire(&peer->packet_crypt_pending)); > + > + flush_work(&peer->transmit_packet_work); > > - /* a) For encrypt/decrypt. */ > - flush_workqueue(peer->device->packet_crypt_wq); > - /* b.1) For send (but not receive, since that's napi). */ > - flush_workqueue(peer->device->packet_crypt_wq); > - /* b.2.1) For receive (but not send, since that's wq). */ > napi_disable(&peer->napi); [Severity: High] Can this leave entries in peer->rx_queue when the peer is torn down, and leak both the skbs and the wg_peer itself? The new counter is decremented in wg_queue_enqueue_per_peer_rx() only after the packet has been put on peer->rx_queue and NAPI has been scheduled: drivers/net/wireguard/queueing.h:wg_queue_enqueue_per_peer_rx() { atomic_set_release(&PACKET_CB(skb)->state, state); napi_schedule(&peer->napi); if (atomic_dec_and_test(&peer->packet_crypt_pending)) wake_up_var(&peer->packet_crypt_pending); wg_peer_put(peer); } So packet_crypt_pending reaching zero means "handoff done, NAPI scheduled", not "rx_queue drained". For an rx-only peer flush_work(&peer->transmit_packet_work) is a no-op, so napi_disable() can now run immediately after the last decrypt handoff while up to MAX_QUEUED_PACKETS entries are still queued. If wg_packet_rx_poll() consumes exactly budget entries it breaks out without calling napi_complete_done(): drivers/net/wireguard/receive.c:wg_packet_rx_poll() { ... if (++work_done >= budget) break; } if (work_done < budget) napi_complete_done(napi, work_done); return work_done; } and __napi_poll() then takes the completion path rather than a repoll, because napi_disable_locked() has already set the disable-pending state: net/core/dev.c:__napi_poll() { ... if (unlikely(napi_disable_pending(n))) { napi_complete(n); return work; } so napi_disable()/netif_napi_del() finish with a non-empty rx_queue, and is_dead plus the completed counter wait mean nothing schedules the NAPI again. Each stranded skb still holds the peer reference taken by wg_peer_get_maybe_zero() in wg_packet_consume_data() and a keypair reference, and those are only dropped at the next: label of wg_packet_rx_poll(): next: wg_noise_keypair_put(keypair, false); wg_peer_put(peer); Is there any other consumer of peer->rx_queue that could drain it? If not, the peer kref never reaches zero, so kref_release()/rcu_release() never run and the skbs, the wg_peer, its dst_cache and the staged queue are never freed. That also means the existing safety net in rcu_release(): WARN_ON(wg_prev_queue_peek(&peer->tx_queue) || wg_prev_queue_peek(&peer->rx_queue)); can never fire to report it. The pre-patch code did not strictly guarantee the drain either, but the two flush_workqueue(peer->device->packet_crypt_wq) calls being removed blocked for as long as the shared crypt workers kept running, which is exactly the heavy-traffic case where rx_queue gets deep, and in practice gave the NAPI softirq time to drain it. Would it make sense to have the teardown path wait for the rx_queue to actually empty (or purge it explicitly) rather than only for the handoff counter? > - /* b.2.1) It's now safe to remove the napi struct, which must be done > + /* It's now safe to remove the napi struct, which must be done > * here from process context. > */ > netif_napi_del(&peer->napi); [ ... ] > diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h > index 79b6d70de236..fdd34f0f15a6 100644 > --- a/drivers/net/wireguard/queueing.h > +++ b/drivers/net/wireguard/queueing.h [ ... ] > @@ -194,6 +198,8 @@ static inline void wg_queue_enqueue_per_peer_rx(struct sk_buff *skb, enum packet > > atomic_set_release(&PACKET_CB(skb)->state, state); > napi_schedule(&peer->napi); > + if (atomic_dec_and_test(&peer->packet_crypt_pending)) > + wake_up_var(&peer->packet_crypt_pending); > wg_peer_put(peer); > } -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902-fix-wg-peer-removal-v1-1-7a0190f5cdb1%40cloudflare.com