[Qemu-devel] [PATCHv3] virtio-net: correct packet length math

[Qemu-devel] [PATCHv3] virtio-net: correct packet length math

[Michael S. Tsirkin]

We were requesting too much when checking buffer
length: size already includes host header length.

Further, we should not exit if we get a packet that
is too long, since this might not be under control
of the guest. Just drop the packet.

Red Hat bz 591494

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Changes from v2:
	fixed format warning.
Changes from v1:
	drop packet instead of exit.

 hw/virtio-net.c |   41 ++++++++++++++++++++++++++++-------------
 1 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 06ba481..1018c32 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -527,17 +527,18 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
 {
     VirtIONet *n = DO_UPCAST(NICState, nc, nc)->opaque;
     struct virtio_net_hdr_mrg_rxbuf *mhdr = NULL;
-    size_t hdr_len, offset, i;
+    size_t guest_hdr_len, offset, i, host_hdr_len;
 
     if (!virtio_net_can_receive(&n->nic->nc))
         return -1;
 
     /* hdr_len refers to the header we supply to the guest */
-    hdr_len = n->mergeable_rx_bufs ?
+    guest_hdr_len = n->mergeable_rx_bufs ?
         sizeof(struct virtio_net_hdr_mrg_rxbuf) : sizeof(struct virtio_net_hdr);
 
 
-    if (!virtio_net_has_buffers(n, size + hdr_len))
+    host_hdr_len = n->has_vnet_hdr ? sizeof(struct virtio_net_hdr) : 0;
+    if (!virtio_net_has_buffers(n, size + guest_hdr_len - host_hdr_len))
         return 0;
 
     if (!receive_filter(n, buf, size))
@@ -552,13 +553,14 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
 
         total = 0;
 
-        if ((i != 0 && !n->mergeable_rx_bufs) ||
-            virtqueue_pop(n->rx_vq, &elem) == 0) {
+        if (virtqueue_pop(n->rx_vq, &elem) == 0) {
             if (i == 0)
                 return -1;
-            fprintf(stderr, "virtio-net truncating packet: "
-                    "offset %zd, size %zd, hdr_len %zd\n",
-                    offset, size, hdr_len);
+            fprintf(stderr, "virtio-net unexpected empty queue: "
+                    "i %zd mergeable %d offset %zd, size %zd, "
+                    "guest hdr len %zd, host hdr len %zd guest features 0x%x\n",
+                    i, n->mergeable_rx_bufs, offset, size,
+                    guest_hdr_len, host_hdr_len, n->vdev.guest_features);
             exit(1);
         }
 
@@ -567,7 +569,7 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
             exit(1);
         }
 
-        if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != hdr_len) {
+        if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != guest_hdr_len) {
             fprintf(stderr, "virtio-net header not in first element\n");
             exit(1);
         }
@@ -579,19 +581,32 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
                 mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base;
 
             offset += receive_header(n, sg, elem.in_num,
-                                     buf + offset, size - offset, hdr_len);
-            total += hdr_len;
+                                     buf + offset, size - offset, guest_hdr_len);
+            total += guest_hdr_len;
         }
 
         /* copy in packet.  ugh */
         len = iov_from_buf(sg, elem.in_num,
                            buf + offset, size - offset);
         total += len;
+        offset += len;
+        /* If buffers can't be merged, at this point we
+         * must have consumed the complete packet.
+         * Otherwise, drop it. */
+        if (!n->mergeable_rx_bufs && offset < size) {
+#if 0
+            fprintf(stderr, "virtio-net truncated non-mergeable packet: "
+
+                    "i %zd mergeable %d offset %zd, size %zd, "
+                    "guest hdr len %zd, host hdr len %zd\n",
+                    i, n->mergeable_rx_bufs,
+                    offset, size, guest_hdr_len, host_hdr_len);
+#endif
+            return size;
+        }
 
         /* signal other side */
         virtqueue_fill(n->rx_vq, &elem, total, i++);
-
-        offset += len;
     }
 
     if (mhdr)
-- 
1.7.1.12.g42b7f

[Qemu-devel] Re: [PATCHv3] virtio-net: correct packet length math

[Amit Shah]

On (Thu) Jun 24 2010 [18:54:07], Michael S. Tsirkin wrote:
> We were requesting too much when checking buffer
> length: size already includes host header length.
> 
> Further, we should not exit if we get a packet that
> is too long, since this might not be under control
> of the guest. Just drop the packet.

control of the host?

> @@ -579,19 +581,32 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
>                  mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base;
>  
>              offset += receive_header(n, sg, elem.in_num,
> -                                     buf + offset, size - offset, hdr_len);
> -            total += hdr_len;
> +                                     buf + offset, size - offset, guest_hdr_len);
> +            total += guest_hdr_len;
>          }
>  
>          /* copy in packet.  ugh */
>          len = iov_from_buf(sg, elem.in_num,
>                             buf + offset, size - offset);
>          total += len;
> +        offset += len;
> +        /* If buffers can't be merged, at this point we
> +         * must have consumed the complete packet.
> +         * Otherwise, drop it. */
> +        if (!n->mergeable_rx_bufs && offset < size) {
> +#if 0
> +            fprintf(stderr, "virtio-net truncated non-mergeable packet: "
> +
> +                    "i %zd mergeable %d offset %zd, size %zd, "
> +                    "guest hdr len %zd, host hdr len %zd\n",
> +                    i, n->mergeable_rx_bufs,
> +                    offset, size, guest_hdr_len, host_hdr_len);
> +#endif
> +            return size;
> +        }

Before returning, won't you have to finish off the virtqueue operations
-- fill, flush, kick, etc.?

		Amit

[Qemu-devel] Re: [PATCHv3] virtio-net: correct packet length math

[Michael S. Tsirkin]

On Fri, Jun 25, 2010 at 12:47:03PM +0530, Amit Shah wrote:
> On (Thu) Jun 24 2010 [18:54:07], Michael S. Tsirkin wrote:
> > We were requesting too much when checking buffer
> > length: size already includes host header length.
> > 
> > Further, we should not exit if we get a packet that
> > is too long, since this might not be under control
> > of the guest. Just drop the packet.
> 
> control of the host?

Well, host too I guess. What I was trying to say, it might not
be the fault of the guest that it got a packet
that is too long.

> > @@ -579,19 +581,32 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_
> >                  mhdr = (struct virtio_net_hdr_mrg_rxbuf *)sg[0].iov_base;
> >  
> >              offset += receive_header(n, sg, elem.in_num,
> > -                                     buf + offset, size - offset, hdr_len);
> > -            total += hdr_len;
> > +                                     buf + offset, size - offset, guest_hdr_len);
> > +            total += guest_hdr_len;
> >          }
> >  
> >          /* copy in packet.  ugh */
> >          len = iov_from_buf(sg, elem.in_num,
> >                             buf + offset, size - offset);
> >          total += len;
> > +        offset += len;
> > +        /* If buffers can't be merged, at this point we
> > +         * must have consumed the complete packet.
> > +         * Otherwise, drop it. */
> > +        if (!n->mergeable_rx_bufs && offset < size) {
> > +#if 0
> > +            fprintf(stderr, "virtio-net truncated non-mergeable packet: "
> > +
> > +                    "i %zd mergeable %d offset %zd, size %zd, "
> > +                    "guest hdr len %zd, host hdr len %zd\n",
> > +                    i, n->mergeable_rx_bufs,
> > +                    offset, size, guest_hdr_len, host_hdr_len);
> > +#endif
> > +            return size;
> > +        }
> 
> Before returning, won't you have to finish off the virtqueue operations
> -- fill, flush, kick, etc.?
> 
> 		Amit

No, this would consume a buffer. We just want to drop the packet
without side-effects. But we do need to decrement the inuse counter.
I'll respin the patch.

-- 
MST