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

[Marc-André Lureau]

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, should I add your sign-off-by?

thanks

> > 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.
> >>
> > 
> > 
>