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

Jason Xing
2026-04-16T09:59:05Z

On Wed, Apr 15, 2026 at 5:38 PM <sashiko-bot@kernel.org> wrote:
>
> 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?

Indeed, I'm going to move those read operations under the mutex lock protection.

>
> > +
> > + /* 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.

Point taken. It costs me some time to investigate how to support
multi-buffer in this scenario. I finally come to conclusion that
change the behavior of __xskq_cons_read_desc_batch() should be fine:
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index c3b97c6f2910..c8b7079a6728 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -270,6 +270,7 @@ __xskq_cons_read_desc_batch(struct xsk_queue *q,
struct xsk_buff_pool *pool,
{
u32 cached_cons = q->cached_cons, nb_entries = 0;
u32 total_descs = 0, nr_frags = 0;
+ bool copy = !!nb_pkts;

/* track first entry, if stumble upon *any* invalid descriptor, rewind
* current packet that consists of frags and stop the processing
@@ -288,7 +289,7 @@ __xskq_cons_read_desc_batch(struct xsk_queue *q,
struct xsk_buff_pool *pool,
if (likely(!parsed.mb)) {
total_descs += (nr_frags + 1);
nr_frags = 0;
- if (nb_pkts)
+ if (copy)
(*nb_pkts)++;
} else {
nr_frags++;
@@ -300,10 +301,11 @@ __xskq_cons_read_desc_batch(struct xsk_queue *q,
struct xsk_buff_pool *pool,
nb_entries++;
}

- cached_cons -= nr_frags;
+ if (!copy)
+ cached_cons -= nr_frags;
/* Release valid plus any invalid entries */
xskq_cons_release_n(q, cached_cons - q->cached_cons);
- return total_descs;
+ return copy ? nb_entries : total_descs;
}

static inline u32

After this, this function shall return the correct descriptors.

>
> > + 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.

Right, I should avoid that under the condition where the nb_pkts is zero :
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a61477d04c14..e04eda59eb7a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -671,7 +671,7 @@ int xsk_alloc_batch_skb(struct xdp_sock *xs, u32
nb_pkts, u32 nb_descs, int *err
if (!(dev->priv_flags & IFF_TX_SKB_NO_LINEAR))
base_len += dev->needed_tailroom;

- if (xs->skb)
+ if (xs->skb && nb_pkts > 0)
nb_pkts--;

if (skb_count >= nb_pkts)

>
> > + /* 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?

Right, I mentioned in the previous patch that we can handle this issue
in patch 2 to bypass this part in batch mode to solve the problem.

>
> [ ... ]
> > + 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?

Right. It will be fixed by:
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 8c115202fe42..c7c81ee81c00 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -1020,7 +1020,7 @@ static int __xsk_generic_xmit_batch(struct xdp_sock *xs)
err = -EAGAIN;

out:
- if (xs->skb)
+ if (xs->skb && err != -EAGAIN)
xsk_drop_skb(xs->skb);

Thanks,
Jason

>
> > +
> > + /* 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