qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notific


From: Cornelia Huck
Subject: Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification
Date: Wed, 1 Mar 2017 17:15:54 +0100

On Wed, 1 Mar 2017 17:55:24 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Tue, Feb 28, 2017 at 04:24:11PM +0100, Cornelia Huck wrote:
> > Switching to vring caches exposed an existing bug in
> > virtio_queue_set_notification(): We can't access vring structures
> > if they have not been set up yet. This may happen, for example,
> > for virtio-blk devices with multiple queues: The code will try to
> > switch notifiers for every queue, but the guest may have only set up
> > a subset of them.
> > 
> > Fix this by (1) guarding access to the vring memory by checking
> > for vring.desc and (2) triggering an update to the vring flags
> > for consistency with the configured notification state once the
> > queue is actually configured.
> > 
> > Signed-off-by: Cornelia Huck <address@hidden>
> > ---
> >  hw/virtio/virtio.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index e487e36..d2ecd64 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -284,10 +284,11 @@ static inline void vring_set_avail_event(VirtQueue 
> > *vq, uint16_t val)
> >      virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val);
> >  }
> >  
> > -void virtio_queue_set_notification(VirtQueue *vq, int enable)
> > +static void vring_set_notification(VirtQueue *vq, int enable)
> >  {
> > -    vq->notification = enable;
> > -
> > +    if (!vq->vring.desc) {
> > +        return;
> > +    }
> >      rcu_read_lock();
> >      if (virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> >          vring_set_avail_event(vq, vring_avail_idx(vq));
> > @@ -303,6 +304,13 @@ void virtio_queue_set_notification(VirtQueue *vq, int 
> > enable)
> >      rcu_read_unlock();
> >  }
> >  
> > +void virtio_queue_set_notification(VirtQueue *vq, int enable)
> > +{
> > +    vq->notification = enable;
> > +
> > +    vring_set_notification(vq, enable);
> > +}
> > +
> >  int virtio_queue_ready(VirtQueue *vq)
> >  {
> >      return vq->vring.avail != 0;
> > @@ -1348,6 +1356,7 @@ void virtio_queue_set_addr(VirtIODevice *vdev, int n, 
> > hwaddr addr)
> >  {
> >      vdev->vq[n].vring.desc = addr;
> >      virtio_queue_update_rings(vdev, n);
> > +    vring_set_notification(&vdev->vq[n], vdev->vq[n].notification);
> >  }
> >  
> >  hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
> > @@ -1362,6 +1371,7 @@ void virtio_queue_set_rings(VirtIODevice *vdev, int 
> > n, hwaddr desc,
> >      vdev->vq[n].vring.avail = avail;
> >      vdev->vq[n].vring.used = used;
> >      virtio_init_region_cache(vdev, n);
> > +    vring_set_notification(&vdev->vq[n], vdev->vq[n].notification);
> >  }
> >  
> >  void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
> 
> There's a problem here, this violates the spec:
> - for legacy devices, we shouldn't touch rings until we get a first kick
> - for virtio 1 devices, we should not do it until DRIVER_OK
> 
> This is the real problem therefore: aio poll should not even
> start before these events. Pls fix that and then you will not
> need to call vring_set_notification from set rings.

Hooking into set_status for virtio-1 devices should not be a problem.

For legacy, we probably need to track and do a delayed switch.  Let me
see what I can come up with.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]