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

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/ (local)

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?

[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?

[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,
+};
+

[ ... ]

[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