Re: [PATCH net v2 3/5] xsk: drain continuation descs after overflow in xsk_build_skb()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] UAPI breakage and transmission batch abortion due to returning -EOVERFLOW to userspace
--

commit 1d238b3786b35997fb62e781dd7528f204ca8b34
Author: Jason Xing <kerneljasonxing@gmail.com>

xsk: drain continuation descs after overflow in xsk_build_skb()

When a multi-buffer packet exceeds MAX_SKB_FRAGS and triggers -EOVERFLOW,
only the current descriptor is released from the TX ring. This patch
introduces a drain_cont flag to properly handle the remaining continuation
descriptors, preventing them from being misinterpreted as the beginning
of a new packet.

Link: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 0a6203c425766..9c51eed6ee99c 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
[ ... ]
> @@ -1062,6 +1064,26 @@ static int __xsk_generic_xmit(struct sock *sk)
>  			goto out;
>  		}
>  
> +		if (unlikely(xs->drain_cont)) {
> +			unsigned long flags;
> +			u32 idx;
> +
> +			spin_lock_irqsave(&xs->pool->cq_prod_lock, flags);
> +			idx = xskq_get_prod(xs->pool->cq);
> +			xskq_prod_write_addr(xs->pool->cq, idx, desc.addr);
> +			xskq_prod_submit_n(xs->pool->cq, 1);
> +			spin_unlock_irqrestore(&xs->pool->cq_prod_lock, flags);
> +
> +			xs->tx->invalid_descs++;
> +			xskq_cons_release(xs->tx);
> +			if (!xp_mb_desc(&desc)) {
> +				xs->drain_cont = false;
> +				err = -EOVERFLOW;
> +				goto out;

Does returning -EOVERFLOW to userspace break the expected UAPI here?

The original code suppressed the -EOVERFLOW error to silently drop the invalid
packet and continue processing the batch:

		if (IS_ERR(skb)) {
			err = PTR_ERR(skb);
			if (err != -EOVERFLOW)
				goto out;
			err = 0;
			continue;
		}

By using goto out with err = -EOVERFLOW, doesn't this abort the transmission
batch prematurely? Any valid packets queued immediately behind the malformed
packet would not be processed during the current system call.

Additionally, if the batch is aborted before any valid packets are transmitted
(when sent_frame == false), the __xsk_tx_release(xs) call at the out label
will be skipped. Will this cause the global TX consumer index to remain
un-updated for the dropped descriptors, leading to a ring slot leak from the
userspace perspective?

Note that a later patch in the series ('selftests/xsk: fix multi-buffer
invalid desc tests for drain_cont') acknowledges this behavior by modifying
the selftests to tolerate the early return, but does this actually fix the
underlying ABI breakage in the kernel?

> +			}
> +			continue;
> +		}
> +
>  		skb = xsk_build_skb(xs, &desc);
>  		if (IS_ERR(skb)) {
>  			err = PTR_ERR(skb);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515123018.80147-1-kerneljasonxing@gmail.com?part=3