xsk: fix bugs around xsk skb allocation

Sashiko AI review found 2 potential issue(s):
- [High] Premature SKB free triggers AF_XDP completion destructor, corrupting the Completion Queue (CQ) and causing an infinite TX loop.
- [High] Calling xsk_skb_metadata() before SKB fragments are attached breaks software checksum offload and permanently wedges the TX ring. --

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

xsk: avoid skb leak in XDP_TX_METADATA case

This commit prevents a memory leak when xsk_skb_metadata() fails by explicitly freeing the skb before returning to the caller.

Link https://lore.kernel.org/all/[email protected]/ open here

net/xdp/xsk.c static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, · line 825

Quoted 8 lines

net/xdp/xsk.c +0 -0

index 887abed254668..5b273bcd0dbce 100644
@@ -823,8 +823,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
              xsk_skb_init_misc(skb, xs, desc->addr);
              if (desc->options & XDP_TX_METADATA) {
                      err = xsk_skb_metadata(skb, buffer, desc, pool, hr);

Is it safe to call xsk_skb_metadata() here before the SKB fragments are attached?

Because the fragments are populated later in the function via skb_fill_page_desc(), both skb->len and skb_headlen(skb) are 0 at this point.

If XDP_TXMD_FLAGS_CHECKSUM is requested and software fallback is active, xsk_skb_metadata() will call skb_checksum_help(). Because skb_headlen is 0, the bounds check in skb_checksum_help() can trigger a WARN_ONCE and return -EINVAL. This permanently wedges the TX ring since -EINVAL does not consume the descriptor.

net/xdp/xsk.c static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, · line 747

Quoted 4 lines

net/xdp/xsk.c +2 -1

-                     if (unlikely(err))
+                     if (unlikely(err)) {
+                             kfree_skb(skb);

Does freeing the SKB here cause issues with the completion queue?

Just before this block, xsk_skb_init_misc() sets skb->destructor to xsk_destruct_skb. Calling kfree_skb() triggers this destructor, which submits the descriptor's address to the CQ and increments the producer index.

However, the caller xsk_build_skb() will see the error and call xsk_cq_cancel_locked(), which decrements the cached producer index. This desynchronizes the queue reservation pointers.

Additionally, because the error is not -EOVERFLOW, the kernel does not advance the TX ring consumer pointer. The bad descriptor remains in the TX ring and will be retried continuously, potentially causing an infinite loop.

Should skb->destructor be restored to sock_wfree before calling kfree_skb(), or should xsk_skb_init_misc() be deferred until after the metadata is validated?

net/xdp/xsk.c static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, · line 750

Quoted 4 lines

net/xdp/xsk.c +1 -0

                              return ERR_PTR(err);
+                     }
              }

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

From: Jason Xing <[email protected]>

Fix it by explicitly adding kfree_skb() before returning back to its caller.

How to reproduce it in virtio_net:
1. the current skb is the first one (which means no frag and xs->skb is NULL) and users enable metadata feature.
2. xsk_skb_metadata() returns a error code.
3. the caller xsk_build_skb() clears skb by using 'skb = NULL;'.
4. there is no chance to free this skb anymore.

Closes https://lore.kernel.org/all/[email protected]/ open here

Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")

Signed-off-by Jason Xing <[email protected]>

Quoted 12 lines · Jason Xing

From: Jason Xing <[email protected]>

Fix it by explicitly adding kfree_skb() before returning back to its caller.

How to reproduce it in virtio_net:
1. the current skb is the first one (which means no frag and xs->skb is NULL) and users enable metadata feature.
2. xsk_skb_metadata() returns a error code.
3. the caller xsk_build_skb() clears skb by using 'skb = NULL;'.
4. there is no chance to free this skb anymore.

Quoted 3 lines

Closes https://lore.kernel.org/all/[email protected]/ open here

Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")

Signed-off-by Jason Xing <[email protected]>

Acked-by Stanislav Fomichev <[email protected]>

Quoted 17 lines · Stanislav Fomichev

Quoted 15 lines · Jason Xing

From: Jason Xing <[email protected]>

Fix it by explicitly adding kfree_skb() before returning back to its caller.

How to reproduce it in virtio_net:
1. the current skb is the first one (which means no frag and xs->skb is NULL) and users enable metadata feature.
2. xsk_skb_metadata() returns a error code.
3. the caller xsk_build_skb() clears skb by using 'skb = NULL;'.
4. there is no chance to free this skb anymore.

Closes https://lore.kernel.org/all/[email protected]/ open here

Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")

Signed-off-by Jason Xing <[email protected]>

Quoted 1 line

Acked-by Stanislav Fomichev <[email protected]>

Actually, I take that back.. While looking at patch 2 (which always confuses me with that -EOVERFLOW handling), looks like we set skb->destructor in xsk_skb_init_misc? So this kfree will do xsk_cq_submit_addr_locked? Not sure that's what we want? Should we move xsk_skb_init_misc to happen after xsk_skb_metadata?

Quoted 20 lines · Stanislav Fomichev

Quoted 18 lines · Stanislav Fomichev

Quoted 12 lines · Jason Xing

From: Jason Xing <[email protected]>

Fix it by explicitly adding kfree_skb() before returning back to its caller.

How to reproduce it in virtio_net:
1. the current skb is the first one (which means no frag and xs->skb is NULL) and users enable metadata feature.
2. xsk_skb_metadata() returns a error code.
3. the caller xsk_build_skb() clears skb by using 'skb = NULL;'.
4. there is no chance to free this skb anymore.

Quoted 3 lines

Closes https://lore.kernel.org/all/[email protected]/ open here

Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")

Signed-off-by Jason Xing <[email protected]>

Acked-by Stanislav Fomichev <[email protected]>

Quoted 5 lines

Actually, I take that back.. While looking at patch 2 (which always confuses me with that -EOVERFLOW handling), looks like we set skb->destructor in xsk_skb_init_misc? So this kfree will do xsk_cq_submit_addr_locked? Not sure that's what we want? Should we move xsk_skb_init_misc to happen after xsk_skb_metadata?

Ooops, looks like you already addressed these in v2? Let me look into that..

Sashiko AI review found 2 potential issue(s):
- [High] The patch leaves a guaranteed NULL pointer dereference in the caller's error handling path when -EOVERFLOW is returned for the first fragment.
- [High] Calling kfree_skb(skb) directly triggers the packet destructor and incorrectly submits the dropped packet to the userspace Completion Queue (CQ). --

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

xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS

This commit explicitly frees the skb when the MAX_SKB_FRAGS limit is hit on the first descriptor to avoid a memory leak.

net/xdp/xsk.c static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, · line 863

Quoted 12 lines

net/xdp/xsk.c +3 -1

index 5b273bcd0dbce..3d6c6caa7ff82 100644
@@ -858,8 +858,11 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
      addr = buffer - pool->addrs;
 
      for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
-             if (unlikely(i >= MAX_SKB_FRAGS))
+             if (unlikely(i >= MAX_SKB_FRAGS)) {
+                     if (!xs->skb)
+                             kfree_skb(skb);

If we free the newly allocated skb here and return -EOVERFLOW, the caller xsk_build_skb() catches the error and jumps to free_err, where it calls:

    if (err == -EOVERFLOW) {
        /* Drop the packet */
        xsk_inc_num_desc(xs->skb);

Since xs->skb is still NULL, will this cause xsk_inc_num_desc() to dereference a NULL pointer?

Note that this regression appears to be fixed in a later commit 1c728000f884c ("xsk: handle NULL dereference of the skb without frags issue").

Additionally, does calling kfree_skb(skb) here trigger the packet destructor and incorrectly submit the dropped packet to the userspace completion queue?

Prior to entering the fragmentation loop, xsk_skb_init_misc() sets the destructor to xsk_destruct_skb(). When kfree_skb(skb) is called, it invokes this destructor, which sequentially calls xsk_cq_submit_addr_locked().

This writes the descriptor address to the completion queue and advances the user-visible q->ring->producer pointer, falsely notifying userspace of a successful transmission.

The caller xsk_build_skb() then executes its own error handling, calling xsk_cq_cancel_locked(), which only decrements the kernel's internal q->cached_prod counter.

Could this desynchronize the internal queue tracking variables from the shared memory ring pointers?

Should the destructor be changed back to sock_wfree before freeing it, similar to how xsk_consume_skb() handles it?

net/xdp/xsk.c static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, · line 785

Quoted 4 lines

net/xdp/xsk.c +1 -0

                      return ERR_PTR(-EOVERFLOW);
+             }
[ ... ]

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

From: Jason Xing <[email protected]>

Fix it by explicitly adding kfree_skb() before returning back to its caller.

How to reproduce it in virtio_net:
1. the current skb is the first one (which means xs->skb is NULL) and hit the limit MAX_SKB_FRAGS.
2. xsk_build_skb_zerocopy() returns -EOVERFLOW.
3. the caller xsk_build_skb() clears skb by using 'skb = NULL;'. This is why bug can be triggered.
4. there is no chance to free this skb anymore.

Note that if in this case the xs->skb is not NULL, xsk_build_skb() will call xsk_drop_skb(xs->skb) to do the right thing.

Fixes cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")

Signed-off-by Jason Xing <[email protected]>

---
 net/xdp/xsk.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

net/xdp/xsk.c +4 -1

index 8fcde34aec7b..5d3dbb118730 100644
@@ -778,8 +778,11 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
 	addr = buffer - pool->addrs;
 
 	for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
-		if (unlikely(i >= MAX_SKB_FRAGS))
+		if (unlikely(i >= MAX_SKB_FRAGS)) {
+			if (!xs->skb)
+				kfree_skb(skb);
 			return ERR_PTR(-EOVERFLOW);
+		}
 
 		page = pool->umem->pgs[addr >> PAGE_SHIFT];
 		get_page(page);

Signature · 1 line

2.41.3

Sashiko AI review found 2 potential issue(s):
- [Critical] Calling kfree_skb(skb) without clearing the SKB destructor causes Completion Queue (CQ) ring corruption and false completions.
- [High] Use-after-free of xs->skb in the continuation path when an error like -EAGAIN occurs.
--

commit 1c728000f884ce0a536d9c1fd763eb0c560ada51
Author: Jason Xing <[email protected]>

xsk: handle NULL dereference of the skb without frags issue

This commit addresses a NULL pointer dereference in the error path of xsk_build_skb_zerocopy() by checking if xs->skb is valid before dereferencing it during -EOVERFLOW handling. It also adds an alternative branch to handle bookkeeping for the first-descriptor case.

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

Quoted 6 lines

net/xdp/xsk.c +0 -0

index 3d6c6caa7ff82..2f00f188533c1 100644
@@ -978,9 +978,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
              kfree_skb(skb);

Can this lead to a use-after-free on xs->skb?

In the non-zerocopy path, if the first descriptor is linear, nr_frags remains 0 and the caller __xsk_generic_xmit() stores the SKB in xs->skb.

During the next iteration for a continuation descriptor, if an error like -EAGAIN occurs (e.g., from alloc_page()), the code jumps to the free_err label. The cleanup logic if (skb && !skb_shinfo(skb)->nr_frags) would evaluate to true, freeing the entire SKB.

The function then returns -EAGAIN, leaving the freed SKB pointer in xs->skb. On the next application retry, the code could dereference the freed xs->skb.

Note that this regression is fixed later in the patch series by commit 'xsk:
fix use-after-free of xs->skb in xsk_build_skb() free_err path'.

Also, could calling kfree_skb(skb) without clearing the SKB destructor cause Completion Queue (CQ) ring corruption and false completions?

In xsk_build_skb_zerocopy(), xsk_skb_init_misc() sets skb->destructor = xsk_destruct_skb. When an error occurs on the first descriptor (e.g., xsk_skb_metadata() returning -EINVAL), kfree_skb(skb) is called.

Because the destructor is not overridden with sock_wfree (like xsk_consume_skb() does), kfree_skb(skb) triggers xsk_destruct_skb(), which submits the descriptor to the CQ and increments q->ring->producer.

Quoted 16 lines

net/xdp/xsk.c +8 -3

      if (err == -EOVERFLOW) {
-             /* Drop the packet */
-             xsk_inc_num_desc(xs->skb);
-             xsk_drop_skb(xs->skb);
+             if (xs->skb) {
+                     /* Drop the packet */
+                     xsk_inc_num_desc(xs->skb);
+                     xsk_drop_skb(xs->skb);
+             } else {
+                     xsk_cq_cancel_locked(xs->pool, 1);
+                     xs->tx->invalid_descs++;
+             }
              xskq_cons_release(xs->tx);
      } else {
              /* Let application retry */
              xsk_cq_cancel_locked(xs->pool, 1);

Quoted 1 line

      }

The error handling paths then call xsk_cq_cancel_locked(), which decrements the reservation counter q->cached_prod.

Does decrementing the reservation while the actual producer pointer has advanced break queue accounting?

The kernel calculates available space using cached_prod; this artificial underflow could cause the queue to over-report free space, allowing the producer to silently overwrite unread user-space CQ entries.

Furthermore, in the -EINVAL path, does the invalid descriptor generate a false completion to user space without being released from the TX ring? When the application reuses the completed buffer, could the kernel retransmit the same descriptor on retry, causing a use-after-free and data corruption?

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

Sashiko AI review found 3 potential issue(s):
- [High] Calling kfree_skb(skb) in free_err after xsk_skb_init_misc() triggers premature CQ submission and corrupts Queue invariants.
- [High] Memory leak of xsk_addr when alloc_page() fails during multi-buffer packet processing.
- [High] On 32-bit architectures, xsk_skb_destructor_set_addr() truncates the 64-bit UMEM offset, corrupting the Completion Queue address. --

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

xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path

When xsk_build_skb() processes multi-buffer packets in copy mode, the first descriptor stores data into the skb linear area without adding any frags. If a continuation descriptor fails, the error path frees the skb while xs->skb still points to it. This patch adds a check to ensure we only free skbs that were freshly allocated in this call.

Link https://lore.kernel.org/all/[email protected]/ open here

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

Quoted 11 lines

net/xdp/xsk.c +1 -1

index 2f00f188533c1..470f6d25ddb93 100644
@@ -974,7 +974,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
      return skb;
 
 free_err:
-     if (skb && !skb_shinfo(skb)->nr_frags)
+     if (skb && !xs->skb && !skb_shinfo(skb)->nr_frags)
              kfree_skb(skb);

This isn't a regression introduced by this patch, but could calling kfree_skb() here trigger a premature completion queue submission?

If xsk_skb_metadata() fails earlier in the function, the skb destructor is already set to xsk_destruct_skb by xsk_skb_init_misc().

Freeing the skb here synchronously invokes xsk_destruct_skb(), which submits the completion to user space and increments the queue producer. The error path then executes xsk_cq_cancel_locked(), which decrements the cached producer.

Will this permanently desync the completion queue invariants and report a successful completion for a failed transmission? Would it be safer to clear the skb destructor before freeing it?

Also, is there a potential memory leak of the xsk_addr struct when alloc_page() fails during multi-buffer packet processing?

When processing a continuation descriptor, an xsk_addr struct is dynamically allocated and stored in skb_shinfo(skb)->destructor_arg with num_descs set to 1. If the subsequent alloc_page() fails, the code jumps here to free_err and returns -EAGAIN, leaving the partial packet in xs->skb for a retry.

If the socket is closed instead of retrying, xsk_drop_skb() is called on xs->skb, which calls xsk_consume_skb(). Inside xsk_consume_skb(), the cleanup logic uses this condition:

if (unlikely(num_descs > 1))

Since num_descs is exactly 1, the condition fails. Does this permanently leak the xsk_addr memory?

Finally, does xsk_skb_destructor_set_addr() truncate the 64-bit umem offset on 32-bit architectures?

The inline storage attempts to cast the address to uintptr_t:

    skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);

On 32-bit architectures, uintptr_t is 32 bits, which drops the upper 32 bits of the u64 addr. In af_xdp unaligned mode, the chunk offset is stored in bits 48-63.

Could this truncation lose the offset entirely, causing xsk_skb_destructor_get_addr() to retrieve a truncated address and submit it to the user-space completion queue?

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

Quoted 4 lines

net/xdp/xsk.c +0 -0

      if (err == -EOVERFLOW) {
              if (xs->skb) {

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