[v2] virtio: guard vring access when setting notification

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 AND the device is in a state that
the rings may be actually accessed (i.e. DRIVER_OK has been set
or a legacy device kicks for the first time).

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
v1->v2:
- Don't touch queues before DRIVER_OK or first kick
- Migration stuff

Only very lightly tested.

---
 hw/virtio/virtio.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 3 deletions(-)

On Wed, Mar 01, 2017 at 05:56:56PM +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 AND the device is in a state that
> the rings may be actually accessed (i.e. DRIVER_OK has been set
> or a legacy device kicks for the first time).

I am still puzzled about 2. drivers pre-zero rings as they
are set up so notification is on by default. Why do you want to poke
at it one extra time? You might get an extra notification
if you tried to disable too early but is this really so bad?

> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> v1->v2:
> - Don't touch queues before DRIVER_OK or first kick
> - Migration stuff
> 
> Only very lightly tested.
> 
> ---
>  hw/virtio/virtio.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 71 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index e487e36..b467290 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -99,6 +99,9 @@ struct VirtQueue
>      /* Notification enabled? */
>      bool notification;
>  
> +    /* Delayed setup of flags for notifications */
> +    bool delayed_notification_setup;
> +
>      uint16_t queue_index;
>  
>      unsigned int inuse;
> @@ -284,10 +287,15 @@ 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 ||
> +        (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_VERSION_1) &&
> +         !(vq->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK))) {
> +        vq->delayed_notification_setup = true;
> +        return;
> +    }
> +    vq->delayed_notification_setup = false;
>      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 +311,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;
> @@ -1087,6 +1102,17 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>      if (k->set_status) {
>          k->set_status(vdev, val);
>      }
> +    if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> +        val & VIRTIO_CONFIG_S_FEATURES_OK) {
> +        int i;
> +
> +        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +            if (!virtio_queue_get_num(vdev, i)) {
> +                break;
> +            }
> +            vring_set_notification(&vdev->vq[i], vdev->vq[i].notification);
> +        }
> +    }
>      vdev->status = val;
>      return 0;
>  }
> @@ -1152,6 +1178,7 @@ void virtio_reset(void *opaque)
>          vdev->vq[i].notification = true;
>          vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
>          vdev->vq[i].inuse = 0;
> +        vdev->vq[i].delayed_notification_setup = false;
>      }
>  }
>  
> @@ -1671,6 +1698,19 @@ static bool virtio_broken_needed(void *opaque)
>      return vdev->broken;
>  }
>  
> +static bool virtio_delayed_notification_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +    int i;
> +
> +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        if (vdev->vq[i].delayed_notification_setup) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>  static const VMStateDescription vmstate_virtqueue = {
>      .name = "virtqueue_state",
>      .version_id = 1,
> @@ -1799,6 +1839,29 @@ static const VMStateDescription vmstate_virtio_broken = {
>      }
>  };
>  
> +static const VMStateDescription vmstate_delayed_notification = {
> +    .name = "delayed_notification_state",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(delayed_notification_setup, struct VirtQueue),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_virtio_delayed_notification = {
> +    .name = "virtio/delayed_notification",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = &virtio_delayed_notification_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice,
> +                      VIRTIO_QUEUE_MAX, 0, vmstate_delayed_notification,
> +                      VirtQueue),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio = {
>      .name = "virtio",
>      .version_id = 1,
> @@ -1814,6 +1877,7 @@ static const VMStateDescription vmstate_virtio = {
>          &vmstate_virtio_ringsize,
>          &vmstate_virtio_broken,
>          &vmstate_virtio_extra_state,
> +        &vmstate_virtio_delayed_notification,
>          NULL
>      }
>  };
> @@ -2273,6 +2337,10 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq)
>  static void virtio_queue_host_notifier_aio_read(EventNotifier *n)
>  {
>      VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
> +
> +    if (unlikely(vq->delayed_notification_setup)) {
> +        vring_set_notification(vq, vq->notification);
> +    }
>      if (event_notifier_test_and_clear(n)) {
>          virtio_queue_notify_aio_vq(vq);
>      }
> -- 
> 2.8.4

On Wed, 1 Mar 2017 19:07:14 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Mar 01, 2017 at 05:56:56PM +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 AND the device is in a state that
> > the rings may be actually accessed (i.e. DRIVER_OK has been set
> > or a legacy device kicks for the first time).
> 
> I am still puzzled about 2. drivers pre-zero rings as they
> are set up so notification is on by default. Why do you want to poke
> at it one extra time? You might get an extra notification
> if you tried to disable too early but is this really so bad?

Just checking for !desc and then leaving the queues alone might be the
right thing. See also my reply in the other thread.