From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46551) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cj7ih-0002NZ-97 for qemu-devel@nongnu.org; Wed, 01 Mar 2017 12:07:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cj7ie-0004yQ-4g for qemu-devel@nongnu.org; Wed, 01 Mar 2017 12:07:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41632) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cj7id-0004xJ-Sb for qemu-devel@nongnu.org; Wed, 01 Mar 2017 12:07:16 -0500 Date: Wed, 1 Mar 2017 19:07:14 +0200 From: "Michael S. Tsirkin" Message-ID: <20170301190609-mutt-send-email-mst@kernel.org> References: <20170301165656.80456-1-cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170301165656.80456-1-cornelia.huck@de.ibm.com> Subject: Re: [Qemu-devel] [PATCH v2] virtio: guard vring access when setting notification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, borntraeger@de.ibm.com 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 > --- > 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