Re: [PATCH 1/1] virtio-net: fix bug 1451 aka "assert(!virtio_net_get_subqueue(nc)->async_tx.elem);"

[Alexey Dobriyan <adobriyan@gmail.com>]

On Mon, Apr 08, 2024 at 03:26:35PM +0800, Jason Wang wrote:
> On Fri, Apr 5, 2024 at 7:22 PM Alexey Dobriyan <adobriyan@yandex-team.ru> wrote:
> >
> > Don't send zero length packets in virtio_net_flush_tx().
> >
> > Reproducer from https://gitlab.com/qemu-project/qemu/-/issues/1451
> > creates small packet (1 segment, len = 10 == n->guest_hdr_len),
> > destroys queue.
> >
> > "if (n->host_hdr_len != n->guest_hdr_len)" is triggered, if body creates
> > zero length/zero segment packet, because there is nothing after guest
> > header.
> 
> And in this case host_hdr_len is 0.

Yes.

> > qemu_sendv_packet_async() tries to send it.
> >
> > slirp discards it because it is smaller than Ethernet header,
> > but returns 0.
> >
> > 0 length is propagated upwards and is interpreted as "packet has been sent"
> > which is terrible because queue is being destroyed, nothing has been sent,
> > nobody is waiting for TX to complete and assert it triggered.

> > @@ -2807,6 +2803,11 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >                               n->guest_hdr_len, -1);
> >              out_num = sg_num;
> >              out_sg = sg;
> > +
> > +            if (iov_size(out_sg, out_num) == 0) {
> > +                virtio_error(vdev, "virtio-net nothing to send");
> > +                goto detach;
> > +            }
> 
> Nit, I think we can do this check before the iov_copy()?

I'd rather check for "badness" right before emitting packet.

> >          }
> >
> >          ret = qemu_sendv_packet_async(qemu_get_subqueue(n->nic, queue_index),
> > @@ -2827,6 +2828,11 @@ drop:
> >          }
> >      }
> >      return num_packets;
> > +
> > +detach:
> > +    virtqueue_detach_element(q->tx_vq, elem, 0);
> > +    g_free(elem);
> > +    return -EINVAL;
> >  }
> >
> >  static void virtio_net_tx_timer(void *opaque);
> > --
> > 2.34.1
> >
>