Re: [PATCH v3 net 1/6] xsk: fix buffer leak in xsk_drop_skb() for AF_XDP multi-buffer Tx

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Thu, Jul 16, 2026 at 01:42:28PM +0200, Jason Xing wrote:
> On Thu, Jul 16, 2026 at 1:25 PM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> wrote:
> >
> > On Thu, Jul 16, 2026 at 01:22:24PM +0200, Jason Xing wrote:
> > > On Tue, Jul 14, 2026 at 4:08 PM Maciej Fijalkowski
> > > <maciej.fijalkowski@intel.com> wrote:
> > > >
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > This patch is inspired by the check[1] from sashiko. It says when
> > > > overflow happens, the address of cq to be published is invalid.
> > > > Actually the severer thing is the whole process of publishing the
> > > > address of cq in this particular case is not right: it should truely
> > > > publish the address and advance the cached_prod in cq as long as it
> > > > reads descriptors from txq.
> > > >
> > > > The following is the full analysis.
> > > > xsk_drop_skb() is called in three places, which all discard a partially
> > > > built multi-buffer skb:
> > > > 1) xsk_build_skb() -EOVERFLOW error path: packet exceeds MAX_SKB_FRAGS
> > > > 2) __xsk_generic_xmit() post-loop cleanup: an invalid descriptor in
> > > >    the TX ring prevents the partial packet from completing
> > > > 3) xsk_release(): socket close while xs->skb holds an incomplete packet
> > > >
> > > > In all three cases, the TX descriptors for the already-processed frags
> > > > have been consumed from the TX ring (xskq_cons_release), and CQ slots
> > > > have been reserved. However, xsk_drop_skb() calls xsk_consume_skb()
> > > > which cancels the CQ reservations via xsk_cq_cancel_locked(). Since
> > > > the buffer addresses never appear in the completion queue, userspace
> > > > permanently loses track of these buffers.
> > > >
> > > > Fix this by letting consume_skb() trigger the existing xsk_destruct_skb
> > > > destructor, which already submits buffer addresses to the CQ via
> > > > xsk_cq_submit_addr_locked().
> > > >
> > > > Note that cancelling the descriptors back to the TX ring (via
> > > > xskq_cons_cancel_n) is not a appropriate option because an oversized
> > > > packet that always exceeds MAX_SKB_FRAGS would be retried indefinitely,
> > > > which is an obviously deadlock bug in the TX path.
> > > >
> > > > Also move the desc->addr assignment in xsk_build_skb() above the
> > > > overflow check so that the current descriptor's address is recorded
> > > > before a potential -EOVERFLOW jump to free_err, consistent with the
> > > > zerocopy path in xsk_build_skb_zerocopy().
> > > >
> > > > [1]: 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>
> > >
> > > Maciej, maybe your tag is missing here?
> >
> > IIRC this has been taken as-is from your patchset. The next one has been
> > touched in few ways and I included my co-developed tag there.
> 
> Yep, I mean maybe you can simply drop your reviewed-by or acked-by tag
> here if you approve.

I see. I am not sure what are principles for such cases, I could assume
that if I include this as a sender then it implies my ack on such change.
Anyways I can just:

Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

BTW now I reminded myself I dropped Stan's tags in whole set. I think it
was of changes on 4/6, otherwise I don't have other excuse.

So Jason if you bump at Stan somewhere in the netdevconf's hallway could
you ask him to re-ack it if it still works for him?

> 
> Thanks,
> Jason
> 
> >
> > >
> > > Thanks,
> > > Jason