On Mon, Apr 08, 2024 at 03:26:35PM +0800, Jason Wang wrote:
> On Fri, Apr 5, 2024 at 7:22 PM Alexey Dobriyan <adobri...@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
> >
>
