Re: [PATCH v2 net 3/5] xsk: reclaim invalid multi-buffer Tx descs in ZC path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Infinite loop and CPU soft lockup in NAPI context when processing an incomplete multi-buffer packet on a shared UMEM socket.
- [High] Permanent TX queue stall or deadlock when a valid multi-buffer packet on a shared UMEM socket exceeds the TX ring size of the first bound socket.
--

--- Patch [3]: [PATCH v2 net 3/5] xsk: reclaim invalid multi-buffer Tx descs in ZC path ---
commit 1dba20f5d7f0672e2b323a1eff536a35b9369bb8
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

xsk: reclaim invalid multi-buffer Tx descs in ZC path

The zero-copy Tx batch parser stops when it encounters an invalid
descriptor. If this happens after one or more continuation descriptors,
the Tx consumer can be advanced past fragments that are neither submitted
to the driver nor returned to userspace through the completion ring.
This patch parses Tx batches in packet units and reclaims descriptors
belonging to invalid or oversized packets.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 43791647cf182..7d8b42c4c214b 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> @@ -574,22 +591,169 @@ static u32 xsk_tx_peek_release_fallback(struct xsk_buff_pool *pool, u32 max_entr
[ ... ]
> +static struct xsk_tx_batch
> +__xsk_tx_peek_release_desc_batch(struct xsk_buff_pool *pool, struct xdp_sock *xs,
> +				 struct xdp_desc *descs, u32 max_descs)
> +{
> +	struct xsk_tx_batch batch = {};
> +	u32 entries;
> +
> +	entries = xskq_cons_nb_entries(xs->tx, max_descs);
> +	if (!entries)
> +		return batch;
> +
> +	batch = xskq_cons_read_desc_batch(xs, pool, descs, entries);
                                                                   ^^^^^^^

[Severity: Critical]
Does passing the number of available fragments as the 'max' boundary hide the
true budget limit from the reader?

If an unprivileged user space application sharing a UMEM pool submits an
incomplete multi-buffer packet without an EOP, xskq_cons_read_desc_batch()
reaches the end of the available fragments and evaluates (nb_entries == max)
to true. It incorrectly flags the batch as budget_limited and un-reads the
descriptors.

In xsk_tx_peek_release_shared_desc_batch(), this false budget_limited flag
combined with sufficient global CQ space causes budget_exhausted to become
true. When the loop finishes with no progress, it resets the budgets and
jumps to 'again'. Since the queue state is completely unchanged, could this
result in a permanent infinite loop and a CPU soft lockup in NAPI context?

> +	if (!xsk_tx_batch_cq_descs(&batch)) {
> +		xs->tx->queue_empty_descs++;
> +		if (batch.consumed_descs) {
> +			__xskq_cons_release(xs->tx);
> +			xs->sk.sk_write_space(&xs->sk);
> +		}
> +		return batch;
> +	}
> +
> +	__xskq_cons_release(xs->tx);
> +	xs->sk.sk_write_space(&xs->sk);
> +	return batch;
> +}

[ ... ]

>  u32 xsk_tx_peek_release_desc_batch(struct xsk_buff_pool *pool, u32 nb_pkts)
>  {
> +	struct xsk_tx_batch batch = {};
>  	struct xdp_sock *xs;
> +	u32 cq_cached_prod;
>  
>  	rcu_read_lock();
> +	if (unlikely(READ_ONCE(pool->reclaim_descs)))
> +		goto out;
> +	nb_pkts = min(nb_pkts, pool->tx_descs_nentries);
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this strict cap on the batch size to the TX ring size of the first bound
socket cause issues for subsequent sockets with larger rings?

If a shared UMEM pool has a first socket with a small TX ring (e.g., 32),
pool->tx_descs_nentries becomes 32. If a second socket binds with a larger
TX ring (e.g., 128) and submits a valid multi-buffer packet larger than 32
fragments, this capped value restricts max_descs and remaining in
xsk_tx_peek_release_shared_desc_batch().

The reader will only be able to inspect up to pool->tx_descs_nentries
fragments, never reaching the EOP of the larger packet. The reader un-reads
the fragments and sets budget_limited to true.

If budget_left >= remaining (because remaining is artificially small),
budget_exhausted evaluates to false, causing the function to silently return
0. Could this permanently stall the TX queue for the socket?

> +	if (!nb_pkts)
> +		goto out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710194424.84844-1-maciej.fijalkowski@intel.com?part=3