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

[Jason Wang <jasowang@redhat.com>]

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.

>
> 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.
>
> Signed-off-by: Alexey Dobriyan <adobriyan@yandex-team.ru>
> ---
>  hw/net/virtio-net.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 58014a92ad..258633f885 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2765,18 +2765,14 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>          out_sg = elem->out_sg;
>          if (out_num < 1) {
>              virtio_error(vdev, "virtio-net header not in first element");
> -            virtqueue_detach_element(q->tx_vq, elem, 0);
> -            g_free(elem);
> -            return -EINVAL;
> +            goto detach;
>          }
>
>          if (n->has_vnet_hdr) {
>              if (iov_to_buf(out_sg, out_num, 0, &vhdr, n->guest_hdr_len) <
>                  n->guest_hdr_len) {
>                  virtio_error(vdev, "virtio-net header incorrect");
> -                virtqueue_detach_element(q->tx_vq, elem, 0);
> -                g_free(elem);
> -                return -EINVAL;
> +                goto detach;
>              }
>              if (n->needs_vnet_hdr_swap) {
>                  virtio_net_hdr_swap(vdev, (void *) &vhdr);
> @@ -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()?

Thanks

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