[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 2/7] virtio: stop virtqueue processing if device i
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [RFC 2/7] virtio: stop virtqueue processing if device is broken |
Date: |
Fri, 25 Mar 2016 14:48:37 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> ---
> 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
>
- [Qemu-devel] [RFC 0/7] virtio: avoid exit() when device enters invalid states, Stefan Hajnoczi, 2016/03/24
- [Qemu-devel] [RFC 1/7] virtio: fix stray tab character, Stefan Hajnoczi, 2016/03/24
- [Qemu-devel] [RFC 2/7] virtio: stop virtqueue processing if device is broken, Stefan Hajnoczi, 2016/03/24
- [Qemu-devel] [RFC 3/7] virtio: handle virtqueue_map_desc() errors, Stefan Hajnoczi, 2016/03/24
- [Qemu-devel] [RFC 4/7] virtio: handle virtqueue_get_avail_bytes() errors, Stefan Hajnoczi, 2016/03/24
- [Qemu-devel] [RFC 6/7] virtio: handle virtqueue_num_heads() errors, Stefan Hajnoczi, 2016/03/24
- [Qemu-devel] [RFC 5/7] virtio: handle virtqueue_read_next_desc() errors, Stefan Hajnoczi, 2016/03/24