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

[Marc-André Lureau]

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?

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