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

On Sun, May 3, 2026 at 11:09 PM <[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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?

It's a very carefully designed order of this series. Without this
patch the skb is unconditionally leaked (permanent memory loss), which
is strictly worse than a single spurious CQ completion that userspace
can handle. Patch 5/8 then eliminates the destructor issue by
postponing xsk_skb_init_misc() past all error paths.

This is why I move this patch ahead in this version.

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

As you noticed, this patch doesn't introduce new bugs at all and the
following patches take care of it.

>
> 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").

Right, it has been fixed...

No action needs to be taken so far.

Thanks,
Jason

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