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

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] Completion Queue (CQ) state corruption when handling -EOVERFLOW for the first descriptor.
> --
>
> commit 819a57cd3753db9f503ac093f83dccc180da574d
> Author: Jason Xing <[email protected]>
>
> xsk: handle NULL dereference of the skb without frags issue
>
> This commit fixes a NULL pointer dereference when a first descriptor
> triggers -EOVERFLOW in xsk_build_skb_zerocopy() by adding a check for
> xs->skb. It handles the first-descriptor case by manually canceling the CQ
> slot and incrementing invalid_descs.
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -976,9 +976,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);
>
> Does this cause a Completion Queue (CQ) state corruption?
>
> When a first descriptor triggers -EOVERFLOW in xsk_build_skb_zerocopy(), the
> free_err path first calls kfree_skb(skb) just above this code. Because
> xsk_skb_init_misc() was called earlier, the SKB's destructor is set to
> xsk_destruct_skb(). The kfree_skb() call invokes this destructor, which
> submits a completion and increments the ring's producer index.
>
> If we then call xsk_cq_cancel_locked() here, it decrements the reservation
> index while the producer index was already incremented, breaking the core
> invariant cached_prod >= ring->producer.
>
> This causes space calculations to underflow and can overwrite unconsumed
> entries in the CQ.
>
> I see this was addressed later in the series by commit 88b75ff1d12b ("xsk:
> prevent CQ desync when freeing half-built skbs in xsk_build_skb()").

Yep, it has been resolved in the series.

Thanks,
Jason

>
> > +                     xs->tx->invalid_descs++;
> > +             }
> >               xskq_cons_release(xs->tx);
> >       } else {
> >               /* Let application retry */
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3