Re: [PATCH net 4/7] xsk: reclaim offending invalid desc in generic multi-buffer Tx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incomplete reclaim of the first invalid descriptor in a multi-buffer packet, leaking a completion.
- [Medium] Artificial inflation of the tx_invalid_descs counter when the completion queue is full.
--

commit 94ab243e4d16baca076b39e394196d66b2624dbb
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

xsk: reclaim offending invalid desc in generic multi-buffer Tx

The patch aims to reclaim invalid descriptors found in __xsk_generic_xmit()
by adding them to the completion ring before releasing them from the Tx ring.
This targets failed descriptors belonging to already-started or
already-draining multi-buffer packets.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index c489fadc36087..43791647cf182 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -1125,8 +1125,22 @@ static int __xsk_generic_xmit(struct sock *sk)
>  	}
>  
>  	if (xskq_has_descs(xs->tx)) {
> +		bool reclaim_desc = xs->skb || xs->drain_cont;

[Severity: High]
Does this logic correctly handle the first invalid descriptor of a
multi-buffer packet?

If the very first descriptor of a multi-buffer packet is invalid, both
xs->skb and xs->drain_cont will be false, meaning reclaim_desc evaluates
to false. The invalid descriptor will then be dropped without submitting
an address to the completion queue.

However, xs->drain_cont is subsequently set to true below because
xp_mb_desc(&desc) evaluates to true. Subsequent valid descriptors of the
same packet will then be processed in the drain path and have completions
generated, leaving userspace with N-1 completions for an N-fragment packet.

> +
> +		if (reclaim_desc) {
> +			err = xsk_cq_reserve_locked(xs->pool);
> +			if (err) {
> +				err = -EAGAIN;
> +				goto out;

[Severity: Medium]
Could returning -EAGAIN here artificially inflate the tx_invalid_descs
statistic?

If xsk_cq_reserve_locked() fails, the function returns -EAGAIN without
consuming the descriptor since xskq_cons_release(xs->tx) is skipped.

On subsequent retries, when xskq_cons_peek_desc() evaluates this exact same
invalid descriptor, it unconditionally increments q->invalid_descs (in
xskq_cons_is_valid_desc() in net/xdp/xsk_queue.h). Since the descriptor
is never consumed when the completion queue is full, the counter will be
incremented repeatedly on each retry.

> +			}
> +		}
> +
>  		if (xs->skb)
>  			xsk_drop_skb(xs->skb);
> +
> +		if (reclaim_desc)
> +			xsk_cq_submit_addr_single_locked(xs->pool, &desc);
> +
>  		xskq_cons_release(xs->tx);
>  		xs->drain_cont = xp_mb_desc(&desc);
>  	}

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