[PATCH net] wireguard: wait for per-peer crypto during removal

[PATCH net] wireguard: wait for per-peer crypto during removal

[Chris J Arges]

Calling peer_remove_after_dead() currently flushes device-wide packet
crypto and handshake workqueues while holding RTNL. This is problematic as
unrelated peers can continue adding work onto those queues blocking other
tasks that want to take the RTNL lock.

Instead, this patch proposes tracking pending crypto handoffs for each
peer using a counter. After marking the peer dead, synchronize_net()
prevents new submissions; wait for pending crypto workers to schedule TX
work or RX NAPI. Then flush only the peer's transmit packet and handshake
work.

This scopes teardown synchronization to the removed peer and prevents
unrelated peers from extending the RTNL hold time.

Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
Signed-off-by: Chris J Arges <carges@cloudflare.com>
---
 drivers/net/wireguard/peer.c     | 30 ++++++++++++++++--------------
 drivers/net/wireguard/peer.h     |  1 +
 drivers/net/wireguard/queueing.h |  6 ++++++
 3 files changed, 23 insertions(+), 14 deletions(-)

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
@@ -14,6 +14,7 @@
 #include <linux/lockdep.h>
 #include <linux/rcupdate.h>
 #include <linux/list.h>
+#include <linux/wait_bit.h>
 
 static struct kmem_cache *peer_cache;
 static atomic64_t peer_counter = ATOMIC64_INIT(0);
@@ -49,6 +50,8 @@ struct wg_peer *wg_peer_create(struct wg_device *wg,
 	INIT_WORK(&peer->transmit_packet_work, wg_packet_tx_worker);
 	wg_prev_queue_init(&peer->tx_queue);
 	wg_prev_queue_init(&peer->rx_queue);
+	/* Keep this above zero until teardown prevents new packet handoffs. */
+	atomic_set(&peer->packet_crypt_pending, 1);
 	rwlock_init(&peer->endpoint_lock);
 	kref_init(&peer->refcount);
 	skb_queue_head_init(&peer->staged_packet_queue);
@@ -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);
-	/* 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);
 
-	/* Ensure any workstructs we own (like transmit_handshake_work or
-	 * clear_peer_work) no longer are in use.
+	/* clear_peer_work was flushed by wg_timers_stop(). Ensure the remaining
+	 * peer-owned handshake work is no longer in use.
 	 */
-	flush_workqueue(peer->device->handshake_send_wq);
+	flush_work(&peer->transmit_handshake_work);
 
 	/* After the above flushes, a peer might still be active in a few
 	 * different contexts: 1) from xmit(), before hitting is_dead and
diff --git a/drivers/net/wireguard/peer.h b/drivers/net/wireguard/peer.h
index 718fb42bdac7..64412c67f413 100644
--- a/drivers/net/wireguard/peer.h
+++ b/drivers/net/wireguard/peer.h
@@ -37,6 +37,7 @@ struct endpoint {
 struct wg_peer {
 	struct wg_device *device;
 	struct prev_queue tx_queue, rx_queue;
+	atomic_t packet_crypt_pending;
 	struct sk_buff_head staged_packet_queue;
 	int serial_work_cpu;
 	bool is_dead;
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
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/wait_bit.h>
 #include <net/ip_tunnels.h>
 
 struct wg_device;
@@ -161,6 +162,7 @@ static inline int wg_queue_enqueue_per_device_and_peer(
 	 */
 	if (unlikely(!wg_prev_queue_enqueue(peer_queue, skb)))
 		return -ENOSPC;
+	atomic_inc(&PACKET_PEER(skb)->packet_crypt_pending);
 
 	/* Then we queue it up in the device queue, which consumes the
 	 * packet as soon as it can.
@@ -182,6 +184,8 @@ static inline void wg_queue_enqueue_per_peer_tx(struct sk_buff *skb, enum packet
 	atomic_set_release(&PACKET_CB(skb)->state, state);
 	queue_work_on(wg_cpumask_choose_online(&peer->serial_work_cpu, peer->internal_id),
 		      peer->device->packet_crypt_wq, &peer->transmit_packet_work);
+	if (atomic_dec_and_test(&peer->packet_crypt_pending))
+		wake_up_var(&peer->packet_crypt_pending);
 	wg_peer_put(peer);
 }
 
@@ -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);
 }
 

---
base-commit: 70f3995830d3f1e79faa14eb0605914f778feca9
change-id: 20260901-fix-wg-peer-removal-43fc390d0311

Best regards,
--  
Chris J Arges <carges@cloudflare.com>

Re: [PATCH net] wireguard: wait for per-peer crypto during removal

[netdev-bot+sashiko]

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