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

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

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.