[PATCH net v2 0/2] packet: fix PACKET_TX_RING data corruption on skb_orphan

[PATCH net v2 0/2] packet: fix PACKET_TX_RING data corruption on skb_orphan

[Willem de Bruijn]

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

When transmitting packets via PACKET_TX_RING, tpacket_snd links user
ring buffer pages as skb frags and releases the slot on skb->destructor
(tpacket_destruct_skb).

skb_orphan() invokes the destructor while the skb is still alive.
This marks the slot as TP_STATUS_AVAILABLE prematurely, allowing
userspace to overwrite the slot and causing data corruption.

This series fixes the issue by switching PACKET_TX_RING to standard
ubuf_info zerocopy completion, ensuring ring slots are released only
after all payload references are freed or copied.

Virtio-net needs a separate solution, because deferring the release
can cause deadlock in its !use_napi mode.

- Patch 1 addresses the virtio-net special case.
- Patch 2 converts tpacket_snd to standard ubuf_info completion

Patch 1 must be applied, and backported, before patch 2. Both carry
the same Fixes tag for that reason.

v1: https://lore.kernel.org/netdev/20260914214229.1674102-1-willemdebruijn.kernel@gmail.com/

Willem de Bruijn (2):
  virtio_net: copy zerocopy frags in start_xmit without NAPI
  packet: use ubuf_info completion for TX_RING packets

 drivers/net/virtio_net.c |   9 ++++
 include/linux/skbuff.h   |  19 +-------
 net/packet/af_packet.c   | 102 ++++++++++++++++++++++++---------------
 3 files changed, 74 insertions(+), 56 deletions(-)

-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 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
completed 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>

---

v1->v2
  - Do not suppress device kick on skb_orphan_frags error when !xmit_more

---
 drivers/net/virtio_net.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e34c52d059d3..bf82ef9874ab 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3349,6 +3349,14 @@ 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);
+		kick = !xmit_more || netif_xmit_stopped(txq);
+		goto kick_vq;
+	}
+
 	/* timestamp packet in software */
 	skb_tx_timestamp(skb);
 
@@ -3381,6 +3389,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	kick = use_napi ? __netdev_tx_sent_queue(txq, skb->len, xmit_more) :
 			  !xmit_more || netif_xmit_stopped(txq);
+kick_vq:
 	if (kick) {
 		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
 			u64_stats_update_begin(&sq->stats.syncp);
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 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.

The ubuf_info completion callback can be called with a NULL skb, but
only from net_zcopy_put and related API, used by zerocopy implementations
that hold their own reference on the uarg, such as MSG_ZEROCOPY. This
uarg is only ever completed from skb_zcopy_clear, so skb is always set.

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. Always allocate vec->deferred for tx_ring so page-backed
rings also wait on sk_wmem_alloc when skb_copy_ubufs drops page refs
before calling tpacket_ubuf_complete.

Drop the tx_ring.pg_vec test that tpacket_destruct_skb performed before
accessing the slot. The sk_wmem_alloc reference now guarantees that the
slot is valid. The test is also not sufficient by itself, as it reads
pg_vec without pg_vec_lock, so it can race with packet_set_ring.

As a result a slot is released when its payload is copied, which can
be before transmission (e.g., in skb_orphan_frags_rx). If copied
before skb_tx_timestamp() is called, no slot timestamp is recorded,
similar to when skb_orphan() was called early in the datapath before
this patch.

Revert the now unused previous skb_zcopy_.._nouarg infra.

Depends on commit 992cc9f94ca9 ("net/packet: defer vmalloc TX_RING
free until skbs finish").

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>

---

v1->v2
  - Always allocate vec->deferred for tx_ring so page-backed rings also
    defer freeing until sk_wmem_alloc drops to zero
  - Clarify in commit message that copying before skb_tx_timestamp()
    omits slot timestamp
  - Drop the unreachable NULL skb test in tpacket_ubuf_complete, in
    favor of DEBUG_NET_WARN_ON_ONCE
  - Drop the tx_ring.pg_vec test in tpacket_ubuf_complete: neither
    needed nor synchronized

Verified with tools/testing/selftests/net/txring_overwrite and the RPS
setup from Bjoern's report.

---
 include/linux/skbuff.h |  19 +-------
 net/packet/af_packet.c | 102 ++++++++++++++++++++++++++---------------
 2 files changed, 65 insertions(+), 56 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..fba81036635b 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,56 @@ 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;
+
+	DEBUG_NET_WARN_ON_ONCE(!skb);
+
+	if (!refcount_dec_and_test(&uarg->refcnt))
+		return;
+
+	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 +2756,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 +2884,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 +2916,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);
 
@@ -4484,21 +4511,20 @@ static struct pgv *alloc_pg_vec(struct tpacket_req *req, int order, bool tx_ring
 	vec->len = block_nr;
 	pg_vec = vec->pg_vec;
 
+	if (tx_ring) {
+		vec->deferred = kzalloc_obj(*vec->deferred,
+					    GFP_KERNEL | __GFP_NOWARN);
+		if (!vec->deferred)
+			goto out_free_pgvec;
+		vec->deferred->vec = vec;
+		INIT_DELAYED_WORK(&vec->deferred->work,
+				  packet_free_pg_vec_work);
+	}
+
 	for (i = 0; i < block_nr; i++) {
 		pg_vec[i].buffer = alloc_one_pg_vec_page(order);
 		if (unlikely(!pg_vec[i].buffer))
 			goto out_free_pgvec;
-
-		if (tx_ring && !vec->deferred &&
-		    is_vmalloc_addr(pg_vec[i].buffer)) {
-			vec->deferred = kzalloc_obj(*vec->deferred,
-						    GFP_KERNEL | __GFP_NOWARN);
-			if (!vec->deferred)
-				goto out_free_pgvec;
-			vec->deferred->vec = vec;
-			INIT_DELAYED_WORK(&vec->deferred->work,
-					  packet_free_pg_vec_work);
-		}
 	}
 
 out:
-- 
2.55.0.1082.g2b9226bbc0-goog

Re: [PATCH net v2 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 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message mischaracterizes the defect being fixed, which
  matters because the patch carries Fixes: 5cd8d46ea156 and Cc: stable.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but after it
  start_xmit() contains two logically identical drop-and-return-…

--

virtio_net: copy zerocopy frags in start_xmit without NAPI

When napi_tx is disabled, start_xmit() now calls skb_orphan_frags() before
transmitting, so zerocopy frags that do not set SKBFL_DONT_ORPHAN are deep
copied instead of being handed to the virtqueue and then orphaned. On copy
failure the skb is dropped, counted in tx_dropped, and the new kick_vq label
is used so a pending xmit_more batch is still flushed.

A couple of questions below.

> virtio_net: copy zerocopy frags in start_xmit without NAPI
>
> 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
> completed packets will be freed.
[ ... ]
> This fixes the tpacket_snd slot reuse bug on skb_orphan for
> virtio-net, and prevents PACKET_TX_RING from running out of slots.

[Severity: Medium]
Does the leading description match the tree this patch applies to?  The
patch carries a Fixes: tag and Cc: stable, so the described failure mode is
what a backporter will look for.

In the current tree, tpacket_snd() installs a destructor:

net/packet/af_packet.c:tpacket_snd() {
	...
	skb->destructor = tpacket_destruct_skb;
	...
}

and that destructor releases the slot, drops the pending count and completes
the waiter:

net/packet/af_packet.c:tpacket_destruct_skb() {
	...
		__packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
		packet_dec_pending(&po->tx_ring);
		complete(&po->skb_completion);
	...
}

The pre-existing skb_orphan() in start_xmit() for !use_napi runs that
destructor within the same start_xmit() call, so the slot is returned
immediately.  Given that, can a PACKET_TX_RING sender actually run out of
slots or block in wait_for_completion_interruptible_timeout() before this
series?

The clause about "the tpacket_snd slot reuse bug on skb_orphan" looks like
the defect that exists today, and it is the opposite of exhaustion: because
skb_orphan() runs after xmit_skb() has already linked the ring pages into
the virtqueue, the slot is marked available to userspace while the device may
still be reading those pages.  That is silent payload corruption rather than
a stall.

The deadlock and slot-exhaustion wording appears to apply only after the
follow-up patch that converts PACKET_TX_RING to ubuf_info completion and
removes tpacket_destruct_skb().  Could the message lead with the
data-corruption case for the current tree, and keep the deadlock argument
scoped to the follow-up patch?

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e34c52d059d3..bf82ef9874ab 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3349,6 +3349,14 @@ 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);
> +		kick = !xmit_more || netif_xmit_stopped(txq);
> +		goto kick_vq;
> +	}
> +

[Severity: Low]
This isn't a bug introduced by this patch, but the new drop path and the
existing xmit_skb() failure drop path in start_xmit() now disagree on the
kick protocol.  The new path computes kick and goes to kick_vq, while the
older one returns directly:

drivers/net/virtio_net.c:start_xmit() {
	...
	err = xmit_skb(sq, skb, !use_napi);

	/* This should not happen! */
	if (unlikely(err)) {
		DEV_STATS_INC(dev, tx_fifo_errors);
		...
		DEV_STATS_INC(dev, tx_dropped);
		dev_kfree_skb_any(skb);
		return NETDEV_TX_OK;
	}
	...
}

If xmit_skb() returns an error such as -ENOMEM from the indirect descriptor
allocation on the last packet of an xmit_more batch, do the descriptors
already added by the earlier packets of that batch stay un-notified until
some later start_xmit() on the same send queue computes kick = true?  The
-ENOSPC case is covered because check_sq_full_and_disable() stops the txq and
netif_xmit_stopped(txq) then forces a kick.

Would it make sense to route the xmit_skb() failure path through the new
kick_vq label as well, so both drops behave the same?

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new per-packet `sk_wmem_alloc` charge taken in
  tpacket_snd() (`refcount_inc(&po->sk.sk_wmem_alloc)` at…
- [Medium] Documentation/networking/packet_mmap.rst still describes the
  old TX_RING contract that this patch changes.
- [Low] tpacket_snd() still carries the comments `/* skb was destructed
  already */` (net/packet/af_packet.c:2949) and `/* skb was dropped but…

--

packet: use ubuf_info completion for TX_RING packets

This converts tpacket_snd() from a custom skb->destructor
(tpacket_destruct_skb) to standard zerocopy completion, so a TX_RING slot
is only released once all references to the payload are gone or copied.
The uarg is allocated per packet and 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/

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 50cae32ae269..64b501db660a 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2530,26 +2530,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);
> -}
> -

[ ... ]

> @@ -2589,27 +2569,56 @@ 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;
> +
> +	DEBUG_NET_WARN_ON_ONCE(!skb);
> +
> +	if (!refcount_dec_and_test(&uarg->refcnt))
> +		return;
> +
> +	ts = __packet_set_timestamp(po, ph, skb);
> +	__packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);

[Severity: Medium]
Should Documentation/networking/packet_mmap.rst be updated along with this
change?  The "Transmission process" section still says:

  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.

With slot release moved into tpacket_ubuf_complete(), TP_STATUS_AVAILABLE
now means "the kernel no longer references the payload", which the commit
message says can happen before transmission:

  net/core/dev.c:deliver_skb()
    skb_orphan_frags_rx()
      skb_copy_ubufs()
        skb_zcopy_clear()
          tpacket_ubuf_complete()

The timestamp section also documents a specific recipe:

  Getting timestamps for the TX_RING works as follows: i) fill the ring
  frames, ii) call sendto() e.g. in blocking mode, iii) wait for status of
  relevant frames to be updated resp. the frame handed over to the
  application, iv) walk through the frames to pick up the individual hw/sw
  timestamps.

When the copy happens before skb_tx_timestamp(),
__packet_set_timestamp() returns 0, so the frame is handed back as
TP_STATUS_AVAILABLE with no TP_STATUS_TS_* bit and stale tp_sec/tp_nsec,
even though timestamping was requested.  Can that documented procedure
still be followed, or should the text describe the new semantics?

> +
> +	packet_dec_pending(&po->tx_ring);
> +	complete(&po->skb_completion);
> +
> +	kfree(tu);
> +	sk_free(&po->sk);
> +}

[Severity: Medium]
Does releasing the wmem charge through sk_free() here miss a write space
notification?

sk_free() only does the decrement:

net/core/sock.c:sk_free() {
	if (refcount_dec_and_test(&sk->sk_wmem_alloc))
		__sk_free(sk);
}

whereas the skb charge release path notifies first:

net/core/sock.c:sock_wfree() {
	...
		WARN_ON(refcount_sub_and_test(len - 1, &sk->sk_wmem_alloc));
		sk_write_space(sk);
		len = 1;
	...
}

A blocking TX_RING sender reaches sock_wait_for_wmem() via
tpacket_snd()->sock_alloc_send_skb()->sock_alloc_send_pskb(), and its only
wake condition for send space is sk->sk_write_space():

net/core/sock.c:sock_wait_for_wmem() {
	...
		if (refcount_read(&sk->sk_wmem_alloc) < READ_ONCE(sk->sk_sndbuf))
			break;
		...
		timeo = schedule_timeout(timeo);
	...
}

With the default sk_sndtimeo that schedule_timeout() is
MAX_SCHEDULE_TIMEOUT.  If the drop below sk_sndbuf is performed by the new
uarg charges only, is anything left to wake the sleeper?  A path where the
skb charge is gone while the uarg charge is still outstanding is
loopback_xmit(), which calls skb_orphan(skb) (running sock_wfree) without
skb_orphan_frags(), so the ubuf_info survives in the backlog.

[ ... ]

> @@ -2908,7 +2918,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);
>  

[Severity: Low]
Just below this, the packet_xmit() error handling in tpacket_snd() still
refers to the destructor that this patch removes:

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
			 */
	...
}

With skb->destructor back to sock_wfree and slot release moved to
tpacket_ubuf_complete(), does the slot status still imply anything about
the skb?  TP_STATUS_AVAILABLE can now be observed while the skb is alive
(the uarg completed early through skb_copy_ubufs()), and the slot can
still read TP_STATUS_SENDING after the skb was freed if a clone or segment
holds an extra uarg reference.  Should these comments be reworded for the
new completion scheme?

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

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

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] Commit message mischaracterizes the defect being fixed, which
>   matters because the patch carries Fixes: 5cd8d46ea156 and Cc: stable.
> 
> Pre-existing issues:
> - [Low] This problem wasn't introduced by this patch, but after it
>   start_xmit() contains two logically identical drop-and-return-…
> 
> --
> 
> virtio_net: copy zerocopy frags in start_xmit without NAPI
> 
> When napi_tx is disabled, start_xmit() now calls skb_orphan_frags() before
> transmitting, so zerocopy frags that do not set SKBFL_DONT_ORPHAN are deep
> copied instead of being handed to the virtqueue and then orphaned. On copy
> failure the skb is dropped, counted in tx_dropped, and the new kick_vq label
> is used so a pending xmit_more batch is still flushed.
> 
> A couple of questions below.
> 
> > virtio_net: copy zerocopy frags in start_xmit without NAPI
> >
> > 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
> > completed packets will be freed.
> [ ... ]
> > This fixes the tpacket_snd slot reuse bug on skb_orphan for
> > virtio-net, and prevents PACKET_TX_RING from running out of slots.
> 
> [Severity: Medium]
> Does the leading description match the tree this patch applies to?  The
> patch carries a Fixes: tag and Cc: stable, so the described failure mode is
> what a backporter will look for.
> 
> In the current tree, tpacket_snd() installs a destructor:
> 
> net/packet/af_packet.c:tpacket_snd() {
> 	...
> 	skb->destructor = tpacket_destruct_skb;
> 	...
> }
> 
> and that destructor releases the slot, drops the pending count and completes
> the waiter:
> 
> net/packet/af_packet.c:tpacket_destruct_skb() {
> 	...
> 		__packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
> 		packet_dec_pending(&po->tx_ring);
> 		complete(&po->skb_completion);
> 	...
> }
> 
> The pre-existing skb_orphan() in start_xmit() for !use_napi runs that
> destructor within the same start_xmit() call, so the slot is returned
> immediately.  Given that, can a PACKET_TX_RING sender actually run out of
> slots or block in wait_for_completion_interruptible_timeout() before this
> series?
> 
> The clause about "the tpacket_snd slot reuse bug on skb_orphan" looks like
> the defect that exists today, and it is the opposite of exhaustion: because
> skb_orphan() runs after xmit_skb() has already linked the ring pages into
> the virtqueue, the slot is marked available to userspace while the device may
> still be reading those pages.  That is silent payload corruption rather than
> a stall.
> 
> The deadlock and slot-exhaustion wording appears to apply only after the
> follow-up patch that converts PACKET_TX_RING to ubuf_info completion and
> removes tpacket_destruct_skb().  Could the message lead with the
> data-corruption case for the current tree, and keep the deadlock argument
> scoped to the follow-up patch?

[Medium] the request is only to change the emphasis of the wording
of the commit message. No code changes suggested:

Focus more on the data corruption bug and less on the deadlock that
can occur when not releasing the slots prematurely (that data
corruption).

See this statement in the commit

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

Maybe it should be moved up and say "while preventing".

Seems a bit of a pedantic comment to me, honestly.

I will reword if the other patch requires a respin, but this does not
warrant one on its own imho.

> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index e34c52d059d3..bf82ef9874ab 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -3349,6 +3349,14 @@ 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);
> > +		kick = !xmit_more || netif_xmit_stopped(txq);
> > +		goto kick_vq;
> > +	}
> > +
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but the new drop path and the
> existing xmit_skb() failure drop path in start_xmit() now disagree on the
> kick protocol.  The new path computes kick and goes to kick_vq, while the
> older one returns directly:
> 
> drivers/net/virtio_net.c:start_xmit() {
> 	...
> 	err = xmit_skb(sq, skb, !use_napi);
> 
> 	/* This should not happen! */
> 	if (unlikely(err)) {
> 		DEV_STATS_INC(dev, tx_fifo_errors);
> 		...
> 		DEV_STATS_INC(dev, tx_dropped);
> 		dev_kfree_skb_any(skb);
> 		return NETDEV_TX_OK;
> 	}
> 	...
> }
> 
> If xmit_skb() returns an error such as -ENOMEM from the indirect descriptor
> allocation on the last packet of an xmit_more batch, do the descriptors
> already added by the earlier packets of that batch stay un-notified until
> some later start_xmit() on the same send queue computes kick = true?  The
> -ENOSPC case is covered because check_sq_full_and_disable() stops the txq and
> netif_xmit_stopped(txq) then forces a kick.
> 
> Would it make sense to route the xmit_skb() failure path through the new
> kick_vq label as well, so both drops behave the same?

[Low] but I'll respond: pre-existing issue is out of scope here. But
probably makes sense as a separate fix.

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

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 2 · Low: 1
> 

> [Severity: Medium]
> Should Documentation/networking/packet_mmap.rst be updated along with this
> change?  The "Transmission process" section still says:
> 
>   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.
> 
> With slot release moved into tpacket_ubuf_complete(), TP_STATUS_AVAILABLE
> now means "the kernel no longer references the payload", which the commit
> message says can happen before transmission:
> 
>   net/core/dev.c:deliver_skb()
>     skb_orphan_frags_rx()
>       skb_copy_ubufs()
>         skb_zcopy_clear()
>           tpacket_ubuf_complete()

[Medium] This is a pre-existing issue.

The slot can be returned prior to the end of transfer after this
change. But the same was true before this change with skb_orphan().

I can improve this documentation in a follow-up to net-next. But I
don't think it belongs in a stable patch.

> 
> The timestamp section also documents a specific recipe:
> 
>   Getting timestamps for the TX_RING works as follows: i) fill the ring
>   frames, ii) call sendto() e.g. in blocking mode, iii) wait for status of
>   relevant frames to be updated resp. the frame handed over to the
>   application, iv) walk through the frames to pick up the individual hw/sw
>   timestamps.
> 
> When the copy happens before skb_tx_timestamp(),
> __packet_set_timestamp() returns 0, so the frame is handed back as
> TP_STATUS_AVAILABLE with no TP_STATUS_TS_* bit and stale tp_sec/tp_nsec,
> even though timestamping was requested.  Can that documented procedure
> still be followed, or should the text describe the new semantics?
> 
> > +
> > +	packet_dec_pending(&po->tx_ring);
> > +	complete(&po->skb_completion);
> > +
> > +	kfree(tu);
> > +	sk_free(&po->sk);
> > +}
> 
> [Severity: Medium]
> Does releasing the wmem charge through sk_free() here miss a write space
> notification?
> 
> sk_free() only does the decrement:
> 
> net/core/sock.c:sk_free() {
> 	if (refcount_dec_and_test(&sk->sk_wmem_alloc))
> 		__sk_free(sk);
> }
> 
> whereas the skb charge release path notifies first:
> 
> net/core/sock.c:sock_wfree() {
> 	...
> 		WARN_ON(refcount_sub_and_test(len - 1, &sk->sk_wmem_alloc));
> 		sk_write_space(sk);
> 		len = 1;
> 	...
> }

[Medium] False positive.

This sk_free only releases a 1 byte hold for the slot.

The actual release of the truesize attributed to the data (to_write)
is released in the normal sock_wfree path.

It is admittedly odd to have two wmem operations per slot. And perhaps
even questionable to have truesize and wmem count userspace TX_RING
data. But the second is pre-existing and for net-next.

The first is new, but alternatives like sock_hold were found to be
less preferable than an extra 1B hold on wmem.

> 
> A blocking TX_RING sender reaches sock_wait_for_wmem() via
> tpacket_snd()->sock_alloc_send_skb()->sock_alloc_send_pskb(), and its only
> wake condition for send space is sk->sk_write_space():
> 
> net/core/sock.c:sock_wait_for_wmem() {
> 	...
> 		if (refcount_read(&sk->sk_wmem_alloc) < READ_ONCE(sk->sk_sndbuf))
> 			break;
> 		...
> 		timeo = schedule_timeout(timeo);
> 	...
> }
> 
> With the default sk_sndtimeo that schedule_timeout() is
> MAX_SCHEDULE_TIMEOUT.  If the drop below sk_sndbuf is performed by the new
> uarg charges only, is anything left to wake the sleeper?  A path where the
> skb charge is gone while the uarg charge is still outstanding is
> loopback_xmit(), which calls skb_orphan(skb) (running sock_wfree) without
> skb_orphan_frags(), so the ubuf_info survives in the backlog.
> 
> [ ... ]
> 
> > @@ -2908,7 +2918,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);
> >  
> 
> [Severity: Low]
> Just below this, the packet_xmit() error handling in tpacket_snd() still
> refers to the destructor that this patch removes:
> 
> 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
> 			 */
> 	...
> }
> 
> With skb->destructor back to sock_wfree and slot release moved to
> tpacket_ubuf_complete(), does the slot status still imply anything about
> the skb?  TP_STATUS_AVAILABLE can now be observed while the skb is alive
> (the uarg completed early through skb_copy_ubufs()), and the slot can
> still read TP_STATUS_SENDING after the skb was freed if a clone or segment
> holds an extra uarg reference.  Should these comments be reworded for the
> new completion scheme?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919004748.1463985-1-willemdebruijn.kernel%40gmail.com

Re: [PATCH net v2 0/2] packet: fix PACKET_TX_RING data corruption on skb_orphan

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 18 Sep 2026 20:47:27 -0400 you wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> When transmitting packets via PACKET_TX_RING, tpacket_snd links user
> ring buffer pages as skb frags and releases the slot on skb->destructor
> (tpacket_destruct_skb).
> 
> skb_orphan() invokes the destructor while the skb is still alive.
> This marks the slot as TP_STATUS_AVAILABLE prematurely, allowing
> userspace to overwrite the slot and causing data corruption.
> 
> [...]

Here is the summary with links:
  - [net,v2,1/2] virtio_net: copy zerocopy frags in start_xmit without NAPI
    https://git.kernel.org/netdev/net/c/07e1a9408b6c
  - [net,v2,2/2] packet: use ubuf_info completion for TX_RING packets
    https://git.kernel.org/netdev/net/c/951840561386

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html