From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36465) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfih-0005mR-CI for qemu-devel@nongnu.org; Mon, 25 Jul 2016 09:14:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bRfic-00042Z-4h for qemu-devel@nongnu.org; Mon, 25 Jul 2016 09:14:54 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:20547) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfib-00041m-T2 for qemu-devel@nongnu.org; Mon, 25 Jul 2016 09:14:50 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout2.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0OAV001URGSMZ660@mailout2.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 25 Jul 2016 14:14:46 +0100 (BST) Date: Mon, 25 Jul 2016 16:14:44 +0300 From: Ilya Maximets In-reply-to: <1508108764.8320623.1469451933774.JavaMail.zimbra@redhat.com> Message-id: <579610C4.40609@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> <57960B9C.8040103@samsung.com> <1508108764.8320623.1469451933774.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 16:05, Marc-Andr=C3=A9 Lureau wrote: > Hi >=20 > ----- Original Message ----- >> On 25.07.2016 15:45, Marc-Andr=C3=A9 Lureau wrote: >>> Hi >>> >>> ----- 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,= calls >>>>> vhost_dev_cleanup() directly. However, the structure is already >>>>> partially cleaned on error. Calling vhost_dev_cleanup() again w= ill call >>>>> vhost_virtqueue_cleanup() on already clean queues, and causing = potential >>>>> double-close. Instead, adjust dev->nvqs and simplify vhost_dev_= init() >>>>> code to not call vhost_virtqueue_cleanup() but vhost_dev_cleanu= p() >>>>> 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 *hd= ev, 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_in= dex + 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= or 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,= 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= , 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->v= q_index + >>>> i); >>>> if (r < 0) { >>>> - hdev->nvqs =3D i; >>> >>> Isn't that assignment doing the same thing? >> >> Yes. >> But assignment to zero (hdev->nvqs =3D 0) required before all prev= ious >> 'goto fail;' instructions. I think, it's not a clean solution. >> >=20 > Good point, I'll squash your change, Thanks for fixing it. > should I add your sign-off-by? I don't mind if you want to. Best regards, Ilya Maximets.