Re: [PATCH net v5 2/8] xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] This problem wasn't introduced by this patch, but returning -EOVERFLOW when xs->skb is NULL leads to an unconditional NULL pointer dereference in the caller's error path.
- [High] Calling kfree_skb(skb) directly on a partially built SKB triggers the custom XSK destructor, which incorrectly submits the failed descriptor to the AF_XDP completion queue.
- [High] This problem wasn't introduced by this patch, but an identical SKB memory leak exists when xsk_skb_metadata() fails.
--

commit cf402952a5ccf9981b4749df15227be947716b0d
Author: Jason Xing <kerneljasonxing@gmail.com>

xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS

This patch explicitly adds kfree_skb() before returning -EOVERFLOW to
its caller when hitting the MAX_SKB_FRAGS limit on a newly allocated
SKB. This prevents the SKB from being permanently leaked.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 887abed25466..d706b1e0bf60 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -856,8 +856,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);

Does calling kfree_skb() directly here incorrectly trigger the custom
XSK destructor for a dropped packet?

Earlier in xsk_build_skb_zerocopy(), the newly allocated SKB is initialized
via xsk_skb_init_misc(), which explicitly sets skb->destructor =
xsk_destruct_skb.

When xsk_destruct_skb() runs, it calls xsk_cq_submit_addr_locked(), which
erroneously generates a spurious TX completion for userspace and bypasses
the queue's cached_prod reservation state, corrupting the completion queue
logic.

Should the destructor be cleared first to safely discard the partially
built SKB?

Note that this regression was fixed later in the patch series by commit
88b75ff1d12b ("xsk: prevent CQ desync when freeing half-built skbs in
xsk_build_skb()").

This problem wasn't introduced by this patch, but can returning -EOVERFLOW
when xs->skb is NULL cause a NULL pointer dereference in the caller's error
path?

In the caller xsk_build_skb(), the local skb pointer is set to NULL, and
execution jumps to the free_err label. Because err == -EOVERFLOW, the error
handler unconditionally calls:

	xsk_inc_num_desc(xs->skb);
	xsk_drop_skb(xs->skb);

Since xs->skb is strictly NULL, these functions will immediately dereference
the NULL pointer via skb_shinfo(NULL) inside xsk_skb_destructor_is_addr(),
causing a panic.

Note that this regression was fixed later in the patch series by commit
819a57cd3753 ("xsk: handle NULL dereference of the skb without frags
issue").

This problem wasn't introduced by this patch either, but is there an
identical SKB memory leak earlier in this function when xsk_skb_metadata()
fails?

If desc->options & XDP_TX_METADATA is set and xsk_skb_metadata() fails,
the function returns ERR_PTR(err) directly without freeing the newly
allocated skb:

	xsk_skb_init_misc(skb, xs, desc->addr);
	if (desc->options & XDP_TX_METADATA) {
		err = xsk_skb_metadata(skb, buffer, desc, pool, hr);
		if (unlikely(err))
			return ERR_PTR(err);
	}

The caller xsk_build_skb() explicitly overrides its local skb pointer to
NULL before jumping to free_err, causing this SKB to be permanently leaked.

An unprivileged user could trigger this leak by providing malformed
metadata, leading to memory exhaustion.

Note that this regression was fixed later in the patch series by commit
f1079e939977 ("xsk: avoid skb leak in XDP_TX_METADATA case").

>  			return ERR_PTR(-EOVERFLOW);
> +		}

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