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

[Stefan Hajnoczi]

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