| Message ID | 20200327035650.2085-2-pannengyuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix two virtio queues memleak | expand |
On Fri, Mar 27, 2020 at 11:56:49AM +0800, Pan Nengyuan wrote: > virtio_vqs forgot to free on the error path in realize(). Fix that. > > The asan stack: > Direct leak of 14336 byte(s) in 1 object(s) allocated from: > #0 0x7f58b93fd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970) > #1 0x7f58b858249d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d) > #2 0x5562cc627f49 in virtio_add_queue /mnt/sdb/qemu/hw/virtio/virtio.c:2413 > #3 0x5562cc4b524a in virtio_blk_device_realize /mnt/sdb/qemu/hw/block/virtio-blk.c:1202 > #4 0x5562cc613050 in virtio_device_realize /mnt/sdb/qemu/hw/virtio/virtio.c:3615 > #5 0x5562ccb7a568 in device_set_realized /mnt/sdb/qemu/hw/core/qdev.c:891 > #6 0x5562cd39cd45 in property_set_bool /mnt/sdb/qemu/qom/object.c:2238 > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Max Reitz <mreitz@redhat.com> > Cc: qemu-block@nongnu.org > --- > hw/block/virtio-blk.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 142863a3b2..a6682c2ced 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -1204,8 +1204,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) > virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err); > if (err != NULL) { > error_propagate(errp, err); > - virtio_cleanup(vdev); > - return; > + goto fail; > } > > s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s); > @@ -1218,6 +1217,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) > conf->conf.lcyls, > conf->conf.lheads, > conf->conf.lsecs); I think we should add a return here, otherwise we will remove the virt queues also in the success case. Thanks, Stefano > +fail: > + for (i = 0; i < conf->num_queues; i++) { > + virtio_del_queue(vdev, i); > + } > + virtio_cleanup(vdev); > } > > static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) > -- > 2.18.2 > >
On 3/27/2020 4:41 PM, Stefano Garzarella wrote: > On Fri, Mar 27, 2020 at 11:56:49AM +0800, Pan Nengyuan wrote: >> virtio_vqs forgot to free on the error path in realize(). Fix that. >> >> The asan stack: >> Direct leak of 14336 byte(s) in 1 object(s) allocated from: >> #0 0x7f58b93fd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970) >> #1 0x7f58b858249d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d) >> #2 0x5562cc627f49 in virtio_add_queue /mnt/sdb/qemu/hw/virtio/virtio.c:2413 >> #3 0x5562cc4b524a in virtio_blk_device_realize /mnt/sdb/qemu/hw/block/virtio-blk.c:1202 >> #4 0x5562cc613050 in virtio_device_realize /mnt/sdb/qemu/hw/virtio/virtio.c:3615 >> #5 0x5562ccb7a568 in device_set_realized /mnt/sdb/qemu/hw/core/qdev.c:891 >> #6 0x5562cd39cd45 in property_set_bool /mnt/sdb/qemu/qom/object.c:2238 >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> >> --- >> Cc: Stefan Hajnoczi <stefanha@redhat.com> >> Cc: Kevin Wolf <kwolf@redhat.com> >> Cc: Max Reitz <mreitz@redhat.com> >> Cc: qemu-block@nongnu.org >> --- >> hw/block/virtio-blk.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index 142863a3b2..a6682c2ced 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -1204,8 +1204,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) >> virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err); >> if (err != NULL) { >> error_propagate(errp, err); >> - virtio_cleanup(vdev); >> - return; >> + goto fail; >> } >> >> s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s); >> @@ -1218,6 +1217,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) >> conf->conf.lcyls, >> conf->conf.lheads, >> conf->conf.lsecs); > > I think we should add a return here, otherwise we will remove the virt > queues also in the success case. Yes, I have sent a new version v2 and changed it, you can review on it. Thanks. > > Thanks, > Stefano > >> +fail: >> + for (i = 0; i < conf->num_queues; i++) { >> + virtio_del_queue(vdev, i); >> + } >> + virtio_cleanup(vdev); >> } >> >> static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) >> -- >> 2.18.2 >> >> > > . >
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 142863a3b2..a6682c2ced 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -1204,8 +1204,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err); if (err != NULL) { error_propagate(errp, err); - virtio_cleanup(vdev); - return; + goto fail; } s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s); @@ -1218,6 +1217,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) conf->conf.lcyls, conf->conf.lheads, conf->conf.lsecs); +fail: + for (i = 0; i < conf->num_queues; i++) { + virtio_del_queue(vdev, i); + } + virtio_cleanup(vdev); } static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
virtio_vqs forgot to free on the error path in realize(). Fix that. The asan stack: Direct leak of 14336 byte(s) in 1 object(s) allocated from: #0 0x7f58b93fd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970) #1 0x7f58b858249d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d) #2 0x5562cc627f49 in virtio_add_queue /mnt/sdb/qemu/hw/virtio/virtio.c:2413 #3 0x5562cc4b524a in virtio_blk_device_realize /mnt/sdb/qemu/hw/block/virtio-blk.c:1202 #4 0x5562cc613050 in virtio_device_realize /mnt/sdb/qemu/hw/virtio/virtio.c:3615 #5 0x5562ccb7a568 in device_set_realized /mnt/sdb/qemu/hw/core/qdev.c:891 #6 0x5562cd39cd45 in property_set_bool /mnt/sdb/qemu/qom/object.c:2238 Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> --- Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Cc: qemu-block@nongnu.org --- hw/block/virtio-blk.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)