[Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states

[Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states

[Stefan Hajnoczi]

v3:
 * Patch 1: Fix typo and clarify commit description [Markus]
 * Use virtio_set_status() instead of open coding assignment [Cornelia]
 * Add live migration

v2:
 * Add VIRTIO_CONFIG_S_NEEDS_RESET notification for VIRTIO 1.0 [Cornelia]
   (Note I've sent a Linux virtio_config.h patch to get the constant added to
   the headers.)
 * Split int -> unsigned int change into separate commit [Fam]
 * Fix double "index" typo in commit description [Fam]

The virtio code calls exit() when the device enters an invalid state.  This
means invalid vring indices and descriptor chains kill the VM.  See the patch
descriptions for why this is a bad thing.

When the virtio device is in the broken state calls to virtqueue_pop() and
friends will pretend the virtqueue is empty.  This means the device will become
isolated from guest activity until it is reset again.

RFC because two things are missing:
1. Live migration support (subsection for broken flag?)
2. Auditing devices and replacing exit() calls there too

Stefan Hajnoczi (10):
  virtio: fix stray tab character
  include: update virtio_config.h Linux header
  virtio: stop virtqueue processing if device is broken
  virtio: migrate vdev->broken flag
  virtio: handle virtqueue_map_desc() errors
  virtio: handle virtqueue_get_avail_bytes() errors
  virtio: use unsigned int for virtqueue_get_avail_bytes() index
  virtio: handle virtqueue_read_next_desc() errors
  virtio: handle virtqueue_num_heads() errors
  virtio: handle virtqueue_get_head() errors

 hw/virtio/virtio.c                             | 223 +++++++++++++++++++------
 include/hw/virtio/virtio.h                     |   3 +
 include/standard-headers/linux/virtio_config.h |   2 +
 3 files changed, 181 insertions(+), 47 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v3 01/10] virtio: fix stray tab character

[Stefan Hajnoczi]

Fix a single occurrence of a tab character in a file that otherwise uses
spaces for indentation.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
v3:
 * Fix typo and clarify commit description [Markus]
---
 hw/virtio/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f745c4a..02c71a2 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1545,7 +1545,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
                          "inconsistent with Host index 0x%x",
                          i, vdev->vq[i].last_avail_idx);
                 return -1;
-	}
+        }
         if (k->load_queue) {
             ret = k->load_queue(qbus->parent, i, f);
             if (ret)
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 01/10] virtio: fix stray tab character

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:25 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Fix a single occurrence of a tab character in a file that otherwise uses
> spaces for indentation.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> v3:
>  * Fix typo and clarify commit description [Markus]
> ---
>  hw/virtio/virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH v3 02/10] include: update virtio_config.h Linux header

[Stefan Hajnoczi]

Bring in the VIRTIO_CONFIG_S_NEEDS_RESET device status bit definition.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/standard-headers/linux/virtio_config.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h
index bcc445b..b30d0cb 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -40,6 +40,8 @@
 #define VIRTIO_CONFIG_S_DRIVER_OK	4
 /* Driver has finished configuring features */
 #define VIRTIO_CONFIG_S_FEATURES_OK	8
+/* Device entered invalid state, driver must reset it */
+#define VIRTIO_CONFIG_S_NEEDS_RESET	0x40
 /* We've given up on this device. */
 #define VIRTIO_CONFIG_S_FAILED		0x80
 
-- 
2.5.5

[Qemu-devel] [PATCH v3 03/10] virtio: stop virtqueue processing if device is broken

[Stefan Hajnoczi]

QEMU prints an error message and exits when the device enters an invalid
state.  Terminating the process is heavy-handed.  The guest may still be
able to function even if there is a bug in a virtio guest driver.

Moreover, exiting is a bug in nested virtualization where a nested guest
could DoS other nested guests by killing a pass-through virtio device.
I don't think this configuration is possible today but it is likely in
the future.

If the broken flag is set, do not process virtqueues or write back used
descriptors.  The broken flag can be cleared again by resetting the
device.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c         | 39 +++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 02c71a2..cedda4b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -279,6 +279,10 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 
     virtqueue_unmap_sg(vq, elem, len);
 
+    if (unlikely(vq->vdev->broken)) {
+        return;
+    }
+
     idx = (idx + vq->used_idx) % vq->vring.num;
 
     uelem.id = elem->index;
@@ -289,6 +293,12 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 void virtqueue_flush(VirtQueue *vq, unsigned int count)
 {
     uint16_t old, new;
+
+    if (unlikely(vq->vdev->broken)) {
+        vq->inuse -= count;
+        return;
+    }
+
     /* Make sure buffer is written before we update index. */
     smp_wmb();
     trace_virtqueue_flush(vq, count);
@@ -549,6 +559,9 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     struct iovec iov[VIRTQUEUE_MAX_SIZE];
     VRingDesc desc;
 
+    if (unlikely(vdev->broken)) {
+        return NULL;
+    }
     if (virtio_queue_empty(vq)) {
         return NULL;
     }
@@ -708,6 +721,10 @@ static void virtio_notify_vector(VirtIODevice *vdev, uint16_t vector)
     BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
 
+    if (unlikely(vdev->broken)) {
+        return;
+    }
+
     if (k->notify) {
         k->notify(qbus->parent, vector);
     }
@@ -791,6 +808,7 @@ void virtio_reset(void *opaque)
         k->reset(vdev);
     }
 
+    vdev->broken = false;
     vdev->guest_features = 0;
     vdev->queue_sel = 0;
     vdev->status = 0;
@@ -1104,6 +1122,10 @@ static void virtio_queue_notify_vq(VirtQueue *vq)
     if (vq->vring.desc && vq->handle_output) {
         VirtIODevice *vdev = vq->vdev;
 
+        if (unlikely(vdev->broken)) {
+            return;
+        }
+
         trace_virtio_queue_notify(vdev, vq - vdev->vq, vq);
         vq->handle_output(vdev, vq);
     }
@@ -1675,6 +1697,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
     vdev->config_vector = VIRTIO_NO_VECTOR;
     vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_QUEUE_MAX);
     vdev->vm_running = runstate_is_running();
+    vdev->broken = false;
     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
         vdev->vq[i].vector = VIRTIO_NO_VECTOR;
         vdev->vq[i].vdev = vdev;
@@ -1852,6 +1875,22 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
     vdev->bus_name = g_strdup(bus_name);
 }
 
+void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_vreport(fmt, ap);
+    va_end(ap);
+
+    vdev->broken = true;
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
+        virtio_set_status(vdev, vdev->status | VIRTIO_CONFIG_S_NEEDS_RESET);
+        virtio_notify_config(vdev);
+    }
+}
+
 static void virtio_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 6a37065..012b0d1 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -87,6 +87,7 @@ struct VirtIODevice
     VirtQueue *vq;
     uint16_t device_id;
     bool vm_running;
+    bool broken; /* device in invalid state, needs reset */
     VMChangeStateEntry *vmstate;
     char *bus_name;
     uint8_t device_endian;
@@ -135,6 +136,8 @@ void virtio_init(VirtIODevice *vdev, const char *name,
                          uint16_t device_id, size_t config_size);
 void virtio_cleanup(VirtIODevice *vdev);
 
+void virtio_error(VirtIODevice *vdev, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+
 /* Set the child bus name. */
 void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
 
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 03/10] virtio: stop virtqueue processing if device is broken

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:27 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> QEMU prints an error message and exits when the device enters an invalid
> state.  Terminating the process is heavy-handed.  The guest may still be
> able to function even if there is a bug in a virtio guest driver.
> 
> Moreover, exiting is a bug in nested virtualization where a nested guest
> could DoS other nested guests by killing a pass-through virtio device.
> I don't think this configuration is possible today but it is likely in
> the future.
> 
> If the broken flag is set, do not process virtqueues or write back used
> descriptors.  The broken flag can be cleared again by resetting the
> device.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c         | 39 +++++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h |  3 +++
>  2 files changed, 42 insertions(+)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH v3 04/10] virtio: migrate vdev->broken flag

[Stefan Hajnoczi]

Send a subsection if the vdev->broken flag is set.  This allows live
migration of broken virtio devices.

The subsection is only sent if vdev->broken has been set.  In most cases
the flag will be clear and no subsection will be sent.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index cedda4b..6e6b968 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1291,6 +1291,13 @@ static bool virtio_extra_state_needed(void *opaque)
         k->has_extra_state(qbus->parent);
 }
 
+static bool virtio_broken_needed(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    return vdev->broken;
+}
+
 static const VMStateDescription vmstate_virtqueue = {
     .name = "virtqueue_state",
     .version_id = 1,
@@ -1405,6 +1412,17 @@ static const VMStateDescription vmstate_virtio_64bit_features = {
     }
 };
 
+static const VMStateDescription vmstate_virtio_broken = {
+    .name = "virtio/broken",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_broken_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(broken, VirtIODevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio = {
     .name = "virtio",
     .version_id = 1,
@@ -1418,6 +1436,7 @@ static const VMStateDescription vmstate_virtio = {
         &vmstate_virtio_64bit_features,
         &vmstate_virtio_virtqueues,
         &vmstate_virtio_ringsize,
+        &vmstate_virtio_broken,
         &vmstate_virtio_extra_state,
         NULL
     }
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 04/10] virtio: migrate vdev->broken flag

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:28 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Send a subsection if the vdev->broken flag is set.  This allows live
> migration of broken virtio devices.
> 
> The subsection is only sent if vdev->broken has been set.  In most cases
> the flag will be clear and no subsection will be sent.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH v3 05/10] virtio: handle virtqueue_map_desc() errors

[Stefan Hajnoczi]

Errors can occur during virtqueue_pop(), especially in
virtqueue_map_desc().  In order to handle this we must unmap iov[]
before returning NULL.  The caller will consider the virtqueue empty and
the virtio_error() call will have marked the device broken.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 62 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 13 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 6e6b968..0c0d333 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -460,10 +460,12 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
     return in_bytes <= in_total && out_bytes <= out_total;
 }
 
-static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iovec *iov,
+static bool virtqueue_map_desc(VirtIODevice *vdev, unsigned int *p_num_sg,
+                               hwaddr *addr, struct iovec *iov,
                                unsigned int max_num_sg, bool is_write,
                                hwaddr pa, size_t sz)
 {
+    bool ok = false;
     unsigned num_sg = *p_num_sg;
     assert(num_sg <= max_num_sg);
 
@@ -471,8 +473,9 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
         hwaddr len = sz;
 
         if (num_sg == max_num_sg) {
-            error_report("virtio: too many write descriptors in indirect table");
-            exit(1);
+            virtio_error(vdev, "virtio: too many write descriptors in "
+                               "indirect table");
+            goto out;
         }
 
         iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write);
@@ -483,7 +486,28 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
         pa += len;
         num_sg++;
     }
+    ok = true;
+
+out:
     *p_num_sg = num_sg;
+    return ok;
+}
+
+/* Only used by error code paths before we have a VirtQueueElement (therefore
+ * virtqueue_unmap_sg() can't be used).  Assumes buffers weren't written to
+ * yet.
+ */
+static void virtqueue_undo_map_desc(unsigned out_num, unsigned in_num,
+                                    struct iovec *iov)
+{
+    unsigned i;
+
+    for (i = 0; i < out_num + in_num; i++) {
+        int is_write = i >= out_num;
+
+        cpu_physical_memory_unmap(iov->iov_base, iov->iov_len, is_write, 0);
+        iov++;
+    }
 }
 
 static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
@@ -582,8 +606,8 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     vring_desc_read(vdev, &desc, desc_pa, i);
     if (desc.flags & VRING_DESC_F_INDIRECT) {
         if (desc.len % sizeof(VRingDesc)) {
-            error_report("Invalid size for indirect buffer table");
-            exit(1);
+            virtio_error(vdev, "Invalid size for indirect buffer table");
+            return NULL;
         }
 
         /* loop over the indirect descriptor table */
@@ -595,22 +619,30 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 
     /* Collect all the descriptors */
     do {
+        bool map_ok;
+
         if (desc.flags & VRING_DESC_F_WRITE) {
-            virtqueue_map_desc(&in_num, addr + out_num, iov + out_num,
-                               VIRTQUEUE_MAX_SIZE - out_num, true, desc.addr, desc.len);
+            map_ok = virtqueue_map_desc(vdev, &in_num, addr + out_num,
+                                        iov + out_num,
+                                        VIRTQUEUE_MAX_SIZE - out_num, true,
+                                        desc.addr, desc.len);
         } else {
             if (in_num) {
-                error_report("Incorrect order for descriptors");
-                exit(1);
+                virtio_error(vdev, "Incorrect order for descriptors");
+                goto err_undo_map;
             }
-            virtqueue_map_desc(&out_num, addr, iov,
-                               VIRTQUEUE_MAX_SIZE, false, desc.addr, desc.len);
+            map_ok = virtqueue_map_desc(vdev, &out_num, addr, iov,
+                                        VIRTQUEUE_MAX_SIZE, false,
+                                        desc.addr, desc.len);
+        }
+        if (!map_ok) {
+            goto err_undo_map;
         }
 
         /* If we've got too many, that implies a descriptor loop. */
         if ((in_num + out_num) > max) {
-            error_report("Looped descriptor");
-            exit(1);
+            virtio_error(vdev, "Looped descriptor");
+            goto err_undo_map;
         }
     } while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) != max);
 
@@ -630,6 +662,10 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 
     trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
     return elem;
+
+err_undo_map:
+    virtqueue_undo_map_desc(out_num, in_num, iov);
+    return NULL;
 }
 
 /* Reading and writing a structure directly to QEMUFile is *awful*, but
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 05/10] virtio: handle virtqueue_map_desc() errors

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:29 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Errors can occur during virtqueue_pop(), especially in
> virtqueue_map_desc().  In order to handle this we must unmap iov[]
> before returning NULL.  The caller will consider the virtqueue empty and
> the virtio_error() call will have marked the device broken.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 62 ++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 13 deletions(-)
> 

> +/* Only used by error code paths before we have a VirtQueueElement (therefore
> + * virtqueue_unmap_sg() can't be used).  Assumes buffers weren't written to
> + * yet.
> + */

Hm... didn't such an undo function float around in another context as
well? Is it worth factoring out?

> +static void virtqueue_undo_map_desc(unsigned out_num, unsigned in_num,
> +                                    struct iovec *iov)
> +{
> +    unsigned i;

Personally, I don't like pure 'unsigned'. Matter of taste, I guess.

> +
> +    for (i = 0; i < out_num + in_num; i++) {
> +        int is_write = i >= out_num;
> +
> +        cpu_physical_memory_unmap(iov->iov_base, iov->iov_len, is_write, 0);
> +        iov++;
> +    }
>  }

Otherwise, looks good.

[Qemu-devel] [PATCH v3 06/10] virtio: handle virtqueue_get_avail_bytes() errors

[Stefan Hajnoczi]

If the vring is invalid, tell the caller no bytes are available and mark
the device broken.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 0c0d333..48f759e 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -402,14 +402,14 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
         if (desc.flags & VRING_DESC_F_INDIRECT) {
             if (desc.len % sizeof(VRingDesc)) {
-                error_report("Invalid size for indirect buffer table");
-                exit(1);
+                virtio_error(vdev, "Invalid size for indirect buffer table");
+                goto err;
             }
 
             /* If we've got too many, that implies a descriptor loop. */
             if (num_bufs >= max) {
-                error_report("Looped descriptor");
-                exit(1);
+                virtio_error(vdev, "Looped descriptor");
+                goto err;
             }
 
             /* loop over the indirect descriptor table */
@@ -423,8 +423,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
         do {
             /* If we've got too many, that implies a descriptor loop. */
             if (++num_bufs > max) {
-                error_report("Looped descriptor");
-                exit(1);
+                virtio_error(vdev, "Looped descriptor");
+                goto err;
             }
 
             if (desc.flags & VRING_DESC_F_WRITE) {
@@ -449,6 +449,11 @@ done:
     if (out_bytes) {
         *out_bytes = out_total;
     }
+    return;
+
+err:
+    in_total = out_total = 0;
+    goto done;
 }
 
 int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 06/10] virtio: handle virtqueue_get_avail_bytes() errors

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:30 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> If the vring is invalid, tell the caller no bytes are available and mark
> the device broken.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 

> @@ -449,6 +449,11 @@ done:
>      if (out_bytes) {
>          *out_bytes = out_total;
>      }
> +    return;
> +
> +err:
> +    in_total = out_total = 0;
> +    goto done;

This jumping around looks a bit weird, but I don't have any better idea.

>  }
> 
>  int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH v3 07/10] virtio: use unsigned int for virtqueue_get_avail_bytes() index

[Stefan Hajnoczi]

The virtio code uses int, unsigned int, and uint16_t for virtqueue
indices.  The uint16_t is used for the low-level descriptor layout in
virtio_ring.h while code that isn't concerned with descriptor layout can
use unsigned int.

Use of int is problematic because it can result in signed/unsigned
comparison and incompatible int*/unsigned int* pointer types.

Make the virtqueue_get_avail_bytes() 'i' variable unsigned int.  This
eliminates the need to introduce casts and modify code further in the
patches that follow.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 48f759e..493d6a7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -392,7 +392,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
         unsigned int max, num_bufs, indirect = 0;
         VRingDesc desc;
         hwaddr desc_pa;
-        int i;
+        unsigned int i;
 
         max = vq->vring.num;
         num_bufs = total_bufs;
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 07/10] virtio: use unsigned int for virtqueue_get_avail_bytes() index

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:31 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> The virtio code uses int, unsigned int, and uint16_t for virtqueue
> indices.  The uint16_t is used for the low-level descriptor layout in
> virtio_ring.h while code that isn't concerned with descriptor layout can
> use unsigned int.
> 
> Use of int is problematic because it can result in signed/unsigned
> comparison and incompatible int*/unsigned int* pointer types.
> 
> Make the virtqueue_get_avail_bytes() 'i' variable unsigned int.  This
> eliminates the need to introduce casts and modify code further in the
> patches that follow.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH v3 08/10] virtio: handle virtqueue_read_next_desc() errors

[Stefan Hajnoczi]

Stop processing the vring if an avail ring index is invalid.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 493d6a7..58599cf 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -353,28 +353,33 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
     return head;
 }
 
-static unsigned virtqueue_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
-                                         hwaddr desc_pa, unsigned int max)
-{
-    unsigned int next;
+enum {
+    VIRTQUEUE_READ_DESC_ERROR = -1,
+    VIRTQUEUE_READ_DESC_DONE = 0,   /* end of chain */
+    VIRTQUEUE_READ_DESC_MORE = 1,   /* more buffers in chain */
+};
 
+static int virtqueue_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
+                                    hwaddr desc_pa, unsigned int max,
+                                    unsigned int *next)
+{
     /* If this descriptor says it doesn't chain, we're done. */
     if (!(desc->flags & VRING_DESC_F_NEXT)) {
-        return max;
+        return VIRTQUEUE_READ_DESC_DONE;
     }
 
     /* Check they're not leading us off end of descriptors. */
-    next = desc->next;
+    *next = desc->next;
     /* Make sure compiler knows to grab that: we don't want it changing! */
     smp_wmb();
 
-    if (next >= max) {
-        error_report("Desc next is %u", next);
-        exit(1);
+    if (*next >= max) {
+        virtio_error(vdev, "Desc next is %u", *next);
+        return VIRTQUEUE_READ_DESC_ERROR;
     }
 
-    vring_desc_read(vdev, desc, desc_pa, next);
-    return next;
+    vring_desc_read(vdev, desc, desc_pa, *next);
+    return VIRTQUEUE_READ_DESC_MORE;
 }
 
 void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
@@ -383,6 +388,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 {
     unsigned int idx;
     unsigned int total_bufs, in_total, out_total;
+    int rc;
 
     idx = vq->last_avail_idx;
 
@@ -435,7 +441,13 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
             if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
                 goto done;
             }
-        } while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) != max);
+
+            rc = virtqueue_read_next_desc(vdev, &desc, desc_pa, max, &i);
+        } while (rc == VIRTQUEUE_READ_DESC_MORE);
+
+        if (rc == VIRTQUEUE_READ_DESC_ERROR) {
+            goto err;
+        }
 
         if (!indirect)
             total_bufs = num_bufs;
@@ -587,6 +599,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     hwaddr addr[VIRTQUEUE_MAX_SIZE];
     struct iovec iov[VIRTQUEUE_MAX_SIZE];
     VRingDesc desc;
+    int rc;
 
     if (unlikely(vdev->broken)) {
         return NULL;
@@ -649,7 +662,13 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
             virtio_error(vdev, "Looped descriptor");
             goto err_undo_map;
         }
-    } while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) != max);
+
+        rc = virtqueue_read_next_desc(vdev, &desc, desc_pa, max, &i);
+    } while (rc == VIRTQUEUE_READ_DESC_MORE);
+
+    if (rc == VIRTQUEUE_READ_DESC_ERROR) {
+        goto err_undo_map;
+    }
 
     /* Now copy what we have collected and mapped */
     elem = virtqueue_alloc_element(sz, out_num, in_num);
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 08/10] virtio: handle virtqueue_read_next_desc() errors

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:32 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Stop processing the vring if an avail ring index is invalid.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 45 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 13 deletions(-)
> 

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH v3 09/10] virtio: handle virtqueue_num_heads() errors

[Stefan Hajnoczi]

If the avail ring index is bogus virtqueue_num_heads() must return
-EINVAL.

The only caller is virtqueue_get_avail_bytes().  Return saying no bytes
are available when virtqueue_num_heads() fails.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 58599cf..4fe342a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -323,9 +323,9 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int idx)
 
     /* Check it isn't doing very strange things with descriptor numbers. */
     if (num_heads > vq->vring.num) {
-        error_report("Guest moved used index from %u to %u",
+        virtio_error(vq->vdev, "Guest moved used index from %u to %u",
                      idx, vq->shadow_avail_idx);
-        exit(1);
+        return -EINVAL;
     }
     /* On success, callers read a descriptor at vq->last_avail_idx.
      * Make sure descriptor read does not bypass avail index read. */
@@ -393,7 +393,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
     idx = vq->last_avail_idx;
 
     total_bufs = in_total = out_total = 0;
-    while (virtqueue_num_heads(vq, idx)) {
+    while ((rc = virtqueue_num_heads(vq, idx)) > 0) {
         VirtIODevice *vdev = vq->vdev;
         unsigned int max, num_bufs, indirect = 0;
         VRingDesc desc;
@@ -454,6 +454,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
         else
             total_bufs++;
     }
+
+    if (rc < 0) {
+        goto err;
+    }
+
 done:
     if (in_bytes) {
         *in_bytes = in_total;
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 09/10] virtio: handle virtqueue_num_heads() errors

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:33 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> If the avail ring index is bogus virtqueue_num_heads() must return
> -EINVAL.
> 
> The only caller is virtqueue_get_avail_bytes().  Return saying no bytes
> are available when virtqueue_num_heads() fails.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

[Qemu-devel] [PATCH v3 10/10] virtio: handle virtqueue_get_head() errors

[Stefan Hajnoczi]

Stop processing the vring if virtqueue_get_head() fetches an
out-of-bounds head index.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 4fe342a..66394f6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -336,21 +336,20 @@ static int virtqueue_num_heads(VirtQueue *vq, unsigned int idx)
     return num_heads;
 }
 
-static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
+static bool virtqueue_get_head(VirtQueue *vq, unsigned int idx,
+                               unsigned int *head)
 {
-    unsigned int head;
-
     /* Grab the next descriptor number they're advertising, and increment
      * the index we've seen. */
-    head = vring_avail_ring(vq, idx % vq->vring.num);
+    *head = vring_avail_ring(vq, idx % vq->vring.num);
 
     /* If their number is silly, that's a fatal mistake. */
-    if (head >= vq->vring.num) {
-        error_report("Guest says index %u is available", head);
-        exit(1);
+    if (*head >= vq->vring.num) {
+        virtio_error(vq->vdev, "Guest says index %u is available", *head);
+        return false;
     }
 
-    return head;
+    return true;
 }
 
 enum {
@@ -402,7 +401,11 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
         max = vq->vring.num;
         num_bufs = total_bufs;
-        i = virtqueue_get_head(vq, idx++);
+
+        if (!virtqueue_get_head(vq, idx++, &i)) {
+            goto err;
+        }
+
         desc_pa = vq->vring.desc;
         vring_desc_read(vdev, &desc, desc_pa, i);
 
@@ -621,11 +624,14 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
 
     max = vq->vring.num;
 
-    i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
+    if (!virtqueue_get_head(vq, vq->last_avail_idx++, &head)) {
+        return NULL;
+    }
     if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
         vring_set_avail_event(vq, vq->last_avail_idx);
     }
 
+    i = head;
     vring_desc_read(vdev, &desc, desc_pa, i);
     if (desc.flags & VRING_DESC_F_INDIRECT) {
         if (desc.len % sizeof(VRingDesc)) {
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 10/10] virtio: handle virtqueue_get_head() errors

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:34 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Stop processing the vring if virtqueue_get_head() fetches an
> out-of-bounds head index.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states

[Cornelia Huck]

On Tue, 12 Apr 2016 14:25:24 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> v3:
>  * Patch 1: Fix typo and clarify commit description [Markus]
>  * Use virtio_set_status() instead of open coding assignment [Cornelia]
>  * Add live migration
> 
> v2:
>  * Add VIRTIO_CONFIG_S_NEEDS_RESET notification for VIRTIO 1.0 [Cornelia]
>    (Note I've sent a Linux virtio_config.h patch to get the constant added to
>    the headers.)
>  * Split int -> unsigned int change into separate commit [Fam]
>  * Fix double "index" typo in commit description [Fam]
> 
> The virtio code calls exit() when the device enters an invalid state.  This
> means invalid vring indices and descriptor chains kill the VM.  See the patch
> descriptions for why this is a bad thing.
> 
> When the virtio device is in the broken state calls to virtqueue_pop() and
> friends will pretend the virtqueue is empty.  This means the device will become
> isolated from guest activity until it is reset again.
> 
> RFC because two things are missing:
> 1. Live migration support (subsection for broken flag?)
> 2. Auditing devices and replacing exit() calls there too
> 
> Stefan Hajnoczi (10):
>   virtio: fix stray tab character
>   include: update virtio_config.h Linux header
>   virtio: stop virtqueue processing if device is broken
>   virtio: migrate vdev->broken flag
>   virtio: handle virtqueue_map_desc() errors
>   virtio: handle virtqueue_get_avail_bytes() errors
>   virtio: use unsigned int for virtqueue_get_avail_bytes() index
>   virtio: handle virtqueue_read_next_desc() errors
>   virtio: handle virtqueue_num_heads() errors
>   virtio: handle virtqueue_get_head() errors
> 
>  hw/virtio/virtio.c                             | 223 +++++++++++++++++++------
>  include/hw/virtio/virtio.h                     |   3 +
>  include/standard-headers/linux/virtio_config.h |   2 +
>  3 files changed, 181 insertions(+), 47 deletions(-)
> 

As the exit-in-virtio question has popped up several times in the
recent past: I think we should go forward with this series, even if we
still need to look at the individual devices. Do you have a version
that fits on current master?

Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states

[Michael S. Tsirkin]

On Mon, Sep 19, 2016 at 06:07:40PM +0200, Cornelia Huck wrote:
> On Tue, 12 Apr 2016 14:25:24 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > v3:
> >  * Patch 1: Fix typo and clarify commit description [Markus]
> >  * Use virtio_set_status() instead of open coding assignment [Cornelia]
> >  * Add live migration
> > 
> > v2:
> >  * Add VIRTIO_CONFIG_S_NEEDS_RESET notification for VIRTIO 1.0 [Cornelia]
> >    (Note I've sent a Linux virtio_config.h patch to get the constant added to
> >    the headers.)
> >  * Split int -> unsigned int change into separate commit [Fam]
> >  * Fix double "index" typo in commit description [Fam]
> > 
> > The virtio code calls exit() when the device enters an invalid state.  This
> > means invalid vring indices and descriptor chains kill the VM.  See the patch
> > descriptions for why this is a bad thing.
> > 
> > When the virtio device is in the broken state calls to virtqueue_pop() and
> > friends will pretend the virtqueue is empty.  This means the device will become
> > isolated from guest activity until it is reset again.
> > 
> > RFC because two things are missing:
> > 1. Live migration support (subsection for broken flag?)
> > 2. Auditing devices and replacing exit() calls there too
> > 
> > Stefan Hajnoczi (10):
> >   virtio: fix stray tab character
> >   include: update virtio_config.h Linux header
> >   virtio: stop virtqueue processing if device is broken
> >   virtio: migrate vdev->broken flag
> >   virtio: handle virtqueue_map_desc() errors
> >   virtio: handle virtqueue_get_avail_bytes() errors
> >   virtio: use unsigned int for virtqueue_get_avail_bytes() index
> >   virtio: handle virtqueue_read_next_desc() errors
> >   virtio: handle virtqueue_num_heads() errors
> >   virtio: handle virtqueue_get_head() errors
> > 
> >  hw/virtio/virtio.c                             | 223 +++++++++++++++++++------
> >  include/hw/virtio/virtio.h                     |   3 +
> >  include/standard-headers/linux/virtio_config.h |   2 +
> >  3 files changed, 181 insertions(+), 47 deletions(-)
> > 
> 
> As the exit-in-virtio question has popped up several times in the
> recent past: I think we should go forward with this series, even if we
> still need to look at the individual devices. Do you have a version
> that fits on current master?

I agree.

Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states

[Laszlo Ersek]

On 09/19/16 19:51, Michael S. Tsirkin wrote:
> On Mon, Sep 19, 2016 at 06:07:40PM +0200, Cornelia Huck wrote:
>> On Tue, 12 Apr 2016 14:25:24 +0100
>> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>
>>> v3:
>>>  * Patch 1: Fix typo and clarify commit description [Markus]
>>>  * Use virtio_set_status() instead of open coding assignment [Cornelia]
>>>  * Add live migration
>>>
>>> v2:
>>>  * Add VIRTIO_CONFIG_S_NEEDS_RESET notification for VIRTIO 1.0 [Cornelia]
>>>    (Note I've sent a Linux virtio_config.h patch to get the constant added to
>>>    the headers.)
>>>  * Split int -> unsigned int change into separate commit [Fam]
>>>  * Fix double "index" typo in commit description [Fam]
>>>
>>> The virtio code calls exit() when the device enters an invalid state.  This
>>> means invalid vring indices and descriptor chains kill the VM.  See the patch
>>> descriptions for why this is a bad thing.
>>>
>>> When the virtio device is in the broken state calls to virtqueue_pop() and
>>> friends will pretend the virtqueue is empty.  This means the device will become
>>> isolated from guest activity until it is reset again.
>>>
>>> RFC because two things are missing:
>>> 1. Live migration support (subsection for broken flag?)
>>> 2. Auditing devices and replacing exit() calls there too
>>>
>>> Stefan Hajnoczi (10):
>>>   virtio: fix stray tab character
>>>   include: update virtio_config.h Linux header
>>>   virtio: stop virtqueue processing if device is broken
>>>   virtio: migrate vdev->broken flag
>>>   virtio: handle virtqueue_map_desc() errors
>>>   virtio: handle virtqueue_get_avail_bytes() errors
>>>   virtio: use unsigned int for virtqueue_get_avail_bytes() index
>>>   virtio: handle virtqueue_read_next_desc() errors
>>>   virtio: handle virtqueue_num_heads() errors
>>>   virtio: handle virtqueue_get_head() errors
>>>
>>>  hw/virtio/virtio.c                             | 223 +++++++++++++++++++------
>>>  include/hw/virtio/virtio.h                     |   3 +
>>>  include/standard-headers/linux/virtio_config.h |   2 +
>>>  3 files changed, 181 insertions(+), 47 deletions(-)
>>>
>>
>> As the exit-in-virtio question has popped up several times in the
>> recent past: I think we should go forward with this series, even if we
>> still need to look at the individual devices. Do you have a version
>> that fits on current master?
> 
> I agree.
> 

NB, Prasad just posted a patch (v3 being the latest) that adds another
such exit(1), at my suggestion.

[Qemu-devel] [PATCH v3] virtio: add check for descriptor's mapped
                        address

So a rebase of this series should likely consider that patch as well.
(But Stefan is aware anyway.)

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states

[Greg Kurz]

On Mon, 19 Sep 2016 21:27:45 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 09/19/16 19:51, Michael S. Tsirkin wrote:
> > On Mon, Sep 19, 2016 at 06:07:40PM +0200, Cornelia Huck wrote:  
> >> On Tue, 12 Apr 2016 14:25:24 +0100
> >> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >>  
> >>> v3:
> >>>  * Patch 1: Fix typo and clarify commit description [Markus]
> >>>  * Use virtio_set_status() instead of open coding assignment [Cornelia]
> >>>  * Add live migration
> >>>
> >>> v2:
> >>>  * Add VIRTIO_CONFIG_S_NEEDS_RESET notification for VIRTIO 1.0 [Cornelia]
> >>>    (Note I've sent a Linux virtio_config.h patch to get the constant added to
> >>>    the headers.)
> >>>  * Split int -> unsigned int change into separate commit [Fam]
> >>>  * Fix double "index" typo in commit description [Fam]
> >>>
> >>> The virtio code calls exit() when the device enters an invalid state.  This
> >>> means invalid vring indices and descriptor chains kill the VM.  See the patch
> >>> descriptions for why this is a bad thing.
> >>>
> >>> When the virtio device is in the broken state calls to virtqueue_pop() and
> >>> friends will pretend the virtqueue is empty.  This means the device will become
> >>> isolated from guest activity until it is reset again.
> >>>
> >>> RFC because two things are missing:
> >>> 1. Live migration support (subsection for broken flag?)
> >>> 2. Auditing devices and replacing exit() calls there too
> >>>
> >>> Stefan Hajnoczi (10):
> >>>   virtio: fix stray tab character
> >>>   include: update virtio_config.h Linux header
> >>>   virtio: stop virtqueue processing if device is broken
> >>>   virtio: migrate vdev->broken flag
> >>>   virtio: handle virtqueue_map_desc() errors
> >>>   virtio: handle virtqueue_get_avail_bytes() errors
> >>>   virtio: use unsigned int for virtqueue_get_avail_bytes() index
> >>>   virtio: handle virtqueue_read_next_desc() errors
> >>>   virtio: handle virtqueue_num_heads() errors
> >>>   virtio: handle virtqueue_get_head() errors
> >>>
> >>>  hw/virtio/virtio.c                             | 223 +++++++++++++++++++------
> >>>  include/hw/virtio/virtio.h                     |   3 +
> >>>  include/standard-headers/linux/virtio_config.h |   2 +
> >>>  3 files changed, 181 insertions(+), 47 deletions(-)
> >>>  
> >>
> >> As the exit-in-virtio question has popped up several times in the
> >> recent past: I think we should go forward with this series, even if we
> >> still need to look at the individual devices. Do you have a version
> >> that fits on current master?  
> > 
> > I agree.
> >   
> 
> NB, Prasad just posted a patch (v3 being the latest) that adds another
> such exit(1), at my suggestion.
> 
> [Qemu-devel] [PATCH v3] virtio: add check for descriptor's mapped
>                         address
> 
> So a rebase of this series should likely consider that patch as well.
> (But Stefan is aware anyway.)
> 
> Thanks!
> Laszlo
> 

Stefan's series still applies on the current head, except the virtio_config.h
patch which isn't needed anymore.

And indeed there are a bunch of places where QEMU exits:

[greg@bahia qemu-virtio]$ git grep 'exit(1)' hw/virtio hw/*/virtio*
hw/block/virtio-blk.c:        exit(1);
hw/block/virtio-blk.c:        exit(1);
hw/block/virtio-blk.c:        exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:            exit(1);
hw/net/virtio-net.c:                exit(1);
hw/scsi/virtio-scsi-dataplane.c:        exit(1);
hw/scsi/virtio-scsi.c:    exit(1);
hw/scsi/virtio-scsi.c:        exit(1);
hw/scsi/virtio-scsi.c:        exit(1);
hw/virtio/virtio.c:        exit(1);
hw/virtio/virtio.c:            exit(1);
hw/virtio/virtio.c:            exit(1);
hw/virtio/virtio.c:        exit(1);

And also even more places with assert() or BUG_ON(), some of which are
guest errors actually.

For example, in virtio-9p, we have:

static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
{
...
        len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                          &out, sizeof out);
        BUG_ON(len != sizeof out);
...
}

The condition may only be true if the guest sent less than the expected
9P message header which is 7-byte long.

I have a patch for this based on Stefan's series BTW.

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states

[Cornelia Huck]

On Tue, 20 Sep 2016 11:26:57 +0200
Greg Kurz <groug@kaod.org> wrote:

> Stefan's series still applies on the current head, except the virtio_config.h
> patch which isn't needed anymore.

I went through the patches, series generally looks good to me.

> 
> And indeed there are a bunch of places where QEMU exits:

Most of which should be converted to virtio_error(), except...

> 
> [greg@bahia qemu-virtio]$ git grep 'exit(1)' hw/virtio hw/*/virtio*
> hw/block/virtio-blk.c:        exit(1);
> hw/block/virtio-blk.c:        exit(1);
> hw/block/virtio-blk.c:        exit(1);
> hw/net/virtio-net.c:            exit(1);
> hw/net/virtio-net.c:            exit(1);
> hw/net/virtio-net.c:            exit(1);
> hw/net/virtio-net.c:            exit(1);
> hw/net/virtio-net.c:                exit(1);
> hw/scsi/virtio-scsi-dataplane.c:        exit(1);

...this one, which tests for a host misconfiguration, and...

> hw/scsi/virtio-scsi.c:    exit(1);
> hw/scsi/virtio-scsi.c:        exit(1);

...this one, which is a migration stream problem.

> hw/scsi/virtio-scsi.c:        exit(1);
> hw/virtio/virtio.c:        exit(1);
> hw/virtio/virtio.c:            exit(1);
> hw/virtio/virtio.c:            exit(1);
> hw/virtio/virtio.c:        exit(1);
> 
> And also even more places with assert() or BUG_ON(), some of which are
> guest errors actually.

Yes. Let's tackle them piece-by-piece.

> 
> For example, in virtio-9p, we have:
> 
> static void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
> {
> ...
>         len = iov_to_buf(elem->out_sg, elem->out_num, 0,
>                           &out, sizeof out);
>         BUG_ON(len != sizeof out);
> ...
> }
> 
> The condition may only be true if the guest sent less than the expected
> 9P message header which is 7-byte long.
> 
> I have a patch for this based on Stefan's series BTW.

Cool.

Re: [Qemu-devel] [PATCH v3 00/10] virtio: avoid exit() when device enters invalid states

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]

On Mon, Sep 19, 2016 at 06:07:40PM +0200, Cornelia Huck wrote:
> Do you have a version that fits on current master?

Thanks for reminding me about this series.  I will send a rebased
version.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]