[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: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [RFC 2/7] virtio: stop virtqueue processing if device is broken |
Date: |
Tue, 29 Mar 2016 12:12:26 +0100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Fri, Mar 25, 2016 at 02:48:37PM +0800, Fam Zheng wrote:
> 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?
The broken flag is introduced here and without this hunk nothing ever
writes to it. If you had a static analysis tool it might even warn
about the unused field without virtoi_error().
I did consider introducing virtio_error() in another patch before
sending but I think having it here makes this patch more
self-explanatory. It doesn't keep the reviewer guessing about how the
broken flag will be set in later patches.
Stefan
signature.asc
Description: PGP signature
- [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
- [Qemu-devel] [RFC 7/7] virtio: handle virtqueue_get_head() errors, Stefan Hajnoczi, 2016/03/24