Re: [PATCH RFC net-next v4 03/14] xsk: add xsk_alloc_batch_skb() to build skbs in batch

On Wed, Apr 15, 2026 at 5:17 PM <[email protected]> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [High] Pre-allocated SKB and socket memory charge (`sk_wmem_alloc`) are permanently leaked when zerocopy metadata processing fails.
> - [High] Double-free and Use-After-Free of `xs->skb` when fragment allocation fails with `-EAGAIN` in copy mode.
> --
>
> commit 3ef2c03b6204759327ab337fe34617d9dd8e04ef
> Author: Jason Xing <[email protected]>
>
> xsk: add xsk_alloc_batch_skb() to build skbs in batch
>
> This commit introduces xsk_alloc_batch_skb() to support allocating and
> building skbs in batch. It reserves skbs, batches the sk_wmem_alloc charge,
> and then copies data to complete initialization.
>
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
>
> [ ... ]
>
> > +     /* Phase 3: Build SKBs with packet data */
> > +     for (j = 0; j < alloc_descs; j++) {
> > +             if (!xs->skb) {
> > +                     skb = skbs[skb_count - 1 - k];
> > +                     k++;
> > +             }
> > +
> > +             skb = xsk_build_skb(xs, skb, &descs[j]);
> > +             if (IS_ERR(skb)) {
> > +                     *err = PTR_ERR(skb);
> > +                     break;
> > +             }
>
> Can this lead to a double-free and use-after-free of xs->skb if a fragment
> allocation fails?
>
> If xsk_build_skb() fails to allocate a page for a subsequent fragment in copy
> mode, it frees the partially built skb and returns -EAGAIN. However, it
> leaves xs->skb pointing to this freed memory.
>
> When this loop breaks and returns -EAGAIN up the call chain, the caller
> __xsk_generic_xmit_batch() checks xs->skb and calls xsk_drop_skb(xs->skb),
> which would double-free the skb. Furthermore, any subsequent transmissions
> could dereference the dangling pointer. Should xs->skb be explicitly cleared
> when handling this error?

Ah, great job! Sashiko again found a real pre-existing bug! I will
include it in my separate fixes.

The corresponding fix would be adding a more strict check in
xsk_build_skb() to exclude the case and let the caller to free it:
free_err:
        if (skb && !xs->skb && !skb_shinfo(skb)->nr_frags)
                kfree_skb(skb);

>
> > +
> > +             if (xp_mb_desc(&descs[j])) {
> > +                     xs->skb = skb;
> > +                     continue;
> > +             }
> > +
> > +             xs->skb = NULL;
> > +             __skb_queue_tail(&batch->send_queue, skb);
> > +     }
> > +
> > +     /* Phase 4: Reclaim unused allocated SKBs */
> > +     while (k < i)
> > +             kfree_skb(skbs[skb_count - 1 - k++]);
>
> Does this permanently leak the pre-allocated skb and its socket memory charge
> if xsk_build_skb() fails?
>
> When xsk_build_skb() encounters an error like a zerocopy metadata processing
> failure, it returns an ERR_PTR() without freeing the skb. Because k was
> already incremented before the xsk_build_skb() call, the Phase 4 loop starts
> reclaiming at the next skb.
>
> The failed skb at index k - 1 is bypassed. Since it is never freed,
> sock_wfree is skipped, leaking both the skb and the batched sk_wmem_alloc
> charge.

This is the pre-existing bug that you pointed out in patch 2. I'll
take care of it for sure.

Thanks,
Jason

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