[PATCH net 1/2] virtio_net: copy zerocopy frags in start_xmit without NAPI

[PATCH net 1/2] virtio_net: copy zerocopy frags in start_xmit without NAPI

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Virtio-net without NAPI frees completed skbs lazily on the next
start_xmit. Senders waiting for in-flight zerocopy buffers can
deadlock if they cannot transmit more packets, as then no
commpleted packets will be freed.

When !use_napi, virtio-net already calls skb_orphan to avoid waiting
up for transmitted skbs to be freed. For zerocopy packets that
require deep copying on orphan (i.e. those that do not set
SKBFL_DONT_ORPHAN, such as PACKET_TX_RING), call skb_orphan_frags
before orphaning to release the buffers.

This fixes the tpacket_snd slot reuse bug on skb_orphan for
virtio-net, and prevents PACKET_TX_RING from running out of slots.

This fix also touches vhost_net zerocopy packets, which also do not
set SKBFL_DONT_ORPHAN. This is fine: vhost_net packets only encounter
virtio-net in nested virtualization, and only if napi_tx is
explicitly disabled (it has been default-enabled since Linux 4.12).
In that rare case, copying the frags is desirable anyway to prevent
holding guest descriptors pinned across unbounded intervals.

This is a prerequisite for the next patch, which converts
PACKET_TX_RING to standard zerocopy completion. Without this patch
first, a bounded ring sender can stall indefinitely behind a
virtio-net virtqueue that cannot reclaim.

Fixes: 5cd8d46ea156 ("packet: copy user buffers before orphan or clone")
Cc: stable@vger.kernel.org
Cc: mst@redhat.com
Cc: jasowangio@gmail.com
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/virtio_net.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e34c52d059d3..1ce528c18f9e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3349,6 +3349,13 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	else
 		virtqueue_disable_cb(sq->vq);
 
+	if (!use_napi &&
+	    unlikely(skb_orphan_frags(skb, GFP_ATOMIC))) {
+		DEV_STATS_INC(dev, tx_dropped);
+		dev_kfree_skb_any(skb);
+		return NETDEV_TX_OK;
+	}
+
 	/* timestamp packet in software */
 	skb_tx_timestamp(skb);
 
-- 
2.55.0.1032.g73a4cd73de-goog

Re: [PATCH net 1/2] virtio_net: copy zerocopy frags in start_xmit without NAPI

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The new drop path added to start_xmit()
  (drivers/net/virtio_net.c, the `if (!use_napi &&…

--

virtio_net: copy zerocopy frags in start_xmit without NAPI

For the !use_napi path, start_xmit() now calls skb_orphan_frags() before
the existing skb_orphan(), so zerocopy frags that do not set
SKBFL_DONT_ORPHAN (PACKET_TX_RING, vhost_net) are deep-copied instead of
staying pinned until the next transmit. On copy failure the skb is
dropped and tx_dropped is bumped.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e34c52d059d39..1ce528c18f9e7 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3349,6 +3349,13 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	else
>  		virtqueue_disable_cb(sq->vq);
>  
> +	if (!use_napi &&
> +	    unlikely(skb_orphan_frags(skb, GFP_ATOMIC))) {
> +		DEV_STATS_INC(dev, tx_dropped);
> +		dev_kfree_skb_any(skb);
> +		return NETDEV_TX_OK;
> +	}
> +

[Severity: Medium]
Can this early return leave already-queued descriptors in the avail ring
with no doorbell?

In the !use_napi path the only notification in start_xmit() is at the end:

	kick = use_napi ? __netdev_tx_sent_queue(txq, skb->len, xmit_more) :
			  !xmit_more || netif_xmit_stopped(txq);
	if (kick) {
		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {

Earlier skbs of a batch call xmit_skb() with xmit_more == true, publish
their descriptors and deliberately skip the notify, relying on the last
skb of the batch to ring the bell. dev_hard_start_xmit() decides that:

net/core/dev.c:dev_hard_start_xmit() {
	...
		rc = xmit_one(skb, dev, txq, next != NULL);
	...
}

so the final skb in the chain is the one with xmit_more == false.

If that final skb is a zerocopy skb without SKBFL_DONT_ORPHAN (an
AF_PACKET TX_RING sender, or vhost_net under nested virt) and
skb_copy_ubufs() fails its GFP_ATOMIC page allocation, the new block
returns NETDEV_TX_OK without reaching virtqueue_kick_prepare() /
virtqueue_notify(). Are the previously added frames then invisible to the
device until some unrelated later transmit on the same queue happens to
kick it?

Batching for this queue looks reachable without BQL: for a root qdisc
without TCQ_F_ONETXQUEUE on a multi-queue virtio-net device,
dequeue_skb() uses try_bulk_dequeue_skb_slow(), which chains up to 8 skbs
for the same txq. The BQL-driven try_bulk_dequeue_skb() path stays inert
here because the !use_napi path never calls __netdev_tx_sent_queue(), and
__free_old_xmit() accounts ORPHAN skbs into stats->packets, so
netdev_tx_completed_queue() is always fed zero bytes.

Would it work to fall through to the kick logic (or replicate it) before
dropping the skb? The pre-existing xmit_skb() failure return below has the
same shape, though it is annotated "This should not happen!", whereas this
new path turns a routine -ENOMEM under memory pressure into a
doorbell-skipping return.

>  	/* timestamp packet in software */
>  	skb_tx_timestamp(skb);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914214229.1674102-1-willemdebruijn.kernel%40gmail.com

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

[Willem de Bruijn]

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>

---

This is a complex patch for stable. I have tried a variety of
alternatives for stable, including a two-step with deferral of this
full fix to net-next. But all have worse caveats or side effects:

- disable tpacket_snd zerocopy:
  performance regression also on paths that do not call skb_orphan

- detect tpacket skb in skb_orphan and set po->tx_copy that
  disables tpacket_snd zerocopy:
  - does not fix the first skb/slot
  - hot path function now needs to check skb_shinfo(skb) field

- insert skb_orphan_frags in skb_orphan:
  - hot path function now needs to check skb_shinfo(skb) field
  - needs extra code for virtio-net and cxgb4, which link the frags
    into descriptors before calling skb_orphan.

- insert skb_orphan_frags at all relevant callers of skb_orphan:
  10+ sites across drivers and qdiscs

---

 include/linux/skbuff.h | 19 +---------
 net/packet/af_packet.c | 82 ++++++++++++++++++++++++++++--------------
 2 files changed, 56 insertions(+), 45 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 421f6fc45451..b14d6be7370b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1834,22 +1834,6 @@ static inline void skb_zcopy_set(struct sk_buff *skb, struct ubuf_info *uarg,
 	}
 }
 
-static inline void skb_zcopy_set_nouarg(struct sk_buff *skb, void *val)
-{
-	skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t) val | 0x1UL);
-	skb_shinfo(skb)->flags |= SKBFL_ZEROCOPY_FRAG;
-}
-
-static inline bool skb_zcopy_is_nouarg(struct sk_buff *skb)
-{
-	return (uintptr_t) skb_shinfo(skb)->destructor_arg & 0x1UL;
-}
-
-static inline void *skb_zcopy_get_nouarg(struct sk_buff *skb)
-{
-	return (void *)((uintptr_t) skb_shinfo(skb)->destructor_arg & ~0x1UL);
-}
-
 static inline void net_zcopy_put(struct ubuf_info *uarg)
 {
 	if (uarg)
@@ -1872,8 +1856,7 @@ static inline void skb_zcopy_clear(struct sk_buff *skb, bool zerocopy_success)
 	struct ubuf_info *uarg = skb_zcopy(skb);
 
 	if (uarg) {
-		if (!skb_zcopy_is_nouarg(skb))
-			uarg->ops->complete(skb, uarg, zerocopy_success);
+		uarg->ops->complete(skb, uarg, zerocopy_success);
 
 		skb_shinfo(skb)->flags &= ~SKBFL_ALL_ZEROCOPY;
 	}
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 76bde7906d49..41fc053075da 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2528,26 +2528,6 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
 	goto drop_n_restore;
 }
 
-static void tpacket_destruct_skb(struct sk_buff *skb)
-{
-	struct packet_sock *po = pkt_sk(skb->sk);
-
-	if (likely(po->tx_ring.pg_vec)) {
-		void *ph;
-		__u32 ts;
-
-		ph = skb_zcopy_get_nouarg(skb);
-
-		ts = __packet_set_timestamp(po, ph, skb);
-		__packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
-
-		packet_dec_pending(&po->tx_ring);
-		complete(&po->skb_completion);
-	}
-
-	sock_wfree(skb);
-}
-
 static int __packet_snd_vnet_parse(struct virtio_net_hdr *vnet_hdr, size_t len)
 {
 	if ((vnet_hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
@@ -2587,27 +2567,57 @@ static int packet_snd_vnet_parse(struct msghdr *msg, size_t *len,
 	return 0;
 }
 
+struct tpacket_uarg {
+	struct ubuf_info	ubuf;
+	struct packet_sock	*po;
+	void			*ph;
+};
+
+static void tpacket_ubuf_complete(struct sk_buff *skb, struct ubuf_info *uarg,
+				  bool success)
+{
+	struct tpacket_uarg *tu = container_of(uarg, struct tpacket_uarg, ubuf);
+	struct packet_sock *po = tu->po;
+	void *ph = tu->ph;
+	__u32 ts = 0;
+
+	if (!refcount_dec_and_test(&uarg->refcnt))
+		return;
+
+	if (likely(READ_ONCE(po->tx_ring.pg_vec))) {
+		if (likely(skb))
+			ts = __packet_set_timestamp(po, ph, skb);
+		__packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
+
+		packet_dec_pending(&po->tx_ring);
+		complete(&po->skb_completion);
+	}
+
+	kfree(tu);
+	sk_free(&po->sk);
+}
+
+static const struct ubuf_info_ops tpacket_ubuf_ops = {
+	.complete = tpacket_ubuf_complete,
+};
+
 static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
-		void *frame, struct net_device *dev, void *data, int tp_len,
+		struct net_device *dev, void *data, int tp_len,
 		__be16 proto, unsigned char *addr, int hlen, int copylen,
 		int hard_header_len,
 		const struct sockcm_cookie *sockc)
 {
-	union tpacket_uhdr ph;
 	int to_write, offset, len, nr_frags, len_max;
 	struct socket *sock = po->sk.sk_socket;
 	struct page *page;
 	int err;
 
-	ph.raw = frame;
-
 	skb->protocol = proto;
 	skb->dev = dev;
 	skb->priority = sockc->priority;
 	skb->mark = sockc->mark;
 	skb_set_delivery_type_by_clockid(skb, sockc->transmit_time, po->sk.sk_clockid);
 	skb_setup_tx_timestamp(skb, sockc);
-	skb_zcopy_set_nouarg(skb, ph.raw);
 
 	skb_reserve(skb, hlen);
 	skb_reset_network_header(skb);
@@ -2747,6 +2757,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 	struct virtio_net_hdr vnet_hdr;
 	bool has_vnet_hdr = false;
 	struct sockcm_cookie sockc;
+	struct tpacket_uarg *uarg;
 	__be16 proto;
 	int err, reserve = 0;
 	void *ph;
@@ -2874,7 +2885,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 				err = len_sum;
 			goto out_status;
 		}
-		tp_len = tpacket_fill_skb(po, skb, ph, dev, data, tp_len, proto,
+		tp_len = tpacket_fill_skb(po, skb, dev, data, tp_len, proto,
 					  addr, hlen, copylen, hard_header_len,
 					  &sockc);
 		if (likely(tp_len >= 0) &&
@@ -2906,7 +2917,24 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 			virtio_net_hdr_set_proto(skb, &vnet_hdr);
 		}
 
-		skb->destructor = tpacket_destruct_skb;
+		uarg = kmalloc(sizeof(*uarg), GFP_KERNEL);
+		if (unlikely(!uarg)) {
+			if (likely(len_sum > 0))
+				err = len_sum;
+			else
+				err = -ENOMEM;
+			goto out_status;
+		}
+		uarg->po = po;
+		uarg->ph = ph;
+		uarg->ubuf.ops = &tpacket_ubuf_ops;
+		uarg->ubuf.flags = SKBFL_ZEROCOPY_FRAG;
+		refcount_set(&uarg->ubuf.refcnt, 1);
+
+		/* Hold a sk_wmem_alloc reference until completion */
+		refcount_inc(&po->sk.sk_wmem_alloc);
+		skb_zcopy_init(skb, &uarg->ubuf);
+
 		__packet_set_status(po, ph, TP_STATUS_SENDING);
 		packet_inc_pending(&po->tx_ring);
 
-- 
2.55.0.1032.g73a4cd73de-goog

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 · Low: 1

- [High] tpacket_ubuf_complete() (net/packet/af_packet.c:2576) writes
  into the TX ring slot via…
- [Medium] Commit message overstates the ring-teardown lifetime
  guarantee.
- [Medium] uAPI semantics documented in
  Documentation/networking/packet_mmap.rst are no longer implemented,…
- [Low] Two comment/logic pairs in net/packet/af_packet.c still describe
  the tpacket skb destructor that this patch deletes.

--

packet: use ubuf_info completion for TX_RING packets

tpacket_snd() now attaches a per-packet ubuf_info (struct tpacket_uarg) to
the skb instead of overriding skb->destructor, so a ring slot is released
only once all references to the payload have been freed or copied. The uarg
holds an sk_wmem_alloc reference, and the now unused skb_zcopy_*_nouarg
helpers are removed.

Closes: https://lore.kernel.org/netdev/20260909085542.3370986-1-doebel@amazon.de/

A few questions on the commit message and on the new completion path.

The commit message says:

> 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.

[Severity: Medium]
Is that wait unconditional? In the tree at this commit it looks like it
applies only in two narrow cases:

net/packet/af_packet.c:packet_free_tx_ring() {
	deferred = vec->deferred;
	if (!deferred || !sk_wmem_alloc_get(sk)) {
		free_pg_vec(pg_vec, order, len);
		return;
	}
	...
}

and vec->deferred is only allocated for vmalloc-backed blocks:

net/packet/af_packet.c:alloc_pg_vec() {
	...
		if (tx_ring && !vec->deferred &&
		    is_vmalloc_addr(pg_vec[i].buffer)) {
			vec->deferred = kzalloc_obj(*vec->deferred,
						    GFP_KERNEL | __GFP_NOWARN);
	...
}

packet_free_tx_ring() is also reached only on the closing path:

net/packet/af_packet.c:packet_set_ring() {
	...
		if (tx_ring && closing)
			packet_free_tx_ring(sk, pg_vec, order, req->tp_block_nr);
		else
			free_pg_vec(pg_vec, order, req->tp_block_nr);
	...
}

So for the usual __get_free_pages()-backed ring, and for any non-closing
teardown, the new sk_wmem_alloc reference does not appear to hold the ring
pages. Could the message spell out which cases actually wait, since this is
the only lifetime argument offered for a change tagged Cc: stable?

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 76bde7906d494..41fc053075dae 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2528,26 +2528,6 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
>  	goto drop_n_restore;
>  }
>  
> -static void tpacket_destruct_skb(struct sk_buff *skb)
> -{
> -	struct packet_sock *po = pkt_sk(skb->sk);
> -
> -	if (likely(po->tx_ring.pg_vec)) {
> -		void *ph;
> -		__u32 ts;
> -
> -		ph = skb_zcopy_get_nouarg(skb);
> -
> -		ts = __packet_set_timestamp(po, ph, skb);
> -		__packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
> -
> -		packet_dec_pending(&po->tx_ring);
> -		complete(&po->skb_completion);
> -	}
> -
> -	sock_wfree(skb);
> -}
> -

[Severity: Low]
With tpacket_destruct_skb() gone, two comments that describe it are left
behind. In tpacket_snd() the xmit error path still reads:

net/packet/af_packet.c:tpacket_snd() {
	...
			if (err && __packet_get_status(po, ph) ==
				   TP_STATUS_AVAILABLE) {
				/* skb was destructed already */
				skb = NULL;
				goto out_status;
			}
			/*
			 * skb was dropped but not destructed yet;
			 * let's treat it like congestion or err < 0
			 */
	...
}

Do those inferences still hold? The slot can now be AVAILABLE while the skb
is alive (payload copied by skb_copy_ubufs()), and the skb can be gone while
the slot is still TP_STATUS_SENDING when a clone or segment still holds a
uarg reference, since tpacket_ubuf_complete() returns early on
!refcount_dec_and_test().

packet_free_tx_ring() also still says:

	/* A detached ring's pending count can miss late skb destructors. */

Should that now name tpacket_ubuf_complete() instead, given that is what
holds the extra sk_wmem_alloc reference the deferral waits on?

>  static int __packet_snd_vnet_parse(struct virtio_net_hdr *vnet_hdr, size_t len)
>  {
>  	if ((vnet_hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
> @@ -2587,27 +2567,57 @@ static int packet_snd_vnet_parse(struct msghdr *msg, size_t *len,
>  	return 0;
>  }
>  
> +struct tpacket_uarg {
> +	struct ubuf_info	ubuf;
> +	struct packet_sock	*po;
> +	void			*ph;
> +};
> +
> +static void tpacket_ubuf_complete(struct sk_buff *skb, struct ubuf_info *uarg,
> +				  bool success)
> +{
> +	struct tpacket_uarg *tu = container_of(uarg, struct tpacket_uarg, ubuf);
> +	struct packet_sock *po = tu->po;
> +	void *ph = tu->ph;
> +	__u32 ts = 0;
> +
> +	if (!refcount_dec_and_test(&uarg->refcnt))
> +		return;
> +
> +	if (likely(READ_ONCE(po->tx_ring.pg_vec))) {
> +		if (likely(skb))
> +			ts = __packet_set_timestamp(po, ph, skb);
> +		__packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
> +
> +		packet_dec_pending(&po->tx_ring);
> +		complete(&po->skb_completion);
> +	}

[Severity: High]
Can these writes through tu->ph land in pages that have already been freed?

The only guard here is the unsynchronized READ_ONCE(po->tx_ring.pg_vec)
test, and that load is not ordered against the swap(rb->pg_vec, pg_vec) plus
free in packet_set_ring()/packet_free_tx_ring(). For a page-backed ring the
close path takes the !deferred branch shown earlier and frees the blocks
immediately, even though sk_wmem_alloc_get(sk) is non-zero because of the
outstanding uarg reference.

The frag page references taken in tpacket_fill_skb() used to cover the
page-backed case, but they are dropped before the completion runs:

net/core/skbuff.c:skb_copy_ubufs() {
	...
	/* skb frags release userspace buffers */
	for (i = 0; i < num_frags; i++)
		skb_frag_unref(skb, i);
	...
release:
	skb_zcopy_clear(skb, false);
}

So a sequence like:

CPU0 (xmit/copy)			CPU1 (close)
tpacket_ubuf_complete()
  READ_ONCE(po->tx_ring.pg_vec)  /* non-NULL */
					packet_set_ring(closing=1)
					  packet_free_tx_ring()
					    free_pg_vec()  /* !deferred */
  __packet_set_status(po, ph, ...)  /* freed pages */

Frames whose payload is fully consumed by copylen (to_write == 0, e.g. a
vnet_hdr whose hdr_len covers the whole frame) never had a frag reference at
all. An analogous window existed for the deleted tpacket_destruct_skb() once
skb_copy_ubufs() had dropped the frag references, but this patch makes
copy-time completion the normal release path and re-bases the lifetime
argument on sk_wmem_alloc, so should packet_free_tx_ring() honour
sk_wmem_alloc unconditionally, or the ph access be made properly
synchronized?

Separately, on the pg_vec == NULL branch packet_dec_pending() is skipped.
Does a late completion after ring teardown leave the per-CPU pending count
permanently elevated?

> +
> +	kfree(tu);
> +	sk_free(&po->sk);
> +}
> +
> +static const struct ubuf_info_ops tpacket_ubuf_ops = {
> +	.complete = tpacket_ubuf_complete,
> +};
> +

[ ... ]

> @@ -2906,7 +2917,24 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  			virtio_net_hdr_set_proto(skb, &vnet_hdr);
>  		}
>  
> -		skb->destructor = tpacket_destruct_skb;
> +		uarg = kmalloc(sizeof(*uarg), GFP_KERNEL);
> +		if (unlikely(!uarg)) {
> +			if (likely(len_sum > 0))
> +				err = len_sum;
> +			else
> +				err = -ENOMEM;
> +			goto out_status;
> +		}
> +		uarg->po = po;
> +		uarg->ph = ph;
> +		uarg->ubuf.ops = &tpacket_ubuf_ops;
> +		uarg->ubuf.flags = SKBFL_ZEROCOPY_FRAG;

[Severity: Medium]
Since only SKBFL_ZEROCOPY_FRAG is set here (no SKBFL_DONT_ORPHAN), even a
plain skb_clone() takes the skb_orphan_frags() -> skb_copy_ubufs() ->
skb_zcopy_clear() path and completes the uarg, as do
skb_orphan_frags_rx() from dev_queue_xmit_nit() when any tap is attached,
tun_net_xmit(), and __skb_tstamp_tx().

At that point neither skb_hwtstamps(skb)->hwtstamp nor skb_tstamp(skb) has
been filled in, so tpacket_get_timestamp() returns 0 and the slot is
published as bare TP_STATUS_AVAILABLE.

Documentation/networking/packet_mmap.rst still documents the old contract:

	The kernel updates each status of sent
	frames with TP_STATUS_SENDING until the end of transfer.

	At the end of each transfer, buffer status returns to TP_STATUS_AVAILABLE.

and the PACKET_TIMESTAMP section tells applications to "wait for status of
relevant frames to be updated resp. the frame handed over to the
application", then "walk through the frames to pick up the individual hw/sw
timestamps", noting that with only TP_STATUS_AVAILABLE set "the tp_sec and
tp_{n,u}sec members do not contain a valid value".

Should packet_mmap.rst be updated in this patch to describe the new release
and timestamp point?

> +		refcount_set(&uarg->ubuf.refcnt, 1);
> +
> +		/* Hold a sk_wmem_alloc reference until completion */
> +		refcount_inc(&po->sk.sk_wmem_alloc);
> +		skb_zcopy_init(skb, &uarg->ubuf);
> +
>  		__packet_set_status(po, ph, TP_STATUS_SENDING);
>  		packet_inc_pending(&po->tx_ring);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914214229.1674102-1-willemdebruijn.kernel%40gmail.com

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

[Paolo Abeni]

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.

Also I'm wondering if the extra alloc/free is visible in perf figures? 
Out of sheer ignorance, can't the ubuf be carved out of the ring?

/P

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

[Willem de Bruijn]

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).