Re: [PATCH net 1/4] xsk: avoid skb leak in XDP_TX_METADATA case

[sashiko-bot@kernel.org]

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

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/20260415085204.3F87AC19424@smtp.kernel.org/

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

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

>  				return ERR_PTR(err);
> +			}
>  		}

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