Re: [PATCH net 3/4] xsk: drain continuation descs after overflow in xsk_build_skb()

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 05/15, Jason Xing wrote:
> On Fri, May 15, 2026 at 8:29 AM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
> >
> > On 05/14, Jason Xing wrote:
> > > On Thu, May 14, 2026 at 12:27 AM Stanislav Fomichev
> > > <sdf.kernel@gmail.com> wrote:
> > > >
> > > > On 05/10, Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > >
> > > > > When a multi-buffer packet exceeds MAX_SKB_FRAGS and triggers -EOVERFLOW,
> > > > > only the current descriptor is released from the TX ring. The remaining
> > > > > continuation descriptors of the same packet stay in the ring. Since
> > > > > xs->skb is set to NULL after the drop, the TX loop picks up these
> > > > > leftover frags and misinterprets each one as the beginning of a new
> > > > > packet, corrupting the packet stream.
> > > > >
> > > > > Fix this by adding a drain_cont flag to xdp_sock. When overflow occurs
> > > > > and the dropped descriptor has XDP_PKT_CONTD set, the flag is raised.
> > > > > The main TX loop in __xsk_generic_xmit() then handles continuation
> > > > > descriptors one at a time: each gets a normal CQ reservation (with
> > > > > backpressure), its address is submitted to the completion queue, and
> > > > > the descriptor is released from the TX ring. When the last fragment
> > > > > (without XDP_PKT_CONTD) is processed, the flag is cleared and the
> > > > > function returns -EOVERFLOW so the next call starts with a fresh
> > > > > budget for normal packets.
> > > > >
> > > > > This reuses the existing CQ backpressure and budget mechanisms, so if
> > > > > the CQ is full the function returns -EAGAIN and userspace drains the
> > > > > CQ before retrying. Zero buffer leakage, zero packet stream corruption.
> > > > >
> > > > > Closes: https://lore.kernel.org/all/20260425041726.85FB3C2BCB2@smtp.kernel.org/
> > > > > Fixes: cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")
> > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > > ---
> > > > >  include/net/xdp_sock.h |  1 +
> > > > >  net/xdp/xsk.c          | 22 ++++++++++++++++++++++
> > > > >  2 files changed, 23 insertions(+)
> > > > >
> > > > > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> > > > > index 23e8861e8b25..1958d19d9925 100644
> > > > > --- a/include/net/xdp_sock.h
> > > > > +++ b/include/net/xdp_sock.h
> > > > > @@ -80,6 +80,7 @@ struct xdp_sock {
> > > > >        * call of __xsk_generic_xmit().
> > > > >        */
> > > > >       struct sk_buff *skb;
> > > > > +     bool drain_cont;
> > > > >
> > > > >       struct list_head map_list;
> > > > >       /* Protects map_list */
> > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > > > index 3f1e590c855d..232dd7126905 100644
> > > > > --- a/net/xdp/xsk.c
> > > > > +++ b/net/xdp/xsk.c
> > > > > @@ -936,6 +936,8 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> > > > >                       xs->tx->invalid_descs++;
> > > > >               }
> > > > >               xskq_cons_release(xs->tx);
> > > > > +             if (xp_mb_desc(desc))
> > > > > +                     xs->drain_cont = true;
> > > > >       } else {
> > > > >               /* Let application retry */
> > > > >               xsk_cq_cancel_locked(xs->pool, 1);
> > > > > @@ -982,6 +984,26 @@ static int __xsk_generic_xmit(struct sock *sk)
> > > > >                       goto out;
> > > > >               }
> > > > >
> > > > > +             if (unlikely(xs->drain_cont)) {
> > > > > +                     unsigned long flags;
> > > > > +                     u32 idx;
> > > > > +
> > > >
> > > > [..]
> > > >
> > > > > +                     spin_lock_irqsave(&xs->pool->cq_prod_lock, flags);
> > > > > +                     idx = xskq_get_prod(xs->pool->cq);
> > > > > +                     xskq_prod_write_addr(xs->pool->cq, idx, desc.addr);
> > > > > +                     xskq_prod_submit_n(xs->pool->cq, 1);
> > > > > +                     spin_unlock_irqrestore(&xs->pool->cq_prod_lock, flags);
> > > >
> > > > Not sure I understand why you want this if you're still marking the desc
> > > > invalid.
> > >
> > > The key point is that as long as we read the desc from txq, the desc
> > > should be either put back to txq again or publish it in the cq (for
> > > application to keep track of this) at this point. Or else, the
> > > application will lose track of this desc, which breaks the whole
> > > logic.
> >
> > Yes, makes sense!
> >
> > > > > +
> > > > > +                     xs->tx->invalid_descs++;
> > > > > +                     xskq_cons_release(xs->tx);
> > > > > +                     if (!xp_mb_desc(&desc)) {
> > > > > +                             xs->drain_cont = false;
> > > > > +                             err = -EOVERFLOW;
> > > > > +                             goto out;
> > > > > +                     }
> > > >
> > > > I also don't understand why you want to return -EOVERFLOW again? Why not
> > > > (quietly) swallow these invalid xp_mb_desc from the previous packet and move
> > > > on?
> > >
> > > This particular desc is really one of overflow cases, right? We should
> > > warn users to handle this with this error code.
> >
> > Right, but didn't we already return -EOVERFLOW to the user once?
> > The part where you set drain_count=true will -EOVERFLOW. Then this
> > remainder will also return -EOVERFLOW?
> 
> Right, my intention is to alert users twice under this kind of
> circumstance. We silently drain the remaining portion of the skb the
> second time, which looks a bit strange, doesn't it?
> 
> >
> > But let's maybe step back, what's the expectation on the user
> > when we return -EOVERFLOW? In theory, the user can re-submit new
> > shorter packet (at the current prod index), right? And then your
> > !xp_mb_desc logic will break/swallow/-EOVERFLOW it?
> 
> People would be aware of the skb that is too big to handle when
> receiving two times of overflow warning. To put it in a simpler way,
> the second time in xmit path only handling the remaining is enough, no
> more descs that belong to another skb should be taken care of.
> 
> If the user is able to quickly react to this case, I think the whole
> skb should be re-put into the txq again instead of adding the
> remaining part. I'm not so sure if I interpret the "break" correctly
> here.

Let's say the user puts a packet with too many descriptors. We find that
mid-way and return -EOVERFLOW. What is user supposed to do with that error?
In my mind, the user should drain TX ring completely and start posting
shorter packets. You seem to be trying to handle the case where the user
leaves the remainder of the previously EOVERFLOW'd packet in place, why?