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

[netdev-bot+sashiko@kernel.org]

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