Re: [PATCH net v5 5/8] xsk: prevent CQ desync when freeing half-built skbs in xsk_build_skb()

On Sun, May 3, 2026 at 11:09 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] In `xsk_build_skb_zerocopy()`, if `xsk_skb_metadata()` fails, the newly allocated `skb` is leaked.
> --
>
> commit 88b75ff1d12b482a55e1724cc457909cf3ea19dd
> Author: Jason Xing <[email protected]>
>
> xsk: prevent CQ desync when freeing half-built skbs in xsk_build_skb()
>
> Postpone the skb initialization phase in xsk_build_skb() until the first
> frag allocation is completed to prevent the skb destructor from running
> on error paths. Before the initialization, the skb can be safely freed
> by kfree_skb().
>
> Closes: https://lore.kernel.org/all/[email protected]/
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 55378c3855d5..af3c5752bb63 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -819,8 +819,6 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> >                       return ERR_PTR(err);
> >
> >               skb_reserve(skb, hr);
> > -
> > -             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))
>
> Could this leak the newly allocated skb if xsk_skb_metadata() fails?
>
> If xsk_skb_metadata() returns an error, xsk_build_skb_zerocopy() will return
> ERR_PTR(err) directly without freeing the skb.
>
> The caller receives the error pointer, sets its local skb variable to NULL,
> and jumps to the free_err label:
>
> net/xdp/xsk.c:xsk_build_skb() {
>     ...
>         if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
>                 skb = xsk_build_skb_zerocopy(xs, desc);
>                 if (IS_ERR(skb)) {
>                         err = PTR_ERR(skb);
>                         skb = NULL;
>                         goto free_err;
>                 }
>     ...
> }
>
> Because the skb is explicitly nullified here, the kfree_skb(skb) call in
> the cleanup path is bypassed, leaving the socket buffer and its associated
> sk_wmem_alloc charge leaked.
>
> While this pre-existing issue is explicitly fixed later in this patch series
> in commit "xsk: avoid skb leak in XDP_TX_METADATA case", should it be
> addressed earlier to ensure intermediate states remain safe during a bisect?

No, again, I've mentioned this previously. Not the same issue here. No
more severe issue is caused because of this. And it is fixed in the
series.

Thanks,
Jason

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