[PATCH] vhost-vsock: delete vqs in vhost_vsock_unrealize to avoid memleaks

[PATCH] vhost-vsock: delete vqs in vhost_vsock_unrealize to avoid memleaks

[pannengyuan]

From: Pan Nengyuan <pannengyuan@huawei.com>

Receive/transmit/event vqs forgot to cleanup in vhost_vsock_unrealize. This
patch save receive/transmit vq pointer in realize() and cleanup vqs
through those vq pointers in unrealize(). The leak stack is as follow:

Direct leak of 21504 byte(s) in 3 object(s) allocated from:
  #0 0x7f86a1356970 (/lib64/libasan.so.5+0xef970)  ??:?
  #1 0x7f86a09aa49d (/lib64/libglib-2.0.so.0+0x5249d)  ??:?
  #2 0x5604852f85ca (./x86_64-softmmu/qemu-system-x86_64+0x2c3e5ca)  /mnt/sdb/qemu/hw/virtio/virtio.c:2333
  #3 0x560485356208 (./x86_64-softmmu/qemu-system-x86_64+0x2c9c208)  /mnt/sdb/qemu/hw/virtio/vhost-vsock.c:339
  #4 0x560485305a17 (./x86_64-softmmu/qemu-system-x86_64+0x2c4ba17)  /mnt/sdb/qemu/hw/virtio/virtio.c:3531
  #5 0x5604858e6b65 (./x86_64-softmmu/qemu-system-x86_64+0x322cb65)  /mnt/sdb/qemu/hw/core/qdev.c:865
  #6 0x5604861e6c41 (./x86_64-softmmu/qemu-system-x86_64+0x3b2cc41)  /mnt/sdb/qemu/qom/object.c:2102

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
 hw/virtio/vhost-vsock.c         | 9 +++++++--
 include/hw/virtio/vhost-vsock.h | 2 ++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index f5744363a8..896c0174c1 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -335,8 +335,10 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
                 sizeof(struct virtio_vsock_config));
 
     /* Receive and transmit queues belong to vhost */
-    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
-    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
+    vsock->recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
+                                      vhost_vsock_handle_output);
+    vsock->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
+                                       vhost_vsock_handle_output);
 
     /* The event queue belongs to QEMU */
     vsock->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
@@ -378,6 +380,9 @@ static void vhost_vsock_device_unrealize(DeviceState *dev, Error **errp)
     /* This will stop vhost backend if appropriate. */
     vhost_vsock_set_status(vdev, 0);
 
+    virtio_delete_queue(vsock->recv_vq);
+    virtio_delete_queue(vsock->trans_vq);
+    virtio_delete_queue(vsock->event_vq);
     vhost_dev_cleanup(&vsock->vhost_dev);
     virtio_cleanup(vdev);
 }
diff --git a/include/hw/virtio/vhost-vsock.h b/include/hw/virtio/vhost-vsock.h
index d509d67c4a..bc5a988ee5 100644
--- a/include/hw/virtio/vhost-vsock.h
+++ b/include/hw/virtio/vhost-vsock.h
@@ -33,6 +33,8 @@ typedef struct {
     struct vhost_virtqueue vhost_vqs[2];
     struct vhost_dev vhost_dev;
     VirtQueue *event_vq;
+    VirtQueue *recv_vq;
+    VirtQueue *trans_vq;
     QEMUTimer *post_load_timer;
 
     /*< public >*/
-- 
2.21.0.windows.1

Re: [PATCH] vhost-vsock: delete vqs in vhost_vsock_unrealize to avoid memleaks

[Stefano Garzarella]

On Tue, Jan 14, 2020 at 03:52:29PM +0800, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Receive/transmit/event vqs forgot to cleanup in vhost_vsock_unrealize. This
> patch save receive/transmit vq pointer in realize() and cleanup vqs
> through those vq pointers in unrealize(). The leak stack is as follow:
> 
> Direct leak of 21504 byte(s) in 3 object(s) allocated from:
>   #0 0x7f86a1356970 (/lib64/libasan.so.5+0xef970)  ??:?
>   #1 0x7f86a09aa49d (/lib64/libglib-2.0.so.0+0x5249d)  ??:?
>   #2 0x5604852f85ca (./x86_64-softmmu/qemu-system-x86_64+0x2c3e5ca)  /mnt/sdb/qemu/hw/virtio/virtio.c:2333
>   #3 0x560485356208 (./x86_64-softmmu/qemu-system-x86_64+0x2c9c208)  /mnt/sdb/qemu/hw/virtio/vhost-vsock.c:339
>   #4 0x560485305a17 (./x86_64-softmmu/qemu-system-x86_64+0x2c4ba17)  /mnt/sdb/qemu/hw/virtio/virtio.c:3531
>   #5 0x5604858e6b65 (./x86_64-softmmu/qemu-system-x86_64+0x322cb65)  /mnt/sdb/qemu/hw/core/qdev.c:865
>   #6 0x5604861e6c41 (./x86_64-softmmu/qemu-system-x86_64+0x3b2cc41)  /mnt/sdb/qemu/qom/object.c:2102
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
>  hw/virtio/vhost-vsock.c         | 9 +++++++--
>  include/hw/virtio/vhost-vsock.h | 2 ++
>  2 files changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index f5744363a8..896c0174c1 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -335,8 +335,10 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
>                  sizeof(struct virtio_vsock_config));
>  
>      /* Receive and transmit queues belong to vhost */
> -    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
> -    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
> +    vsock->recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> +                                      vhost_vsock_handle_output);
> +    vsock->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> +                                       vhost_vsock_handle_output);
>  
>      /* The event queue belongs to QEMU */
>      vsock->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> @@ -378,6 +380,9 @@ static void vhost_vsock_device_unrealize(DeviceState *dev, Error **errp)
>      /* This will stop vhost backend if appropriate. */
>      vhost_vsock_set_status(vdev, 0);
>  
> +    virtio_delete_queue(vsock->recv_vq);
> +    virtio_delete_queue(vsock->trans_vq);
> +    virtio_delete_queue(vsock->event_vq);
>      vhost_dev_cleanup(&vsock->vhost_dev);
>      virtio_cleanup(vdev);
>  }
> diff --git a/include/hw/virtio/vhost-vsock.h b/include/hw/virtio/vhost-vsock.h
> index d509d67c4a..bc5a988ee5 100644
> --- a/include/hw/virtio/vhost-vsock.h
> +++ b/include/hw/virtio/vhost-vsock.h
> @@ -33,6 +33,8 @@ typedef struct {
>      struct vhost_virtqueue vhost_vqs[2];
>      struct vhost_dev vhost_dev;
>      VirtQueue *event_vq;
> +    VirtQueue *recv_vq;
> +    VirtQueue *trans_vq;
>      QEMUTimer *post_load_timer;
>  
>      /*< public >*/
> -- 
> 2.21.0.windows.1
> 
> 
> 

-- 

Re: [PATCH] vhost-vsock: delete vqs in vhost_vsock_unrealize to avoid memleaks

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3005 bytes --]

On Tue, Jan 14, 2020 at 03:52:29PM +0800, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> Receive/transmit/event vqs forgot to cleanup in vhost_vsock_unrealize. This
> patch save receive/transmit vq pointer in realize() and cleanup vqs
> through those vq pointers in unrealize(). The leak stack is as follow:
> 
> Direct leak of 21504 byte(s) in 3 object(s) allocated from:
>   #0 0x7f86a1356970 (/lib64/libasan.so.5+0xef970)  ??:?
>   #1 0x7f86a09aa49d (/lib64/libglib-2.0.so.0+0x5249d)  ??:?
>   #2 0x5604852f85ca (./x86_64-softmmu/qemu-system-x86_64+0x2c3e5ca)  /mnt/sdb/qemu/hw/virtio/virtio.c:2333
>   #3 0x560485356208 (./x86_64-softmmu/qemu-system-x86_64+0x2c9c208)  /mnt/sdb/qemu/hw/virtio/vhost-vsock.c:339
>   #4 0x560485305a17 (./x86_64-softmmu/qemu-system-x86_64+0x2c4ba17)  /mnt/sdb/qemu/hw/virtio/virtio.c:3531
>   #5 0x5604858e6b65 (./x86_64-softmmu/qemu-system-x86_64+0x322cb65)  /mnt/sdb/qemu/hw/core/qdev.c:865
>   #6 0x5604861e6c41 (./x86_64-softmmu/qemu-system-x86_64+0x3b2cc41)  /mnt/sdb/qemu/qom/object.c:2102
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
>  hw/virtio/vhost-vsock.c         | 9 +++++++--
>  include/hw/virtio/vhost-vsock.h | 2 ++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index f5744363a8..896c0174c1 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -335,8 +335,10 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
>                  sizeof(struct virtio_vsock_config));
>  
>      /* Receive and transmit queues belong to vhost */
> -    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
> -    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
> +    vsock->recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> +                                      vhost_vsock_handle_output);
> +    vsock->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> +                                       vhost_vsock_handle_output);
>  
>      /* The event queue belongs to QEMU */
>      vsock->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> @@ -378,6 +380,9 @@ static void vhost_vsock_device_unrealize(DeviceState *dev, Error **errp)
>      /* This will stop vhost backend if appropriate. */
>      vhost_vsock_set_status(vdev, 0);
>  
> +    virtio_delete_queue(vsock->recv_vq);
> +    virtio_delete_queue(vsock->trans_vq);
> +    virtio_delete_queue(vsock->event_vq);
>      vhost_dev_cleanup(&vsock->vhost_dev);
>      virtio_cleanup(vdev);
>  }

Please delete the virtqueues after vhost cleanup (the reverse
initialization order).  There is currently no reason why it has to be
done in reverse initialization order, your patch should work too, but
it's a good default for avoiding user-after-free bugs.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] vhost-vsock: delete vqs in vhost_vsock_unrealize to avoid memleaks

[Stefano Garzarella]

On Tue, Jan 14, 2020 at 5:45 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Tue, Jan 14, 2020 at 03:52:29PM +0800, pannengyuan@huawei.com wrote:
> > From: Pan Nengyuan <pannengyuan@huawei.com>
> >
> > Receive/transmit/event vqs forgot to cleanup in vhost_vsock_unrealize. This
> > patch save receive/transmit vq pointer in realize() and cleanup vqs
> > through those vq pointers in unrealize(). The leak stack is as follow:
> >
> > Direct leak of 21504 byte(s) in 3 object(s) allocated from:
> >   #0 0x7f86a1356970 (/lib64/libasan.so.5+0xef970)  ??:?
> >   #1 0x7f86a09aa49d (/lib64/libglib-2.0.so.0+0x5249d)  ??:?
> >   #2 0x5604852f85ca (./x86_64-softmmu/qemu-system-x86_64+0x2c3e5ca)  /mnt/sdb/qemu/hw/virtio/virtio.c:2333
> >   #3 0x560485356208 (./x86_64-softmmu/qemu-system-x86_64+0x2c9c208)  /mnt/sdb/qemu/hw/virtio/vhost-vsock.c:339
> >   #4 0x560485305a17 (./x86_64-softmmu/qemu-system-x86_64+0x2c4ba17)  /mnt/sdb/qemu/hw/virtio/virtio.c:3531
> >   #5 0x5604858e6b65 (./x86_64-softmmu/qemu-system-x86_64+0x322cb65)  /mnt/sdb/qemu/hw/core/qdev.c:865
> >   #6 0x5604861e6c41 (./x86_64-softmmu/qemu-system-x86_64+0x3b2cc41)  /mnt/sdb/qemu/qom/object.c:2102
> >
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> > ---
> >  hw/virtio/vhost-vsock.c         | 9 +++++++--
> >  include/hw/virtio/vhost-vsock.h | 2 ++
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> > index f5744363a8..896c0174c1 100644
> > --- a/hw/virtio/vhost-vsock.c
> > +++ b/hw/virtio/vhost-vsock.c
> > @@ -335,8 +335,10 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
> >                  sizeof(struct virtio_vsock_config));
> >
> >      /* Receive and transmit queues belong to vhost */
> > -    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
> > -    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
> > +    vsock->recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> > +                                      vhost_vsock_handle_output);
> > +    vsock->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> > +                                       vhost_vsock_handle_output);
> >
> >      /* The event queue belongs to QEMU */
> >      vsock->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> > @@ -378,6 +380,9 @@ static void vhost_vsock_device_unrealize(DeviceState *dev, Error **errp)
> >      /* This will stop vhost backend if appropriate. */
> >      vhost_vsock_set_status(vdev, 0);
> >
> > +    virtio_delete_queue(vsock->recv_vq);
> > +    virtio_delete_queue(vsock->trans_vq);
> > +    virtio_delete_queue(vsock->event_vq);
> >      vhost_dev_cleanup(&vsock->vhost_dev);
> >      virtio_cleanup(vdev);
> >  }
>
> Please delete the virtqueues after vhost cleanup (the reverse
> initialization order).  There is currently no reason why it has to be
> done in reverse initialization order, your patch should work too, but
> it's a good default for avoiding user-after-free bugs.
>

Agree!

Since we are here, should we delete the queues also in the error path
of vhost_vsock_device_realize()?

Thanks,
Stefano

Re: [PATCH] vhost-vsock: delete vqs in vhost_vsock_unrealize to avoid memleaks

[Pan Nengyuan]

On 1/15/2020 12:59 AM, Stefano Garzarella wrote:
> On Tue, Jan 14, 2020 at 5:45 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>
>> On Tue, Jan 14, 2020 at 03:52:29PM +0800, pannengyuan@huawei.com wrote:
>>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>>
>>> Receive/transmit/event vqs forgot to cleanup in vhost_vsock_unrealize. This
>>> patch save receive/transmit vq pointer in realize() and cleanup vqs
>>> through those vq pointers in unrealize(). The leak stack is as follow:
>>>
>>> Direct leak of 21504 byte(s) in 3 object(s) allocated from:
>>>   #0 0x7f86a1356970 (/lib64/libasan.so.5+0xef970)  ??:?
>>>   #1 0x7f86a09aa49d (/lib64/libglib-2.0.so.0+0x5249d)  ??:?
>>>   #2 0x5604852f85ca (./x86_64-softmmu/qemu-system-x86_64+0x2c3e5ca)  /mnt/sdb/qemu/hw/virtio/virtio.c:2333
>>>   #3 0x560485356208 (./x86_64-softmmu/qemu-system-x86_64+0x2c9c208)  /mnt/sdb/qemu/hw/virtio/vhost-vsock.c:339
>>>   #4 0x560485305a17 (./x86_64-softmmu/qemu-system-x86_64+0x2c4ba17)  /mnt/sdb/qemu/hw/virtio/virtio.c:3531
>>>   #5 0x5604858e6b65 (./x86_64-softmmu/qemu-system-x86_64+0x322cb65)  /mnt/sdb/qemu/hw/core/qdev.c:865
>>>   #6 0x5604861e6c41 (./x86_64-softmmu/qemu-system-x86_64+0x3b2cc41)  /mnt/sdb/qemu/qom/object.c:2102
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>> ---
>>>  hw/virtio/vhost-vsock.c         | 9 +++++++--
>>>  include/hw/virtio/vhost-vsock.h | 2 ++
>>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
>>> index f5744363a8..896c0174c1 100644
>>> --- a/hw/virtio/vhost-vsock.c
>>> +++ b/hw/virtio/vhost-vsock.c
>>> @@ -335,8 +335,10 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
>>>                  sizeof(struct virtio_vsock_config));
>>>
>>>      /* Receive and transmit queues belong to vhost */
>>> -    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
>>> -    virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE, vhost_vsock_handle_output);
>>> +    vsock->recv_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>>> +                                      vhost_vsock_handle_output);
>>> +    vsock->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>>> +                                       vhost_vsock_handle_output);
>>>
>>>      /* The event queue belongs to QEMU */
>>>      vsock->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
>>> @@ -378,6 +380,9 @@ static void vhost_vsock_device_unrealize(DeviceState *dev, Error **errp)
>>>      /* This will stop vhost backend if appropriate. */
>>>      vhost_vsock_set_status(vdev, 0);
>>>
>>> +    virtio_delete_queue(vsock->recv_vq);
>>> +    virtio_delete_queue(vsock->trans_vq);
>>> +    virtio_delete_queue(vsock->event_vq);
>>>      vhost_dev_cleanup(&vsock->vhost_dev);
>>>      virtio_cleanup(vdev);
>>>  }
>>
>> Please delete the virtqueues after vhost cleanup (the reverse
>> initialization order).  There is currently no reason why it has to be
>> done in reverse initialization order, your patch should work too, but
>> it's a good default for avoiding user-after-free bugs.
>>
> 
> Agree!
> 
> Since we are here, should we delete the queues also in the error path
> of vhost_vsock_device_realize()?

Yes, I will change the cleanup order and aslo delete in the error path.

Thanks.

> 
> Thanks,
> Stefano
> 
> .
>