Re: [PATCH RFC net-next v4 02/14] xsk: extend xsk_build_skb() to support passing an already allocated skb

Jason Xing
2026-04-15T13:19:56Z

On Wed, Apr 15, 2026 at 4:52 PM <sashiko-bot@kernel.org> wrote:
>
> Sashiko AI review found 1 potential issue(s):
> - [High] Inconsistent ownership semantics of `allocated_skb` on error paths lead to a memory leak in the zero-copy path.
> --
>
> commit a6016c1c59927d7e151f38b7f237ad699db86b1e
> Author: Jason Xing <kernelxing@tencent.com>
>
> xsk: extend xsk_build_skb() to support passing an already allocated skb
>
> This patch extends xsk_build_skb() to accept an allocated_skb parameter,
> allowing the batch feature to reuse an already allocated skb instead of
> allocating a new one.
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 1194268ae8676..becc8ebfd82f8 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
>
> [ ... ]
>
> > @@ -793,15 +797,16 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> > return skb;
> > }
> >
> > -static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > - struct xdp_desc *desc)
> > +struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > + struct sk_buff *allocated_skb,
> > + struct xdp_desc *desc)
> > {
> > struct net_device *dev = xs->dev;
> > struct sk_buff *skb = xs->skb;
> > int err;
> >
> > if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
> > - skb = xsk_build_skb_zerocopy(xs, desc);
> > + skb = xsk_build_skb_zerocopy(xs, allocated_skb, desc);
> > if (IS_ERR(skb)) {
> > err = PTR_ERR(skb);
> > skb = NULL;
> > goto free_err;
>
> Does this leak allocated_skb on the zero-copy error path?
>
> If xsk_build_skb_zerocopy() fails on the first descriptor (e.g., if
> xsk_skb_metadata() returns an error due to invalid metadata), it returns an
> ERR_PTR without freeing the SKB.
>
> Because skb is then explicitly set to NULL before jumping to free_err,
> kfree_skb() is skipped.
>
> While the original code also had a leak here for internally allocated SKBs,

Awesome! sashiko spotted a real bug prior to the series. I will cook a
standalone patch to fix that first.

Also I checked the MAX_SKB_FRAGS situation in xsk_build_skb_zerocopy()
that has the absolutely same issue.

While reviewing the whole destruction of skb in the error conditions,
I realized that there is one problem in between patch 2 and 13. Here
at least I need to add the following codes to make sure no wrong
accounting happen:
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 3bf81b838075..19bc786decbc 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -904,14 +904,16 @@ struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
if (skb && !skb_shinfo(skb)->nr_frags)
kfree_skb(skb);

- if (err == -EOVERFLOW) {
- /* Drop the packet */
- xsk_inc_num_desc(xs->skb);
- xsk_drop_skb(xs->skb);
- xskq_cons_release(xs->tx);
- } else {
- /* Let application retry */
- xsk_cq_cancel_locked(xs->pool, 1);
+ if (!allocated_skb) {
+ if (err == -EOVERFLOW) {
+ /* Drop the packet */
+ xsk_inc_num_desc(xs->skb);
+ xsk_drop_skb(xs->skb);
+ xskq_cons_release(xs->tx);
+ } else {
+ /* Let application retry */
+ xsk_cq_cancel_locked(xs->pool, 1);
+ }
}

return ERR_PTR(err);

And the above part will be removed in patch 13. It prevents someone
from bisecting in between two patches and triggering the issue.

Thanks,
Jason

> this patch extends the issue to caller-provided SKBs. In copy mode,
> xsk_build_skb() correctly frees allocated_skb on error, but the zero-copy
> path does not.
>
> Could this lead to memory exhaustion if a caller assumes xsk_build_skb()
> always consumes or frees the provided SKB on error?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260415082654.21026-1-kerneljasonxing@gmail.com?part=2