| Message ID | 20200114075229.40520-1-pannengyuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-vsock: delete vqs in vhost_vsock_unrealize to avoid memleaks | expand |
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 > > > --
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
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
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 > > . >
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 >*/