Re: [PATCH v4 net 2/6] 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:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but rolling back a partial multi-buffer packet via xskq_cons_cancel_n() on NETDEV_TX_BUSY corrupts the Tx ring because the consumer index may have already been prematurely committed to user-space in a previous batch.
--

--- Patch [2]: [PATCH v4 net 2/6] xsk: drain continuation descs after overflow in xsk_build_skb() ---
commit ec981f427791ffa015b5a35518b1a57e2420d64d
Author: Jason Xing <kerneljasonxing@gmail.com>

xsk: drain continuation descs after overflow in xsk_build_skb()

Fixes generic xmit path multi-buffer logic when packets are oversized
or contain invalid descriptors. Introduces the xdp_sock::drain_cont flag
to continue consuming descriptors from the Tx ring and put them directly
onto Cq instead of silently losing them.

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

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a7a83dc4546a0..12a845d012f6f 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> @@ -1096,18 +1120,33 @@ static int __xsk_generic_xmit(struct sock *sk)

[ ... ]

>  out:
> -	if (sent_frame)
> +	if (xs->tx->cached_cons != cached_cons)
>  		__xsk_tx_release(xs);

[Severity: High]
This isn't a bug introduced by this patch, but does this change to the
release condition expose a pre-existing issue where rolling back a partial
multi-buffer packet corrupts the Tx ring?

During multi-buffer packet assembly, if the max_batch limit is reached,
__xsk_generic_xmit() exits and commits the partially consumed descriptors
to ring->consumer. User-space observes this and may overwrite those slots.

On the next call to __xsk_generic_xmit(), packet assembly resumes. If
__dev_direct_xmit() subsequently returns NETDEV_TX_BUSY, the code calls
xskq_cons_cancel_n() to rewind the local cached_cons by the full packet
length:

	if (err == NETDEV_TX_BUSY) {
		/* Tell MAC device to consume more and retry later. */
		xskq_cons_cancel_n(xs->tx, xsk_get_num_desc(skb));
		...

Because cached_cons is rewound by the full length (including fragments
consumed in the previous call), it becomes smaller than the cached_cons
saved at the start of this current call.

Will this new condition (xs->tx->cached_cons != cached_cons) then evaluate
to true and commit the rewound index to the globally visible ring->consumer?

If ring->consumer jumps backwards, wouldn't the kernel fetch descriptors
that user-space may have already overwritten with new data upon retry?

>  
>  	mutex_unlock(&xs->mutex);

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