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

On Wed, Apr 15, 2026 at 5:38 PM <[email protected]> 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 <[email protected]>
> 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/[email protected]?part=11