Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()

[Ilya Maximets]

On 25.07.2016 16:05, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> 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.
>>
> 
> 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.