From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59030) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfNM-0006WH-KH for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:52:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bRfNH-0007BR-MU for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:52:52 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:54511) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfNH-0007BF-DQ for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:52:47 -0400 Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0OAV00G73FRXZ860@mailout4.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 25 Jul 2016 13:52:45 +0100 (BST) Date: Mon, 25 Jul 2016 15:52:44 +0300 From: Ilya Maximets In-reply-to: <1810601664.8302200.1469450752823.JavaMail.zimbra@redhat.com> Message-id: <57960B9C.8040103@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> <5796072A.8080208@samsung.com> <1810601664.8302200.1469450752823.JavaMail.zimbra@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?= Cc: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , qemu-devel@nongnu.org, yuanhan liu , victork@redhat.com, mst@redhat.com, jonshin@cisco.com, mukawa@igel.co.jp, Dyasly Sergey , Heetae Ahn On 25.07.2016 15:45, Marc-Andr=C3=A9 Lureau wrote: > Hi >=20 > ----- Original Message ----- >> On 21.07.2016 11:57, Marc-Andr=C3=A9 Lureau wrote: >>> From: Marc-Andr=C3=A9 Lureau >>> >>> vhost_net_init() calls vhost_dev_init() and in case of failure, c= alls >>> vhost_dev_cleanup() directly. However, the structure is already >>> partially cleaned on error. Calling vhost_dev_cleanup() again wil= l call >>> vhost_virtqueue_cleanup() on already clean queues, and causing po= tential >>> double-close. Instead, adjust dev->nvqs and simplify vhost_dev_in= it() >>> code to not call vhost_virtqueue_cleanup() but vhost_dev_cleanup(= ) >>> instead. >>> >>> Signed-off-by: Marc-Andr=C3=A9 Lureau >>> --- >>> hw/virtio/vhost.c | 13 ++++--------- >>> 1 file changed, 4 insertions(+), 9 deletions(-) >>> >>> 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, = void >>> *opaque, >>> for (i =3D 0; i < hdev->nvqs; ++i) { >>> r =3D vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq= _index + 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_space_memory); >>> QLIST_INSERT_HEAD(&vhost_devices, hdev, entry); >>> return 0; >>> + >>> fail_busyloop: >>> while (--i >=3D 0) { >>> vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_inde= x + 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 >>> >> >> This patch introduces closing of zero fd on backend init failure o= r any >> 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, v= oid >> *opaque, >> VhostBackendType backend_type, uint32_t busylo= op_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, = void >> *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_= index + i); >> if (r < 0) { >> - hdev->nvqs =3D i; >=20 > Isn't that assignment doing the same thing? Yes. But assignment to zero (hdev->nvqs =3D 0) required before all previou= s 'goto fail;' instructions. I think, it's not a clean solution. > btw, thanks for the review >=20 >> 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. >> >=20 >=20