Re: [PATCH net 2/2] packet: use ubuf_info completion for TX_RING packets

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Paolo Abeni wrote:
> On 9/14/26 23:37, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> > 
> > tpacket_snd sends skbs with frags pointing into its ring slots. Slots
> > are released when skb->destructor is called.
> > 
> > A call to skb_orphan calls skb->destructor before the skb is freed.
> > This can cause the slot to be reused while still linked into the skb.
> > 
> > Switch to standard zerocopy completion (ubuf_info) so the slot is only
> > released once all references to the payload are freed or copied.
> > Restore skb->destructor to standard sock_wfree.
> > 
> > To prevent userspace from aliasing in-flight state on shared ring
> > slots, allocate tpacket_uarg per packet, rather than per slot. This
> > adds a small allocation to the transmit path. Use standard kmalloc to
> > allow backporting to stable kernels.
> > 
> > The uarg holds an sk_wmem_alloc reference, rather than an sk_refcnt
> > reference. packet_free_tx_ring waits on sk_wmem_alloc before freeing
> > the ring pages.
> > 
> > As a result a slot is released when its payload is copied, which can
> > be before transmission (e.g., in skb_orphan_frags_rx). Any slot
> > timestamp then reflects the time of copy, rather than of transmit
> > (or skb_orphan).
> > 
> > Revert the now unused previous skb_zcopy_.._nouarg infra.
> > 
> > Reported-by: Katherine Leaver <kleaver@janestreet.com>
> > Reported-by: Bjoern Doebel <doebel@amazon.de>
> > Closes: https://lore.kernel.org/netdev/20260909085542.3370986-1-doebel@amazon.de/
> > Fixes: 5cd8d46ea156 ("packet: copy user buffers before orphan or clone")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Willem de Bruijn <willemb@google.com>

> FTR both the 'high prio' sashiko finding here and the mid one on the 
> previous patch are IMHO worth addressing.

Absolutely, agreed.

I hadn't gotten around to responding to the bot yet, sorry. Was still
reviewing the options.

Simplest is to enable the deferred worker that Kyle also for page
backed rings.

As the commit says, I'd rather send something much simpler to stable,
but after exploring many paths did not found any with fewer risks or
obvious regressions.
 
> Also I'm wondering if the extra alloc/free is visible in perf figures?

It should not, compared to the skb alloc. But I don't have hard data
on that.

> Out of sheer ignorance, can't the ubuf be carved out of the ring?

It can, I actually had that first. But that has more risk. Userspace
can overwrite the ring header status to TP_STATUS_AVAILABLE,
possibly corrupting uarg->ubuf.refcnt. It might be fixable, by
incrementing refcnt rather than initializing to 1. But that is less
obvious(ly correct).