Re: [PATCH 1/2] virtio-blk: delete vqs on the error path in realize()
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 >> Signed-off-by: Pan Nengyuan >> --- >> Cc: Stefan Hajnoczi >> Cc: Kevin Wolf >> Cc: Max Reitz >> Cc: qemu-bl...@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 >> >> > > . >
Re: [PATCH 1/2] virtio-blk: delete vqs on the error path in realize()
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 > Signed-off-by: Pan Nengyuan > --- > Cc: Stefan Hajnoczi > Cc: Kevin Wolf > Cc: Max Reitz > Cc: qemu-bl...@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 > >
[PATCH 1/2] virtio-blk: delete vqs on the error path in realize()
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 Signed-off-by: Pan Nengyuan --- Cc: Stefan Hajnoczi Cc: Kevin Wolf Cc: Max Reitz Cc: qemu-bl...@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); +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