From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57451) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfGk-0001LI-Eu for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:46:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bRfGh-0005le-9Y for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:46:02 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:58475) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfGh-0005la-1R for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:45:59 -0400 Date: Mon, 25 Jul 2016 08:45:52 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1810601664.8302200.1469450752823.JavaMail.zimbra@redhat.com> In-Reply-To: <5796072A.8080208@samsung.com> References: <20160721085750.30008-10-marcandre.lureau@redhat.com> <5796072A.8080208@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: Ilya Maximets 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 Hi ----- Original Message ----- > 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, calls > > 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 potentia= l > > 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, 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_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 >=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. >=20 > I'm suggesting following fixup: >=20 > ---------------------------------------------------------------------- > 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_time= out) > { > 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); >=20 > @@ -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; Isn't that assignment doing the same thing? btw, thanks for the review > 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; > } > ---------------------------------------------------------------------- >=20 > Best regards, Ilya Maximets. >=20