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

[Stefan Hajnoczi]

On Tue, Jan 14, 2020 at 03:52:29PM +0800, address@hidden wrote:
> From: Pan Nengyuan <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Pan Nengyuan <address@hidden>
> ---
>  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

signature.asc
Description: PGP signature