Re: [PATCH RFC net-next v4 11/14] xsk: support batch xmit main logic

[sashiko-bot@kernel.org]

Sashiko AI review found 5 potential issue(s):
- [Critical] TOCTOU race on socket state pointers leading to NULL pointer dereference and Use-After-Free.
- [Critical] Integer underflow in `xsk_alloc_batch_skb()` leading to massive OOB memory allocation and kernel crash.
- [Critical] Unconditional dropping of partially built packets (`xs->skb`) causes buffer leaks and packet stream corruption.
- [High] Double cancellation of CQ slots and incorrect TX pointer manipulation on SKB build failure.
- [High] Infinite `-EAGAIN` livelock and stat inflation when packets span batch boundaries.
--

commit 4041fe3cb310f6f01b04ae2ff1fbd04cf0aa2713
Author: Jason Xing <kernelxing@tencent.com>
xsk: support batch xmit main logic

This commit implements the core logic for batched transmission in AF_XDP
sockets via __xsk_generic_xmit_batch(). It introduces a loop to read
descriptors, reserve completion queue slots, allocate SKBs, and transmit
packets in batches.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index b4f3cc7125804..16e47f6fc30ec 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
[ ... ]
> @@ -914,6 +914,122 @@ struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>  	return ERR_PTR(err);
>  }
>  
> +static int __xsk_generic_xmit_batch(struct xdp_sock *xs)
> +{
> +	struct xsk_buff_pool *pool = xs->pool;
> +	struct xsk_batch *batch = &xs->batch;
> +	struct xdp_desc *descs = batch->desc_cache;
> +	struct net_device *dev = xs->dev;
> +	u32 max_batch, max_budget;
> +	bool sent_frame = false;
> +	struct sk_buff *skb;
> +	u32 cons_descs;
> +	int err = 0;
> +	u32 i = 0;
> +
> +	mutex_lock(&xs->mutex);

Can a TOCTOU race occur here with a concurrent socket bind() or setsockopt()?

The pointers for pool, dev, and batch->desc_cache are read before the mutex
is acquired. If another thread modifies the socket state, could these
pointers be NULL or freed by the time xsk_is_bound() is checked, leading to
a use-after-free or NULL pointer dereference when accessing dev or descs?

> +
> +	/* Since we dropped the RCU read lock, the socket state might have changed. */
> +	if (unlikely(!xsk_is_bound(xs))) {
[ ... ]
> +	for (i = 0; i < max_budget; i += cons_descs) {
> +		u32 nb_pkts = 0;
> +		u32 nb_descs;
[ ... ]
> +		cons_descs = xskq_cons_read_desc_batch_copy(xs->tx, pool, descs,
> +							    nb_descs, &nb_pkts);
> +		if (cons_descs < nb_descs) {
> +			u32 delta = nb_descs - cons_descs;
> +
> +			xsk_cq_cancel_locked(pool, delta);
> +			xs->tx->queue_empty_descs += delta;
> +			if (!cons_descs) {
> +				err = -EAGAIN;
> +				goto out;
> +			}

Could this lead to an infinite -EAGAIN livelock and inflated statistics?

If a multi-fragment packet spans beyond the nb_descs batch limit without an
end-of-packet descriptor, xskq_cons_read_desc_batch_copy() might safely
rewind the fragments and return 0. The code then treats these valid
descriptors as empty, adds to queue_empty_descs, and returns -EAGAIN,
causing the application to loop infinitely hitting the same boundary.

> +			nb_descs = cons_descs;
> +		}
> +
> +		cons_descs = xsk_alloc_batch_skb(xs, nb_pkts, nb_descs, &err);

Does this calculation of nb_pkts trigger an integer underflow?

If a batch contains only fragments without an end-of-packet flag, nb_pkts
will be 0. Inside xsk_alloc_batch_skb(), if there is an existing partially
built skb (xs->skb != NULL), the code may decrement nb_pkts. Since nb_pkts
is unsigned, nb_pkts-- would underflow to 0xFFFFFFFF, resulting in a
massive out-of-bounds memory allocation.

> +		/* Return 'nb_descs - cons_descs' number of descs to the
> +		 * pool if the batch allocation partially fails
> +		 */
> +		if (cons_descs < nb_descs) {
> +			xskq_cons_cancel_n(xs->tx, nb_descs - cons_descs);
> +			xsk_cq_cancel_locked(pool, nb_descs - cons_descs);
> +		}

Can this cause a double cancellation of completion queue slots?

If xsk_build_skb() fails inside xsk_alloc_batch_skb() due to memory limits,
it already calls xsk_cq_cancel_locked() internally:

net/xdp/xsk.c:xsk_build_skb() {
	...
	if (err == -EOVERFLOW) {
		...
	} else {
		xsk_cq_cancel_locked(xs->pool, 1);
	}
}

Calling xsk_cq_cancel_locked() again here for the failed descriptors would
underflow the completion queue pointers, potentially corrupting the user
space ring buffer. 

Also, does xsk_build_skb() advancing the TX consumer pointer on -EOVERFLOW
conflict with the rewind happening here?

[ ... ]
> +		if (err)
> +			goto out;
> +	}
> +
> +	/* Maximum budget of descriptors have been consumed */
> +	if (xskq_has_descs(xs->tx))
> +		err = -EAGAIN;
> +
> +out:
> +	if (xs->skb)
> +		xsk_drop_skb(xs->skb);

Does unconditionally dropping the partially built xs->skb corrupt the packet
stream? 

If transmission hits a NETDEV_TX_BUSY (-EAGAIN) or a valid multi-fragment
packet spans a batch boundary, the code drops the fragments processed so far. 
Because the completion queue slots and TX pointers for these early fragments
might not be properly rewound or notified, does this permanently leak buffers
and merge unrelated fragments into corrupted packets on the next sendmsg()
retry?

> +
> +	/* If send_queue has more pending skbs, we must to clear
> +	 * the rest of them.
> +	 */
> +	while ((skb = __skb_dequeue(&batch->send_queue)) != NULL) {

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