[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() afte
From: |
Ilya Maximets |
Subject: |
Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init() |
Date: |
Mon, 25 Jul 2016 15:52:44 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 |
On 25.07.2016 15:45, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> On 21.07.2016 11:57, Marc-André Lureau wrote:
>>> From: Marc-André Lureau <address@hidden>
>>>
>>> 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 potential
>>> double-close. Instead, adjust dev->nvqs and simplify vhost_dev_init()
>>> code to not call vhost_virtqueue_cleanup() but vhost_dev_cleanup()
>>> instead.
>>>
>>> Signed-off-by: Marc-André Lureau <address@hidden>
>>> ---
>>> 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 = 0; i < hdev->nvqs; ++i) {
>>> r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
>>> if (r < 0) {
>>> - goto fail_vq;
>>> + hdev->nvqs = i;
>>> + goto fail;
>>> }
>>> }
>>>
>>> @@ -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 >= 0) {
>>> vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
>>> }
>>> - i = hdev->nvqs;
>>> -fail_vq:
>>> - while (--i >= 0) {
>>> - vhost_virtqueue_cleanup(hdev->vqs + i);
>>> - }
>>> fail:
>>> - r = -errno;
>>> - hdev->vhost_ops->vhost_backend_cleanup(hdev);
>>> - QLIST_REMOVE(hdev, entry);
>>> + vhost_dev_cleanup(hdev);
>>> return r;
>>> }
>>>
>>>
>>
>> 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;
>>
>> + n_initialized_vqs = 0;
>> hdev->migration_blocker = NULL;
>>
>> r = vhost_set_backend_type(hdev, backend_type);
>>
>> @@ -1069,10 +1071,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>> *opaque,
>> goto fail;
>> }
>>
>> - for (i = 0; i < hdev->nvqs; ++i) {
>> + for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
>> r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
>> if (r < 0) {
>> - hdev->nvqs = i;
>
> Isn't that assignment doing the same thing?
Yes.
But assignment to zero (hdev->nvqs = 0) required before all previous
'goto fail;' instructions. I think, it's not a clean solution.
> 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 = n_initialized_vqs;
>> vhost_dev_cleanup(hdev);
>> return r;
>> }
>> ----------------------------------------------------------------------
>>
>> Best regards, Ilya Maximets.
>>
>
>
- [Qemu-devel] [PATCH v5 02/31] vhost-user: minor simplification, (continued)
- [Qemu-devel] [PATCH v5 02/31] vhost-user: minor simplification, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 03/31] vhost: don't assume opaque is a fd, use backend cleanup, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 04/31] vhost: make vhost_log_put() idempotent, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 05/31] vhost: assert the log was cleaned up, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 06/31] vhost: fix cleanup on not fully initialized device, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 07/31] vhost: make vhost_dev_cleanup() idempotent, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 08/31] vhost-net: always call vhost_dev_cleanup() on failure, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init(), marcandre . lureau, 2016/07/21
- Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init(), Ilya Maximets, 2016/07/25
- Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init(), Marc-André Lureau, 2016/07/25
- Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init(),
Ilya Maximets <=
- Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init(), Marc-André Lureau, 2016/07/25
- Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init(), Ilya Maximets, 2016/07/25
[Qemu-devel] [PATCH v5 10/31] vhost: do not assert() on vhost_ops failure, marcandre . lureau, 2016/07/21
[Qemu-devel] [PATCH v5 11/31] vhost: add missing VHOST_OPS_DEBUG, marcandre . lureau, 2016/07/21
[Qemu-devel] [PATCH v5 12/31] vhost: use error_report() instead of fprintf(stderr, ...), marcandre . lureau, 2016/07/21
[Qemu-devel] [PATCH v5 13/31] qemu-char: fix qemu_chr_fe_set_msgfds() crash when disconnected, marcandre . lureau, 2016/07/21
[Qemu-devel] [PATCH v5 15/31] vhost-user: check qemu_chr_fe_set_msgfds() return value, marcandre . lureau, 2016/07/21
[Qemu-devel] [PATCH v5 14/31] vhost-user: call set_msgfds unconditionally, marcandre . lureau, 2016/07/21
[Qemu-devel] [PATCH v5 16/31] vhost-user: check vhost_user_{read, write}() return value, marcandre . lureau, 2016/07/21
[Qemu-devel] [PATCH v5 17/31] vhost-user: keep vhost_net after a disconnection, marcandre . lureau, 2016/07/21