From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38457) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cj7Sa-0001lf-8u for qemu-devel@nongnu.org; Wed, 01 Mar 2017 11:50:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cj7SX-00061g-0c for qemu-devel@nongnu.org; Wed, 01 Mar 2017 11:50:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36094) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cj7SW-000618-ON for qemu-devel@nongnu.org; Wed, 01 Mar 2017 11:50:36 -0500 Date: Wed, 1 Mar 2017 18:50:34 +0200 From: "Michael S. Tsirkin" Message-ID: <20170301184527-mutt-send-email-mst@kernel.org> References: <20170228152411.81609-1-cornelia.huck@de.ibm.com> <20170301174738-mutt-send-email-mst@kernel.org> <20170301171554.1bd4107b.cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170301171554.1bd4107b.cornelia.huck@de.ibm.com> Subject: Re: [Qemu-devel] [PATCH] 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:15:54PM +0100, Cornelia Huck wrote: > On Wed, 1 Mar 2017 17:55:24 +0200 > "Michael S. Tsirkin" 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 > > > --- > > > 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. Yes all these callbacks complicate code to the point it's barely readable. I really don't see why are we poking at device beforehand at all. IMHO this is closer to the root of the problem. Don't do it. One way to look at it is to say that we start aio too early. Do it at driver_ok for virtio 1 and on kick for virtio 0 and problems will go away. In fact, is there even a need to call vring_set_notification that early? queues are initialized by guest to 0 so you get a notification on first buffer unconditionally. Would just + if (!vq->vring.desc) { + return; + } be enough? -- MST