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

[sashiko-bot@kernel.org]

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 <kerneljasonxing@gmail.com>

    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.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 5b273bcd0dbce..3d6c6caa7ff82 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -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?

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

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