xsk: batch xmit in copy mode

Sashiko AI review found 1 potential issue(s):
- [High] Unsafe array allocation using kmalloc for a potentially large, user-controlled size --

commit 16fdf26210767cd884154d2717d7894d96a2ee54
Author: Jason Xing <[email protected]>
xsk: introduce XDP_GENERIC_XMIT_BATCH setsockopt

This commit introduces the XDP_GENERIC_XMIT_BATCH setsockopt to allow batch processing in copy mode, attempting to improve throughput by allocating and initializing multiple skbs at one time.

Quoted 4 lines

net/xdp/xsk.c +0 -0

index 3b46bc635c432..1194268ae8676 100644

[ ... ]

Quoted 14 lines

net/xdp/xsk.c +10 -0

@@ -1582,6 +1593,42 @@ static int xsk_setsockopt(struct socket *sock, int level, int optname,
              WRITE_ONCE(xs->max_tx_budget, budget);
              return 0;
      }
+     case XDP_GENERIC_XMIT_BATCH:
+     {
+             struct xsk_buff_pool *pool = xs->pool;
+             struct xsk_batch *batch = &xs->batch;
+             struct xdp_desc *descs;
+             struct sk_buff **skbs;
+             unsigned int size;
+             int ret = 0;
+
+             if (optlen != sizeof(size))

net/xdp/xsk.c static int xsk_setsockopt(struct socket *sock, int level, int optname, · line 1615

Quoted 10 lines

net/xdp/xsk.c +10 -0

+                     return -EINVAL;
+             if (copy_from_sockptr(&size, optval, sizeof(size)))
+                     return -EFAULT;
+             if (size == batch->generic_xmit_batch)
+                     return 0;
+             if (!size || size > xs->max_tx_budget || !pool)
+                     return -EACCES;
+
+             mutex_lock(&xs->mutex);
+             skbs = kmalloc(size * sizeof(struct sk_buff *), GFP_KERNEL);

Can the requested size here become large enough to trigger high-order allocation failures under memory fragmentation?

Since size is bounded by xs->max_tx_budget, which follows the TX ring entries, it looks like this could reach up to 65536 on some configurations. A 512KB contiguous allocation via kmalloc might fail on long-running systems and trigger page allocation warnings in the kernel log.

Also, since size is a user-provided value bounded only by the maximum budget, could the open-coded multiplication overflow on 32-bit platforms?

Would it be safer to use kvmalloc_array() here to prevent both integer overflows and the dependency on physically contiguous memory, similar to how kvcalloc() is used for the descs array right below it?

Sashiko AI review found 1 potential issue(s):
- [High] Inconsistent ownership semantics of allocated_skb on error paths lead to a memory leak in the zero-copy path. --

commit a6016c1c59927d7e151f38b7f237ad699db86b1e
Author: Jason Xing <[email protected]>

xsk: extend xsk_build_skb() to support passing an already allocated skb

This patch extends xsk_build_skb() to accept an allocated_skb parameter, allowing the batch feature to reuse an already allocated skb instead of allocating a new one.

Quoted 4 lines

net/xdp/xsk.c +0 -0

index 1194268ae8676..becc8ebfd82f8 100644

[ ... ]

Quoted 20 lines

net/xdp/xsk.c +4 -3

@@ -793,15 +797,16 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
      return skb;
 }
 
-static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
-                                  struct xdp_desc *desc)
+struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
+                           struct sk_buff *allocated_skb,
+                           struct xdp_desc *desc)
 {
      struct net_device *dev = xs->dev;
      struct sk_buff *skb = xs->skb;
      int err;
 
      if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
-             skb = xsk_build_skb_zerocopy(xs, desc);
+             skb = xsk_build_skb_zerocopy(xs, allocated_skb, desc);
              if (IS_ERR(skb)) {
                      err = PTR_ERR(skb);
                      skb = NULL;

Quoted 1 line

                      goto free_err;

Does this leak allocated_skb on the zero-copy error path?

If xsk_build_skb_zerocopy() fails on the first descriptor (e.g., if xsk_skb_metadata() returns an error due to invalid metadata), it returns an ERR_PTR without freeing the SKB.

Because skb is then explicitly set to NULL before jumping to free_err, kfree_skb() is skipped.

While the original code also had a leak here for internally allocated SKBs,

this patch extends the issue to caller-provided SKBs. In copy mode, xsk_build_skb() correctly frees allocated_skb on error, but the zero-copy path does not.

Could this lead to memory exhaustion if a caller assumes xsk_build_skb() always consumes or frees the provided SKB on error?

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Sashiko AI review found 2 potential issue(s):
- [High] Pre-allocated SKB and socket memory charge (sk_wmem_alloc) are permanently leaked when zerocopy metadata processing fails.
- [High] Double-free and Use-After-Free of xs->skb when fragment allocation fails with -EAGAIN in copy mode. --

commit 3ef2c03b6204759327ab337fe34617d9dd8e04ef
Author: Jason Xing <[email protected]>

xsk: add xsk_alloc_batch_skb() to build skbs in batch

This commit introduces xsk_alloc_batch_skb() to support allocating and building skbs in batch. It reserves skbs, batches the sk_wmem_alloc charge, and then copies data to complete initialization.

Quoted 3 lines

net/core/skbuff.c +0 -0

[ ... ]

net/core/skbuff.c static void *kmalloc_reserve(unsigned int *size, gfp_t flags, int node, · line 750

Quoted 12 lines

net/core/skbuff.c +12 -0

+     /* Phase 3: Build SKBs with packet data */
+     for (j = 0; j < alloc_descs; j++) {
+             if (!xs->skb) {
+                     skb = skbs[skb_count - 1 - k];
+                     k++;
+             }
+
+             skb = xsk_build_skb(xs, skb, &descs[j]);
+             if (IS_ERR(skb)) {
+                     *err = PTR_ERR(skb);
+                     break;
+             }

Can this lead to a double-free and use-after-free of xs->skb if a fragment allocation fails?

If xsk_build_skb() fails to allocate a page for a subsequent fragment in copy mode, it frees the partially built skb and returns -EAGAIN. However, it leaves xs->skb pointing to this freed memory.

When this loop breaks and returns -EAGAIN up the call chain, the caller __xsk_generic_xmit_batch() checks xs->skb and calls xsk_drop_skb(xs->skb), which would double-free the skb. Furthermore, any subsequent transmissions could dereference the dangling pointer. Should xs->skb be explicitly cleared when handling this error?

Quoted 4 lines

net/core/skbuff.c +3 -0

+
+             if (xp_mb_desc(&descs[j])) {
+                     xs->skb = skb;

net/core/skbuff.c static void *kmalloc_reserve(unsigned int *size, gfp_t flags, int node, · line 763

Quoted 10 lines

net/core/skbuff.c +10 -0

+                     continue;
+             }
+
+             xs->skb = NULL;
+             __skb_queue_tail(&batch->send_queue, skb);
+     }
+
+     /* Phase 4: Reclaim unused allocated SKBs */
+     while (k < i)
+             kfree_skb(skbs[skb_count - 1 - k++]);

Does this permanently leak the pre-allocated skb and its socket memory charge if xsk_build_skb() fails?

When xsk_build_skb() encounters an error like a zerocopy metadata processing failure, it returns an ERR_PTR() without freeing the skb. Because k was already incremented before the xsk_build_skb() call, the Phase 4 loop starts reclaiming at the next skb.

The failed skb at index k - 1 is bypassed. Since it is never freed, sock_wfree is skipped, leaking both the skb and the batched sk_wmem_alloc charge.

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

Sashiko AI review found 2 potential issue(s):
- [High] Cached data buffers are consumed without validating current allocation flags, potentially violating memory placement constraints.
- [Medium] Meaningful code and commit message mismatch regarding cache refill logic.
--

commit 5a442e9c44f0a64a147908843084cffe5e2fed9f
Author: Jason Xing <[email protected]>

xsk: cache data buffers to avoid frequently calling kmalloc_reserve

This commit introduces a per-socket data buffer cache for small packets to reduce kmalloc_reserve() overhead. It adds bulk allocation from skb_small_head_cache and persists unused buffers across batch cycles.

Quoted 6 lines

Inside the Phase-1 per-descriptor loop, when a small packet needs a data buffer and the cache is empty, a single kmem_cache_alloc_bulk() refills it with generic_xmit_batch objects. Subsequent small packets pop directly from the cache. Large packets bypass the cache entirely and fall back to kmalloc_reserve(). Unused buffers remain in the cache for the next batch.

Is this description accurate? Looking at the implementation below, the bulk allocation happens outside the Phase-1 loop. If the cache runs empty during the loop, it appears to fall back to kmalloc_reserve() for each subsequent packet rather than refilling the cache via kmem_cache_alloc_bulk(). Could the commit message be updated to match the actual execution flow?

Quoted 5 lines

net/core/skbuff.c +0 -0

index c7e42c2966cdd..3b3969adfd23f 100644

[ ... ]

net/core/skbuff.c int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err · line 722

Quoted 10 lines

net/core/skbuff.c +2 -4

@@ -709,10 +718,17 @@ int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err
 
                      skb = skbs[skb_count - 1 - i];
                      skbuff_clear(skb);
-                     data = kmalloc_reserve(&size, gfp_mask, node, skb);
-                     if (unlikely(!data)) {
-                             *err = -ENOBUFS;
-                             break;
+                     if (dc_count &&
+                         SKB_HEAD_ALIGN(size) <= SKB_SMALL_HEAD_CACHE_SIZE) {

If the socket's sk_allocation (which populates gfp_mask) was modified to require restricted memory placement (e.g. __GFP_DMA), could this code use a standard memory buffer left over from a previous batch?

While the bulk allocation check before the loop correctly avoids allocating from skb_small_head_cache if KMALLOC_NOT_NORMAL_BITS is set, the consumption path here only checks dc_count. Does this need to validate gfp_mask against KMALLOC_NOT_NORMAL_BITS before consuming from the cache, to ensure memory placement constraints are not bypassed?

net/core/skbuff.c int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err · line 733

Quoted 11 lines

net/core/skbuff.c +9 -0

+                             data = dc[--dc_count];
+                             size = SKB_SMALL_HEAD_CACHE_SIZE;
+                     } else {
+                             data = kmalloc_reserve(&size, gfp_mask,
+                                                    node, skb);
+                             if (unlikely(!data)) {
+                                     *err = -ENOBUFS;
+                                     break;
+                             }
                      }

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

Quoted 22 lines · On Wed, Apr 15, 2026 at 5:38 PM <[email protected]> wrote:

Sashiko AI review found 2 potential issue(s):
- [High] Cached data buffers are consumed without validating current allocation flags, potentially violating memory placement constraints.
- [Medium] Meaningful code and commit message mismatch regarding cache refill logic.
--

commit 5a442e9c44f0a64a147908843084cffe5e2fed9f
Author: Jason Xing <[email protected]>

xsk: cache data buffers to avoid frequently calling kmalloc_reserve

This commit introduces a per-socket data buffer cache for small packets to reduce kmalloc_reserve() overhead. It adds bulk allocation from skb_small_head_cache and persists unused buffers across batch cycles.

Quoted 6 lines

Inside the Phase-1 per-descriptor loop, when a small packet needs a data buffer and the cache is empty, a single kmem_cache_alloc_bulk() refills it with generic_xmit_batch objects. Subsequent small packets pop directly from the cache. Large packets bypass the cache entirely and fall back to kmalloc_reserve(). Unused buffers remain in the cache for the next batch.

Quoted 5 lines

Is this description accurate? Looking at the implementation below, the bulk allocation happens outside the Phase-1 loop. If the cache runs empty during the loop, it appears to fall back to kmalloc_reserve() for each subsequent packet rather than refilling the cache via kmem_cache_alloc_bulk(). Could the commit message be updated to match the actual execution flow?

Will update it.

Quoted 21 lines

Quoted 5 lines

net/core/skbuff.c +0 -0

index c7e42c2966cdd..3b3969adfd23f 100644

[ ... ]

net/core/skbuff.c int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err · line 722

Quoted 10 lines

net/core/skbuff.c +2 -4

@@ -709,10 +718,17 @@ int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err
 
                      skb = skbs[skb_count - 1 - i];
                      skbuff_clear(skb);
-                     data = kmalloc_reserve(&size, gfp_mask, node, skb);
-                     if (unlikely(!data)) {
-                             *err = -ENOBUFS;
-                             break;
+                     if (dc_count &&
+                         SKB_HEAD_ALIGN(size) <= SKB_SMALL_HEAD_CACHE_SIZE) {

If the socket's sk_allocation (which populates gfp_mask) was modified to require restricted memory placement (e.g. __GFP_DMA), could this code use a standard memory buffer left over from a previous batch?

Quoted 5 lines

While the bulk allocation check before the loop correctly avoids allocating from skb_small_head_cache if KMALLOC_NOT_NORMAL_BITS is set, the consumption path here only checks dc_count. Does this need to validate gfp_mask against KMALLOC_NOT_NORMAL_BITS before consuming from the cache, to ensure memory placement constraints are not bypassed?

I can add the check but it seems meaningless to me. In the whole implementation of xsk, there is no place where sk_allocation can be set to a different value other than GPF_KERNEL.

Sashiko AI review found 4 potential issue(s):
- [High] Packet validation executed inside hardware transmit lock, causing severe lock contention
- [High] Missing transmit recursion protection introduces deadlock vectors
- [High] Missing device state validation allows transmission to stopped or suspended devices
- [Medium] Hardware doorbell batching is defeated by hardcoded 'more' parameter --

commit b98984346f64d24e3ecd521b219464ccbe9fba91
Author: Jason Xing <[email protected]>

xsk: add direct xmit in batch function

This commit adds a batch transmission logic for AF_XDP. It introduces a new direct transmit function that disables bottom halves, grabs the lock once, and sends all aggregated packets in a loop.

Quoted 3 lines

net/core/dev.c +0 -0

[ ... ]

net/core/dev.c int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) · line 4904

Quoted 8 lines

net/core/dev.c +8 -0

+int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev)
+{
+     u16 queue_id = xs->queue_id;
+     struct netdev_queue *txq = netdev_get_tx_queue(dev, queue_id);
+     struct sk_buff_head *send_queue = &xs->batch.send_queue;
+     int ret = NETDEV_TX_BUSY;
+     struct sk_buff *skb;
+

Should there be a check for the device state before attempting to transmit? Direct transmit paths that bypass the Qdisc layer, like __dev_direct_xmit(), typically verify that the device is running and the carrier is up:

        if (unlikely(!netif_running(dev) ||
                     !netif_carrier_ok(dev)))
                goto drop;

Could invoking transmission on a stopped or suspended device cause issues?

Quoted 1 line

+ local_bh_disable();

Are we missing transmit recursion protection here? Standard paths wrap the hardware transmit lock with recursion tracking to protect against stack overflows and deadlocks when routing through virtual devices:

        dev_xmit_recursion_inc();
        HARD_TX_LOCK(dev, txq, smp_processor_id());

net/core/dev.c int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) · line 4911

Quoted 7 lines

net/core/dev.c +6 -0

+     HARD_TX_LOCK(dev, txq, smp_processor_id());
+     while ((skb = __skb_dequeue(send_queue)) != NULL) {
+             struct sk_buff *orig_skb = skb;
+             bool again = false;
+
+             skb = validate_xmit_skb_list(skb, dev, &again);

Will executing validate_xmit_skb_list() inside the hardware transmit lock cause lock contention? This function can perform computationally expensive operations such as GSO segmentation, software checksum fallbacks, and subsystem hooks. Other transmit paths execute this validation strictly before acquiring the hardware lock.

Quoted 4 lines

net/core/dev.c +3 -0

+             if (skb != orig_skb) {
+                     dev_core_stats_tx_dropped_inc(dev);
+                     kfree_skb_list(skb);

net/core/dev.c int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) · line 4924

Quoted 10 lines

net/core/dev.c +10 -0

+                     ret = NET_XMIT_DROP;
+                     break;
+             }
+
+             if (netif_xmit_frozen_or_drv_stopped(txq)) {
+                     __skb_queue_head(send_queue, skb);
+                     break;
+             }
+             skb_set_queue_mapping(skb, queue_id);
+             ret = netdev_start_xmit(skb, dev, txq, false);

Does passing false for the more parameter defeat the purpose of hardware doorbell batching? It instructs the driver to ring the hardware doorbell for every single packet instead of delaying it until the end of the batch.

It looks like this might be fixed later in the patch series by commit 6d7c8f2ad056e ("xsk: support dynamic xmit.more control for batch xmit").

net/core/dev.c int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) · line 4931

Quoted 8 lines

net/core/dev.c +7 -0

+             if (ret != NETDEV_TX_OK) {
+                     if (ret == NETDEV_TX_BUSY)
+                             __skb_queue_head(send_queue, skb);
+                     break;
+             }
+     }
+     HARD_TX_UNLOCK(dev, txq);

Also, we would need to pair any added recursion increment with a decrement here:

        dev_xmit_recursion_dec();

net/core/dev.c int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) · line 4935

Quoted 4 lines

net/core/dev.c +4 -0

+     local_bh_enable();
+
+     return ret;
+}

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

Sashiko AI review found 1 potential issue(s):
- [High] Stalled TX queue due to missing hardware doorbell ring on early loop exit --

commit 6d7c8f2ad056eb4cb2eec8d360e632f9ef9044f5
Author: Jason Xing <[email protected]>

xsk: support dynamic xmit.more control for batch xmit

This commit updates xsk batch transmission to only set xmit.more to false for the last skb in the send queue, aiming to reduce unnecessary IRQs and improve batching performance.

Quoted 11 lines

net/core/dev.c +1 -0

@@ -4873,6 +4873,7 @@ int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev)
      struct sk_buff_head *send_queue = &xs->batch.send_queue;
      int ret = NETDEV_TX_BUSY;
      struct sk_buff *skb;
+     bool more = true;
 
      local_bh_disable();
      HARD_TX_LOCK(dev, txq, smp_processor_id());

[ ... ]

Quoted 14 lines

net/core/dev.c +5 -1

@@ -4892,8 +4893,12 @@ int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev)
                      __skb_queue_head(send_queue, skb);
                      break;
              }
+
+             if (!skb_peek(send_queue))
+                     more = false;
+
              skb_set_queue_mapping(skb, queue_id);
-             ret = netdev_start_xmit(skb, dev, txq, false);
+             ret = netdev_start_xmit(skb, dev, txq, more);
              if (ret != NETDEV_TX_OK) {
                      if (ret == NETDEV_TX_BUSY)
                              __skb_queue_head(send_queue, skb);

Quoted 3 lines

                      break;
              }
      }

If an error occurs and the loop breaks early, does this leave previously transmitted packets stalled in the driver's TX ring?

Since more is passed as true for all packets prior to the failure, the network driver defers ringing the hardware TX doorbell.

If the code breaks out of the loop due to validate_xmit_skb_list() failing, netif_xmit_frozen_or_drv_stopped(), or netdev_start_xmit() returning NETDEV_TX_BUSY, the loop terminates without ever passing more = false to the driver.

Could this omission prevent the hardware from being notified to transmit the successfully processed packets that preceded the error?

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

Quoted 3 lines · On Wed, Apr 15, 2026 at 5:35 PM <[email protected]> wrote:

Sashiko AI review found 1 potential issue(s):
- [High] Stalled TX queue due to missing hardware doorbell ring on early loop exit

Quoted 4 lines · Jason Xing

Quoted 3 lines · On Wed, Apr 15, 2026 at 5:35 PM <[email protected]> wrote:

Sashiko AI review found 1 potential issue(s):
- [High] Stalled TX queue due to missing hardware doorbell ring on early loop exit

Does sahshiko bot have reply-to set to "[email protected]" by chance? And can it be changed? I am subscribed here for discussions and news about sashiko itself, not for replies from all different kernel subsystems to reviews of their patches...

Thanks.

Sashiko AI review found 2 potential issue(s):
- [Critical] Bypassing validate_xmit_skb_list() based on NETIF_F_SG skips critical hardware capability checks, software fallbacks, and SKB initialization, leading to kernel panics and corrupted packets.
- [High] Breaking out of the transmit loop early leaves packets stuck in the TX ring indefinitely because the hardware doorbell is never flushed.
--

commit 7603ac608a36b8d39e026a60b567ea2b106617f0
Author: Jason Xing <[email protected]>

xsk: try to skip validating skb list in xmit path

This patch attempts to optimize the XSK transmit path by skipping validate_xmit_skb_list() if the device supports Scatter-Gather, aiming to improve performance for drivers like ixgbe.

Quoted 4 lines

net/core/dev.c +0 -0

index 99188353abec6..12643cbab6530 100644

[ ... ]

Quoted 8 lines

net/core/dev.c +0 -4

@@ -4878,15 +4879,17 @@ int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev)
      local_bh_disable();
      HARD_TX_LOCK(dev, txq, smp_processor_id());
      while ((skb = __skb_dequeue(send_queue)) != NULL) {
-             struct sk_buff *orig_skb = skb;
-             bool again = false;
-
-             skb = validate_xmit_skb_list(skb, dev, &again);

net/core/dev.c int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev) · line 4886

Quoted 10 lines

net/core/dev.c +5 -5

-             if (skb != orig_skb) {
-                     dev_core_stats_tx_dropped_inc(dev);
-                     kfree_skb_list(skb);
-                     ret = NET_XMIT_DROP;
-                     break;
+             if (unlikely(need_validate)) {
+                     struct sk_buff *orig_skb = skb;
+                     bool again = false;
+
+                     skb = validate_xmit_skb_list(skb, dev, &again);

Does skipping validate_xmit_skb_list() based on NETIF_F_SG bypass necessary hardware capability checks and software fallbacks?

validate_xmit_skb() performs dynamic feature verification via ndo_features_check(), transport header initialization via skb_set_transport_header(), and software checksum fallbacks via skb_csum_hwoffload_help().

AF_XDP users can request TX checksum offload, setting skb->ip_summed to CHECKSUM_PARTIAL. By skipping validation solely because the device supports Scatter-Gather, packets bypass skb_set_transport_header(), leaving skb->transport_header uninitialized.

Could this cause drivers that use skb_transport_offset() to compute massive sizes, corrupting TX descriptors and potentially hanging the NIC?

Additionally, if the device lacks NETIF_F_HW_CSUM, skipping skb_csum_hwoffload_help() sends CHECKSUM_PARTIAL to drivers unable to handle it. Might this result in silent data corruption on the wire or driver crashes?

net/core/dev.c int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev) · line 4919

Quoted 6 lines

net/core/dev.c +5 -0

+                     if (skb != orig_skb) {
+                             dev_core_stats_tx_dropped_inc(dev);
+                             kfree_skb_list(skb);
+                             ret = NET_XMIT_DROP;
+                             break;

If this loop breaks early, does it leave packets stuck in the TX ring?

While this behavior was introduced earlier in the series and not by this specific patch, the issue is present in this modified block.

The batching logic calls netdev_start_xmit() with more = true for all packets except the last one in the queue.

If the loop breaks early here due to a validate_xmit_skb_list() failure, the function returns without ever calling netdev_start_xmit() with more = false.

Will this cause the driver to defer ringing the hardware TX doorbell, leaving previously submitted packets stranded in the TX ring and causing latency spikes or TX watchdog timeouts?

net/core/dev.c int xsk_direct_xmit_batch(struct xdp_sock *xs, struct net_device *dev) · line 4923

Quoted 5 lines

net/core/dev.c +1 -0

+                     }
              }
 
              if (netif_xmit_frozen_or_drv_stopped(txq)) {

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

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.

Quoted 4 lines

net/xdp/xsk.c +0 -0

index b4f3cc7125804..16e47f6fc30ec 100644

[ ... ]

Quoted 8 lines

net/xdp/xsk.c +4 -0

@@ -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;

net/xdp/xsk.c struct sk_buff *xsk_build_skb(struct xdp_sock *xs, · line 930

Quoted 10 lines

net/xdp/xsk.c +10 -0

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

Quoted 4 lines

net/xdp/xsk.c +3 -0

+
+     /* Since we dropped the RCU read lock, the socket state might have changed. */
+     if (unlikely(!xsk_is_bound(xs))) {

[ ... ]

Quoted 3 lines

net/xdp/xsk.c +3 -0

+     for (i = 0; i < max_budget; i += cons_descs) {
+             u32 nb_pkts = 0;
+             u32 nb_descs;

[ ... ]

net/xdp/xsk.c

Quoted 11 lines

net/xdp/xsk.c +11 -0

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

net/xdp/xsk.c

Quoted 5 lines

net/xdp/xsk.c +4 -0

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

net/xdp/xsk.c

Quoted 8 lines

net/xdp/xsk.c +7 -0

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

[ ... ]

net/xdp/xsk.c

Quoted 11 lines

net/xdp/xsk.c +11 -0

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

net/xdp/xsk.c

Quoted 6 lines

net/xdp/xsk.c +5 -0

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

Sashiko AI review found 1 potential issue(s):
- [High] The patch groups NAPI RX write-heavy fields and NAPI TX write-heavy fields into the exact same cacheline, introducing severe false sharing between RX and TX CPUs. --

commit cf39ff4311089098b2e57c37ba6e85e7a36ac4a8
Author: Jason Xing <[email protected]>

xsk: separate read-mostly and write-heavy fields in xsk_buff_pool

This commit separates read-mostly pool geometry fields from write-heavy fields in struct xsk_buff_pool by adding cacheline alignment. This is intended to eliminate false sharing between the sendmsg CPU and the NAPI TX-completion CPU.

Quoted 4 lines

include/net/xsk_buff_pool.h +0 -0

index 0b1abdb99c9ea..0376bd4cc95b0 100644

[ ... ]

Quoted 1 line

include/net/xsk_buff_pool.h +0 -0

@@ -73,23 +73,27 @@ struct xsk_buff_pool {

[ ... ]

include/net/xsk_buff_pool.h struct xsk_buff_pool { · line 98

Quoted 13 lines

      void *addrs;

include/net/xsk_buff_pool.h +5 -1

+
+     /* Write-heavy fields */
      /* Mutual exclusion of the completion ring in the SKB mode.
       * Protect: NAPI TX thread and sendmsg error paths in the SKB
       * destructor callback.
       */
-     spinlock_t cq_prod_lock;
+     spinlock_t cq_prod_lock ____cacheline_aligned_in_smp;
+     u8 cached_need_wakeup;
+     u32 free_heads_cnt;
      struct xdp_buff_xsk *free_heads[];
 };

Could placing these four fields together in the same aligned cacheline introduce new false sharing between the NAPI RX and TX CPUs?

The fields cq_prod_lock and cached_need_wakeup are updated by the TX completion path.

However, free_heads_cnt and the free_heads flexible array are heavily mutated by the RX path during buffer allocation and release.

Because cq_prod_lock (spinlock_t) and cached_need_wakeup (u8) only consume a fraction of the 64-byte cacheline, free_heads_cnt and the first several elements of the free_heads array will pack into this exact same cacheline.

If a deployment pins RX and TX interrupts to different CPU cores, it looks like this might cause continuous cacheline bouncing between the RX CPU and the TX CPU for every packet processed.

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12

Sashiko AI review found 3 potential issue(s):
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) Use-After-Free (UAF) vulnerability on batch->desc_cache.
- [High] Leaked socket structure on error path in xsk_create().
- [High] Unbounded kmalloc size for user-controlled batch size triggers WARN_ON and potential panic. --

commit 5f2cebbff2efe715b882067f337741749751ac65
Author: Jason Xing <[email protected]>

xsk: retire old xmit path in copy mode

This commit adds a new helper xsk_init_batch() with a default value of 1, and obsoletes __xsk_generic_xmit() by substituting it with __xsk_generic_xmit_batch() in the generic transmit path.

Quoted 4 lines

net/xdp/xsk.c +0 -0

index 16e47f6fc30ec..232e7bf96b342 100644

[ ... ]

Quoted 4 lines

net/xdp/xsk.c +0 -0

@@ -1030,101 +1030,14 @@ static int __xsk_generic_xmit_batch(struct xdp_sock *xs)
      return err;
 }
 

[ ... ]

net/xdp/xsk.c static int __xsk_generic_xmit_batch(struct xdp_sock *xs) · line 1046

Quoted 9 lines

static int xsk_generic_xmit(struct sock *sk)
{

net/xdp/xsk.c +2 -1

+     struct xdp_sock *xs = xdp_sk(sk);
      int ret;
 
      /* Drop the RCU lock since the SKB path might sleep. */
      rcu_read_unlock();
-     ret = __xsk_generic_xmit(sk);
+     ret = __xsk_generic_xmit_batch(xs);

By switching to __xsk_generic_xmit_batch(), does this expose a use-after-free race with xsk_setsockopt()?

In __xsk_generic_xmit_batch(), the local pointer descs is assigned batch->desc_cache before acquiring xs->mutex.

Concurrently, a user can call setsockopt(..., XDP_GENERIC_XMIT_BATCH), which acquires xs->mutex and updates the batch size via xsk_init_batch(). This safely frees the old desc_cache array using kvfree() in xsk_batch_reset() because the setsockopt path does not check for xs->state != XSK_READY.

Once setsockopt drops the mutex, the blocked __xsk_generic_xmit_batch() thread acquires the mutex and proceeds to use its stale descs pointer. Can this lead to a use-after-free write into the freed descriptor array?

Quoted 3 lines

      /* Reaquire RCU lock before going into common code. */
      rcu_read_lock();

[ ... ]

net/xdp/xsk.c struct xdp_umem_reg_v1 { · line 1542

Quoted 11 lines

net/xdp/xsk.c +7 -0

@@ -1620,6 +1533,34 @@ struct xdp_umem_reg_v1 {
      __u32 headroom;
 };
 
+static int xsk_init_batch(struct xsk_batch *batch, unsigned int size)
+{
+     struct xdp_desc *descs;
+     struct sk_buff **skbs;
+     void **data;
+
+     skbs = kmalloc(size * sizeof(struct sk_buff *), GFP_KERNEL);

Can this trigger a WARN_ON_ONCE in the page allocator if a user requests a very large batch size?

The size parameter is controlled by userspace via the XDP_GENERIC_XMIT_BATCH setsockopt and is bounded by the TX ring size (xs->max_tx_budget), which can be quite large.

Since these pointer arrays can exceed KMALLOC_MAX_SIZE, should this be allocated using kvmalloc_array() similar to how the descs array is allocated below?

Quoted 11 lines

net/xdp/xsk.c +10 -0

+     if (!skbs)
+             return -ENOMEM;
+
+     data = kmalloc_array(size, sizeof(void *), GFP_KERNEL);
+     if (!data) {
+             kfree(skbs);
+             return -ENOMEM;
+     }
+
+     descs = kvcalloc(size, sizeof(struct xdp_desc), GFP_KERNEL);

[ ... ]

net/xdp/xsk.c static int xsk_create(struct net *net, struct socket *sock, int protocol, · line 1991

Quoted 9 lines

net/xdp/xsk.c +5 -0

@@ -2065,6 +1984,11 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
      if (!sk)
              return -ENOBUFS;
 
+     xs = xdp_sk(sk);
+     ret = xsk_init_batch(&xs->batch, 1);
+     if (ret)
+             return ret;
+

Does this leak the sk object if xsk_init_batch() fails?

If xsk_init_batch() returns an error here, the function returns early without calling sk_free(sk).

Because sock_init_data() has not been executed yet to link the sk to the sock structure, the VFS socket layer will not free the socket memory when tearing down the failed creation. Should this path clean up the allocated socket before returning?

net/xdp/xsk.c static int xsk_create(struct net *net, struct socket *sock, int protocol, · line 2000

Quoted 3 lines

net/xdp/xsk.c +0 -0

      sock->ops = &xsk_proto_ops;
 
      sock_init_data(sock, sk);

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13

Sashiko AI review found 2 potential issue(s):
- [High] An SKB memory leak occurs when xsk_build_skb() returns an error for a newly assigned SKB.
- [Medium] The prefetch instruction uses the raw AF_XDP descriptor address without extracting the offset, creating an invalid pointer in unaligned mode. --

commit 7bdeef673e571c57c404cb8d05446f46b3e2676e
Author: Jason Xing <[email protected]>

xsk: optimize xsk_build_skb for batch copy-mode fast path

This commit introduces three targeted optimizations for the batch copy-mode TX hot path in XDP sockets, including replacing skb_store_bits() with memcpy(), inlining UMEM address computation, and prefetching the next descriptor's UMEM data.

Quoted 14 lines

net/core/skbuff.c +5 -1

index 3b3969adfd23f..0fccb0c8cbe90 100644
@@ -751,14 +751,28 @@ int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err
      if (total_truesize)
              refcount_add(total_truesize, &xs->sk.sk_wmem_alloc);
 
-     /* Phase 3: Build SKBs with packet data */
+     /* Phase 3: Build SKBs with packet data. */
+     struct xsk_buff_pool *pool = xs->pool;
+     void *pool_addrs = pool->addrs;
+     bool unaligned = pool->unaligned;
+

net/core/skbuff.c int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err · line 768

Quoted 10 lines

net/core/skbuff.c +9 -0

      for (j = 0; j < alloc_descs; j++) {
+             u64 addr = descs[j].addr;
+             void *buffer;
+
+             if (unaligned)
+                     addr = xp_unaligned_add_offset_to_addr(addr);
+             buffer = pool_addrs + addr;
+
+             if (j + 1 < alloc_descs)
+                     prefetch(pool_addrs + descs[j + 1].addr);

Does this prefetch use the raw AF_XDP descriptor address without extracting the offset?

In unaligned mode, an AF_XDP descriptor addr field encodes a chunk offset in its upper 16 bits. While xp_unaligned_add_offset_to_addr() is used to decode the current descriptor address, the prefetch directly adds descs[j + 1].addr to pool_addrs.

Could this result in an out-of-bounds pointer in unaligned mode, failing to hide memory latency and causing cache pollution?

Quoted 7 lines

net/core/skbuff.c +1 -0

+
              if (!xs->skb) {
                      skb = skbs[skb_count - 1 - k];
                      k++;
              }
 

net/core/skbuff.c int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err · line 779

Quoted 5 lines

net/core/skbuff.c +1 -1

-             skb = xsk_build_skb(xs, skb, &descs[j]);
+             skb = xsk_build_skb(xs, skb, &descs[j], buffer);
              if (IS_ERR(skb)) {
                      *err = PTR_ERR(skb);
                      break;

Can this leak the newly assigned SKB if xsk_build_skb() returns an error?

If xsk_build_skb() fails, it returns ERR_PTR(err) and the loop breaks. Since k was already incremented, the cleanup loop in Phase 4 (while (k < i) kfree_skb(...)) reclaims unused SKBs starting from the current k, skipping the failed SKB at index k - 1.

Could a malicious userspace program exploit this to systematically leak SKBs by triggering metadata errors?

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14