Re: [PATCH net 3/4] xsk: handle NULL dereference of the skb without frags issue

On Sun, Apr 19, 2026 at 12:58 PM <[email protected]> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [Critical] Calling `kfree_skb(skb)` without clearing the SKB destructor causes Completion Queue (CQ) ring corruption and false completions.
> - [High] Use-after-free of `xs->skb` in the continuation path when an error like `-EAGAIN` occurs.
> --
>
> commit 1c728000f884ce0a536d9c1fd763eb0c560ada51
> Author: Jason Xing <[email protected]>
>
> xsk: handle NULL dereference of the skb without frags issue
>
> This commit addresses a NULL pointer dereference in the error path of
> xsk_build_skb_zerocopy() by checking if xs->skb is valid before
> dereferencing it during -EOVERFLOW handling. It also adds an alternative
> branch to handle bookkeeping for the first-descriptor case.
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 3d6c6caa7ff82..2f00f188533c1 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -978,9 +978,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> >               kfree_skb(skb);
>
> Can this lead to a use-after-free on xs->skb?
>
> In the non-zerocopy path, if the first descriptor is linear, nr_frags remains
> 0 and the caller __xsk_generic_xmit() stores the SKB in xs->skb.

Patch 4 already handles this.

>
> During the next iteration for a continuation descriptor, if an error like
> -EAGAIN occurs (e.g., from alloc_page()), the code jumps to the free_err
> label. The cleanup logic if (skb && !skb_shinfo(skb)->nr_frags) would
> evaluate to true, freeing the entire SKB.
>
> The function then returns -EAGAIN, leaving the freed SKB pointer in xs->skb.
> On the next application retry, the code could dereference the freed xs->skb.
>
> Note that this regression is fixed later in the patch series by commit 'xsk:
> fix use-after-free of xs->skb in xsk_build_skb() free_err path'.
>
> Also, could calling kfree_skb(skb) without clearing the SKB destructor cause
> Completion Queue (CQ) ring corruption and false completions?
>
> In xsk_build_skb_zerocopy(), xsk_skb_init_misc() sets
> skb->destructor = xsk_destruct_skb. When an error occurs on the first
> descriptor (e.g., xsk_skb_metadata() returning -EINVAL), kfree_skb(skb) is
> called.
>
> Because the destructor is not overridden with sock_wfree (like
> xsk_consume_skb() does), kfree_skb(skb) triggers xsk_destruct_skb(), which
> submits the descriptor to the CQ and increments q->ring->producer.
>
> >       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++;
> > +             }
> >               xskq_cons_release(xs->tx);
> >       } else {
> >               /* Let application retry */
> >               xsk_cq_cancel_locked(xs->pool, 1);
> >       }
>
> The error handling paths then call xsk_cq_cancel_locked(), which decrements
> the reservation counter q->cached_prod.
>
> Does decrementing the reservation while the actual producer pointer has
> advanced break queue accounting?
>
> The kernel calculates available space using cached_prod; this artificial
> underflow could cause the queue to over-report free space, allowing the
> producer to silently overwrite unread user-space CQ entries.
>
> Furthermore, in the -EINVAL path, does the invalid descriptor generate a
> false completion to user space without being released from the TX ring? When
> the application reuses the completed buffer, could the kernel retransmit the
> same descriptor on retry, causing a use-after-free and data corruption?

For the above two issues, yes, we've discussed in the previous patch :)

Thanks,
Jason

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