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

On Sun, Apr 19, 2026 at 12:58 PM <[email protected]> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [High] The patch leaves a guaranteed NULL pointer dereference in the caller's error handling path when `-EOVERFLOW` is returned for the first fragment.
> - [High] Calling `kfree_skb(skb)` directly triggers the packet destructor and incorrectly submits the dropped packet to the userspace Completion Queue (CQ).
> --
>
> commit e5bfaac263557abd44f47ec6e9c85e50c7aa1efc
> Author: Jason Xing <[email protected]>
>
>     xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS
>
> This commit explicitly frees the skb when the MAX_SKB_FRAGS limit is hit
> on the first descriptor to avoid a memory leak.
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 5b273bcd0dbce..3d6c6caa7ff82 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -858,8 +858,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);
>
> If we free the newly allocated skb here and return -EOVERFLOW, the caller
> xsk_build_skb() catches the error and jumps to free_err, where it calls:
>
>     if (err == -EOVERFLOW) {
>         /* Drop the packet */
>         xsk_inc_num_desc(xs->skb);
>
> Since xs->skb is still NULL, will this cause xsk_inc_num_desc() to
> dereference a NULL pointer?
>
> Note that this regression appears to be fixed in a later commit 1c728000f884c
> ("xsk: handle NULL dereference of the skb without frags issue").

As you noticed, it was already fixed by patch 3.

>
> Additionally, does calling kfree_skb(skb) here trigger the packet destructor
> and incorrectly submit the dropped packet to the userspace completion queue?
>
> Prior to entering the fragmentation loop, xsk_skb_init_misc() sets the
> destructor to xsk_destruct_skb(). When kfree_skb(skb) is called, it invokes
> this destructor, which sequentially calls xsk_cq_submit_addr_locked().
>
> This writes the descriptor address to the completion queue and advances the
> user-visible q->ring->producer pointer, falsely notifying userspace of a
> successful transmission.
>
> The caller xsk_build_skb() then executes its own error handling, calling
> xsk_cq_cancel_locked(), which only decrements the kernel's internal
> q->cached_prod counter.
>
> Could this desynchronize the internal queue tracking variables from the
> shared memory ring pointers?
>
> Should the destructor be changed back to sock_wfree before freeing it,
> similar to how xsk_consume_skb() handles it?

Right, it's the same (pre-existing) bug as pointed out in patch 1.

Thanks,
Jason

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