From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54932) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRf5A-0003wS-8E for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:34:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bRf55-0002zs-WF for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:34:03 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:34463) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRf55-0002yC-Nl for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:33:59 -0400 Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout1.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0OAV008REEWH9B50@mailout1.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 25 Jul 2016 13:33:53 +0100 (BST) Date: Mon, 25 Jul 2016 15:33:46 +0300 From: Ilya Maximets In-reply-to: <20160721085750.30008-10-marcandre.lureau@redhat.com> Message-id: <5796072A.8080208@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: QUOTED-PRINTABLE References: <20160721085750.30008-10-marcandre.lureau@redhat.com> Subject: Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , qemu-devel@nongnu.org Cc: yuanhan.liu@linux.intel.com, victork@redhat.com, mst@redhat.com, jonshin@cisco.com, mukawa@igel.co.jp, Dyasly Sergey , Heetae Ahn On 21.07.2016 11:57, Marc-Andr=C3=A9 Lureau wrote: > From: Marc-Andr=C3=A9 Lureau >=20 > vhost_net_init() calls vhost_dev_init() and in case of failure, cal= ls > vhost_dev_cleanup() directly. However, the structure is already > partially cleaned on error. Calling vhost_dev_cleanup() again will = call > vhost_virtqueue_cleanup() on already clean queues, and causing pote= ntial > double-close. Instead, adjust dev->nvqs and simplify vhost_dev_init= () > code to not call vhost_virtqueue_cleanup() but vhost_dev_cleanup() > instead. >=20 > Signed-off-by: Marc-Andr=C3=A9 Lureau > --- > hw/virtio/vhost.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) >=20 > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 9400b47..c61302a 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1047,7 +1047,8 @@ int vhost_dev_init(struct vhost_dev *hdev, vo= id *opaque, > for (i =3D 0; i < hdev->nvqs; ++i) { > r =3D vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_i= ndex + i); > if (r < 0) { > - goto fail_vq; > + hdev->nvqs =3D i; > + goto fail; > } > } > =20 > @@ -1104,19 +1105,13 @@ int vhost_dev_init(struct vhost_dev *hdev, = void *opaque, > memory_listener_register(&hdev->memory_listener, &address_spac= e_memory); > QLIST_INSERT_HEAD(&vhost_devices, hdev, entry); > return 0; > + > fail_busyloop: > while (--i >=3D 0) { > vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index = + i, 0); > } > - i =3D hdev->nvqs; > -fail_vq: > - while (--i >=3D 0) { > - vhost_virtqueue_cleanup(hdev->vqs + i); > - } > fail: > - r =3D -errno; > - hdev->vhost_ops->vhost_backend_cleanup(hdev); > - QLIST_REMOVE(hdev, entry); > + vhost_dev_cleanup(hdev); > return r; > } > =20 >=20 This patch introduces closing of zero fd on backend init failure or a= ny other error before virtqueue_init loop because of calling 'vhost_virtqueue_cleanup()' on not initialized virtqueues. I'm suggesting following fixup: ---------------------------------------------------------------------= - diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 6175d8b..d7428c5 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1038,8 +1038,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void= *opaque, VhostBackendType backend_type, uint32_t busyloop_= timeout) { uint64_t features; - int i, r; + int i, r, n_initialized_vqs; =20 + n_initialized_vqs =3D 0; hdev->migration_blocker =3D NULL; =20 r =3D vhost_set_backend_type(hdev, backend_type); @@ -1069,10 +1071,9 @@ int vhost_dev_init(struct vhost_dev *hdev, voi= d *opaque, goto fail; } =20 - for (i =3D 0; i < hdev->nvqs; ++i) { + for (i =3D 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) { r =3D vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_ind= ex + i); if (r < 0) { - hdev->nvqs =3D i; goto fail; } } @@ -1136,6 +1137,7 @@ fail_busyloop: vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + = i, 0); } fail: + hdev->nvqs =3D n_initialized_vqs; vhost_dev_cleanup(hdev); return r; } ---------------------------------------------------------------------= - Best regards, Ilya Maximets.