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

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

[Alexey Dobriyan]

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.

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

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

[Jason Wang]

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
>

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

[Alexey Dobriyan]

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

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

[Michael S. Tsirkin]

On Fri, Apr 05, 2024 at 02:20:15PM +0300, Alexey Dobriyan 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.
> 
> 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) {

calling iov_size an extra time on data path and scanning
a potentially long s/g list just so we can check
it's not 0 is not welcome, though.


won't the previous iov_copy return 0 in this case?

> +                virtio_error(vdev, "virtio-net nothing to send");
> +                goto detach;
> +            }
>          }
>  
>          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

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

[Michael S. Tsirkin]

On Fri, Apr 05, 2024 at 02:20:15PM +0300, Alexey Dobriyan 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.
> 
> qemu_sendv_packet_async() tries to send it.
> 
> slirp discards it because it is smaller than Ethernet header,
> but returns 0.

won't the same issue trigger with a 1 byte packet
as opposed to a 0 byte packet though?



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

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

[Alexey Dobriyan]

On Tue, Apr 09, 2024 at 02:51:38AM -0400, Michael S. Tsirkin wrote:
> On Fri, Apr 05, 2024 at 02:20:15PM +0300, Alexey Dobriyan 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.
> > 
> > qemu_sendv_packet_async() tries to send it.
> > 
> > slirp discards it because it is smaller than Ethernet header,
> > but returns 0.
> 
> won't the same issue trigger with a 1 byte packet
> as opposed to a 0 byte packet though?

I don't think so. Only "return 0" is problematic from qemu_sendv_packet_async().
->deliver hooks are supposed to return total length, so 1 should be fine.

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

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

[Michael S. Tsirkin]

On Tue, Apr 09, 2024 at 07:37:04PM +0300, Alexey Dobriyan wrote:
> On Tue, Apr 09, 2024 at 02:51:38AM -0400, Michael S. Tsirkin wrote:
> > On Fri, Apr 05, 2024 at 02:20:15PM +0300, Alexey Dobriyan 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.
> > > 
> > > qemu_sendv_packet_async() tries to send it.
> > > 
> > > slirp discards it because it is smaller than Ethernet header,
> > > but returns 0.
> > 
> > won't the same issue trigger with a 1 byte packet
> > as opposed to a 0 byte packet though?
> 
> I don't think so. Only "return 0" is problematic from qemu_sendv_packet_async().
> ->deliver hooks are supposed to return total length, so 1 should be fine.


But you said it's smaller than Ethernet is the problem?

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

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

[Alexey Dobriyan]

On Tue, Apr 09, 2024 at 12:41:39PM -0400, Michael S. Tsirkin wrote:
> On Tue, Apr 09, 2024 at 07:37:04PM +0300, Alexey Dobriyan wrote:
> > On Tue, Apr 09, 2024 at 02:51:38AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Apr 05, 2024 at 02:20:15PM +0300, Alexey Dobriyan 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.
> > > > 
> > > > qemu_sendv_packet_async() tries to send it.
> > > > 
> > > > slirp discards it because it is smaller than Ethernet header,
> > > > but returns 0.
> > > 
> > > won't the same issue trigger with a 1 byte packet
> > > as opposed to a 0 byte packet though?
> > 
> > I don't think so. Only "return 0" is problematic from qemu_sendv_packet_async().
> > ->deliver hooks are supposed to return total length, so 1 should be fine.
> 
> 
> But you said it's smaller than Ethernet is the problem?

No, the problem is iov_cnt=0 packet is being generated, which enters
qemu_sendv_packet_async(), discarded inside slirp driver, but
net_slirp_receive() returns 0 which is the length of meaningless packet.
which is propagated upwards.

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

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

[Alexey Dobriyan]

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. There is
nothing after guest header, if body creates zero length/zero segment packet.

qemu_sendv_packet_async() tries to send it.

Packet is discarded inside net_slirp_receive() which returns number of
sent bytes which is 0.

0 is propagated upwards and is interpreted as "zero bytes has been sent"
which is terrible because queue is being destroyed, nobody is waiting
for TX to complete, TX complete hook is _not_ called and
assert(async_tx.elem) is eventually triggered.

The fix is to discard such phantom packets in virtio_net_flush_tx().

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..e156f1d78c 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 (out_num < 1) {
+                virtio_error(vdev, "virtio-net nothing to send");
+                goto detach;
+            }
         }
 
         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.43.0