Re: [Qemu-devel] [RFC 2/7] virtio: stop virtqueue processing if device is broken

[Fam Zheng <famz@redhat.com>]

On Thu, 03/24 17:56, Stefan Hajnoczi 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         | 34 ++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h |  3 +++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index de8a3b3..8fac47c 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -276,6 +276,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;
> @@ -286,6 +290,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);
> @@ -546,6 +556,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;
>      }
> @@ -705,6 +718,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);
>      }
> @@ -788,6 +805,7 @@ void virtio_reset(void *opaque)
>          k->reset(vdev);
>      }
>  
> +    vdev->broken = false;
>      vdev->guest_features = 0;
>      vdev->queue_sel = 0;
>      vdev->status = 0;
> @@ -1091,6 +1109,10 @@ 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);
>      }
> @@ -1661,6 +1683,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;
> @@ -1829,6 +1852,17 @@ 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;
> +}
> +

Since it's not used by the rest of the patch and it is independent, Probably
split this change to a separate patch?

>  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 2b5b248..1565e53 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
>