Re: [PATCH net v4 2/8] xsk: handle NULL dereference of the skb without frags issue

Jason Xing
2026-04-29T03:53:42Z

On Tue, Apr 28, 2026 at 2:35 PM Simon Horman <horms@kernel.org> wrote:
>
> From: 'Simon Horman' <horms@kernel.org>
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
>
> Simon says:
>
> Hi Jason,
>
> I'm providing this for informational purposes.
> My unedrstanding is that the problem flagged below
> is not present with the full 8 patches of the series
> applied.

Hi simon,

Thanks for letting me know:)

Yes, it will disappear after applying the whole series.

>
> ---
> xsk: handle NULL dereference of the skb without frags issue
>
> This change guards the existing xsk_inc_num_desc()/xsk_drop_skb() calls
> in xsk_build_skb()'s free_err path with an xs->skb check, and adds a new
> else branch that cancels the reserved CQ slot and bumps invalid_descs
> when xs->skb is NULL.
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 887abed25466..54d4895fc11e 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -973,9 +973,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > kfree_skb(skb);
> >
> > if (err == -EOVERFLOW) {
> > - /* Drop the packet */
> > - xsk_inc_num_desc(xs->skb);
> > - xsk_drop_skb(xs->skb);
> > + if (xs->skb) {
> > + /* Drop the packet */
> > + xsk_inc_num_desc(xs->skb);
> > + xsk_drop_skb(xs->skb);
> > + } else {
> > + xsk_cq_cancel_locked(xs->pool, 1);
> > + xs->tx->invalid_descs++;
> > + }
>
> The changelog states:
>
> In this series, the skb is already freed by kfree_skb() inside
> xsk_build_skb_zerocopy for the first-descriptor case, so we only need
> to do the bookkeeping: cancel the one reserved CQ slot and account
> for the single invalid descriptor.
>
> Is that accurate for this commit?

The commit says "in this series,..." which might confuse the AI reviewer.
>
> Looking at xsk_build_skb_zerocopy() as it stands here, neither of its
> error returns actually frees the skb:
>
> if (!skb) {
> ...
> skb = sock_alloc_send_skb(&xs->sk, hr, 1, &err);
> if (unlikely(!skb))
> 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))
> return ERR_PTR(err);
> }
> }
> ...
> for (copied = 0, i = skb_shinfo(skb)->nr_frags; copied < len; i++) {
> if (unlikely(i >= MAX_SKB_FRAGS))
> return ERR_PTR(-EOVERFLOW);
>
> page = pool->umem->pgs[addr >> PAGE_SHIFT];
> get_page(page);
> ...
> skb_fill_page_desc(skb, i, page, offset, copy);
> ...
> }
>
> So on the first-descriptor -EOVERFLOW path (xs->skb == NULL,
> IFF_TX_SKB_NO_LINEAR device), doesn't xsk_build_skb_zerocopy() return
> with the skb still allocated, up to MAX_SKB_FRAGS pages pinned via
> get_page() and attached through skb_fill_page_desc(), and an
> sk_wmem_alloc charge held by xsk_destruct_skb (installed by
> xsk_skb_init_misc)?
>
> Control then returns to xsk_build_skb():
>
> skb = xsk_build_skb_zerocopy(xs, desc);
> if (IS_ERR(skb)) {
> err = PTR_ERR(skb);
> skb = NULL;
> goto free_err;
> }
> ...
> free_err:
> if (skb && !skb_shinfo(skb)->nr_frags)
> kfree_skb(skb);
>
> Since the local skb has been set to NULL, the kfree_skb() guard is
> skipped, and the new else-branch only performs the CQ cancel and
> invalid_descs bump.
>
> Doesn't that leak the sk_buff, the attached frag pages, and the
> sk_wmem_alloc charge every time a first descriptor hits the i >=
> MAX_SKB_FRAGS condition (or the xsk_skb_metadata failure)?

This is the issue that I fixed in the subsequent patches.

>
> A later patch in the series, "xsk: free the skb when hitting the upper
> bound MAX_SKB_FRAGS", adds an "if (!xs->skb) kfree_skb(skb);" inside
> xsk_build_skb_zerocopy() before the -EOVERFLOW return, which seems to
> confirm the skb is not freed at this commit. Should the changelog's
> premise be reworked, or the free added here so each patch in the series
> stands on its own?

It's only about the description of the patch and the whole series
IIUC. If a new version is needed, I will send a v5 then :)

Thanks,
Jason