Re: [PATCH net 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

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