[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notific
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification |
Date: |
Wed, 1 Mar 2017 17:55:24 +0200 |
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.
> --
> 2.8.4
>
- Re: [Qemu-devel] [PATCH] virtio: guard vring access when setting notification,
Michael S. Tsirkin <=
- 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
- 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