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

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

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