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

[netdev-bot+sashiko@kernel.org]

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