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

On Sun, Apr 19, 2026 at 12:58 PM <[email protected]> wrote:
>
> 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]/
>
> > 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.

I must state that this problem is not introduced by the series. The
following commit should be fixed soon:
commit 30c3055f9c0d84a67b8fd723bdec9b1b52b3c695
Author: Maciej Fijalkowski <[email protected]>
Date:   Thu Sep 25 18:00:09 2025 +0200

    xsk: wrap generic metadata handling onto separate function

    xsk_build_skb() has gone wild with its size and one of the things we can
    do about it is to pull out a branch that takes care of metadata handling
    and make it a separate function.

    While at it, let us add metadata SW support for devices supporting
    IFF_TX_SKB_NO_LINEAR flag, that happen to have separate logic for
    building skb in xsk's generic xmit path.

I'll consider folding it into the next version even though it's a bit
complex to handle :(

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

Amazing really. The same here. Sashiko checked this pre-existing
behavior is buggy, which the patch 1 only does follow.
In terms of the free_err logic in xsk_build_skb, the untransmitted skb
should be freed by the common new helper:
static void xsk_drop_untransmitted_skb(struct sk_buff *skb)
{
        skb->destructor = sock_wfree;
        kfree_skb(skb);
}

Will cook it as a standalone patch to fix the commit:
commit c30d084960cf316c95fbf145d39974ce1ff7889c
Author: Maciej Fijalkowski <[email protected]>
Date:   Thu Sep 25 18:00:07 2025 +0200

    xsk: avoid overwriting skb fields for multi-buffer traffic

    We are unnecessarily setting a bunch of skb fields per each processed
    descriptor, which is redundant for fragmented frames.

    Let us set these respective members for first fragment only. To address
    both paths that we have within xsk_build_skb(), move assignments onto
    xsk_set_destructor_arg() and rename it to xsk_skb_init_misc().

Thanks,
Jason

>
> >                               return ERR_PTR(err);
> > +                     }
> >               }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1