[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.
- Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification,
Cornelia Huck <=
- Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification, Cornelia Huck, 2017/03/01
- Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification, Cornelia Huck, 2017/03/01
- Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification, Michael S. Tsirkin, 2017/03/01
- Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification, Cornelia Huck, 2017/03/01