Re: [Qemu-stable] [Qemu-devel] [PULL 47/58] qdev-monitor: Unref device when device_add fails

[Amos Kong]

On Mon, Nov 18, 2013 at 03:35:20PM +0100, Andreas Färber wrote:
> Am 18.11.2013 13:29, schrieb Amos Kong:
> > On Tue, Oct 08, 2013 at 07:44:45PM +0200, Andreas Färber wrote:
> >> From: Stefan Hajnoczi <address@hidden>
> >>
> >> qdev_device_add() leaks the created device upon failure.  I suspect this
> >> problem crept in because qdev_free() unparents the device but does not
> >> drop a reference - confusing name.
> >>
> >> Cc: address@hidden
> >> Signed-off-by: Stefan Hajnoczi <address@hidden>
> >> Signed-off-by: Andreas Färber <address@hidden>
> > 
> > Hi Stefan,
> > 
> > This commit caused a regression bug:
> > 
> > hotplug more than 32 disks to vm, qemu crash
> > 
> > ---
> > 
> > address@hidden qemu]$ cat radd.sh 
> > for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 
> > 1f;do
> > for j in `seq 1 7` 0;do
> > /bin/cp /images/none.qcow2 /tmp/resize$i$j.qcow2
> > 
> > echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none
> > echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none | nc -U 
> > /tmp/m
> > 
> > echo device_add 
> > virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on
> > echo device_add 
> > virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on | nc 
> > -U /tmp/m
> > done
> > done
> 
> Hi, thanks for catching this.
> 
> Is this only with virtio-blk-pci or with any PCI card or only when
> drives are involved?

I can reproduce by hotplugging virtio-net-pci NIC

for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f;do
for j in `seq 1 7` 0;do

echo "netdev_add tap,id=dev$i$j" | nc -U /tmp/m
echo "device_add 
virtio-net-pci,netdev=dev$i$j,id=nic$i$j,addr=0x$i.$j,multifunction=on" | nc -U 
/tmp/m

done
done


> Either way it would be really great if you could
> add such tests to Stefan's qtest using QMP, so that it can easily be run
> by everyone.

I will do it.
 
> The stacktrace below is not really telling to me. I wonder if we forget
> to clean up some MemoryRegion in the device that still has a back
> reference or whether the Memory API still references MemoryRegions that
> have been destroyed by the device or forgets the reference devices it
> still needs... Paolo?
> 
> I had reviewed the call paths and believe the patch to be 100% good, so
> the fault will very likely be elsewhere.
> 
> Regards,
> Andreas
> 
> > 
> > ----
> > 
> > #0  0x00005555558b7f95 in flatview_ref (view=0x0) at 
> > /home/devel/qemu/memory.c:300
> > #1  0x00005555558b9689 in address_space_get_flatview (as=0x55555645d660) at 
> > /home/devel/qemu/memory.c:656
> > #2  0x00005555558ba416 in address_space_update_topology (as=0x55555645d660) 
> > at /home/devel/qemu/memory.c:760
> > #3  0x00005555558ba5cf in memory_region_transaction_commit () at 
> > /home/devel/qemu/memory.c:799
> > #4  0x00005555558bcfcc in memory_region_set_enabled (mr=0x55555647af08, 
> > enabled=false) at /home/devel/qemu/memory.c:1503
> > #5  0x000055555571a0af in do_pci_register_device (pci_dev=0x55555647ac10, 
> > bus=0x5555564132b0, name=0x555556261100 "virtio-blk-pci", devfn=26) at 
> > hw/pci/pci.c:846
> > #6  0x000055555571c6cc in pci_qdev_init (qdev=0x55555647ac10) at 
> > hw/pci/pci.c:1751
> > #7  0x0000555555694d70 in device_realize (dev=0x55555647ac10, 
> > err=0x7fffffffc8e8) at hw/core/qdev.c:178
> > #8  0x00005555556966fc in device_set_realized (obj=0x55555647ac10, 
> > value=true, err=0x7fffffffca60) at hw/core/qdev.c:699
> > #9  0x00005555557e7b57 in property_set_bool (obj=0x55555647ac10, 
> > v=0x55555679a830, opaque=0x555556461b10, name=0x5555559922ae "realized", 
> > errp=0x7fffffffca60)
> >     at qom/object.c:1315
> > #10 0x00005555557e665b in object_property_set (obj=0x55555647ac10, 
> > v=0x55555679a830, name=0x5555559922ae "realized", errp=0x7fffffffca60) at 
> > qom/object.c:803
> > #11 0x00005555557e816e in object_property_set_qobject (obj=0x55555647ac10, 
> > value=0x555556678880, name=0x5555559922ae "realized", errp=0x7fffffffca60) 
> > at qom/qom-qobject.c:24
> > #12 0x00005555557e6950 in object_property_set_bool (obj=0x55555647ac10, 
> > value=true, name=0x5555559922ae "realized", errp=0x7fffffffca60) at 
> > qom/object.c:866
> > #13 0x0000555555694ca7 in qdev_init (dev=0x55555647ac10) at 
> > hw/core/qdev.c:163
> > #14 0x00005555557c60ee in qdev_device_add (opts=0x555556525370) at 
> > qdev-monitor.c:543
> > #15 0x00005555557c6730 in do_device_add (mon=0x5555562fb760, 
> > qdict=0x55555645d440, ret_data=0x7fffffffcb80) at qdev-monitor.c:656
> > #16 0x00005555558c8892 in handle_user_command (mon=0x5555562fb760, 
> > cmdline=0x5555563f0f60 "device_add 
> > virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on")
> >     at /home/devel/qemu/monitor.c:4137
> > #17 0x00005555558ca10f in monitor_command_cb (mon=0x5555562fb760, 
> > cmdline=0x5555563f0f60 "device_add 
> > virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on", 
> >     opaque=0x0) at /home/devel/qemu/monitor.c:4757
> > #18 0x00005555557e9491 in readline_handle_byte (rs=0x5555563f0f60, ch=10) 
> > at readline.c:373
> > #19 0x00005555558ca045 in monitor_read (opaque=0x5555562fb760, 
> > buf=0x7fffffffccf0 "\n\315\377\377\377\177", size=1) at 
> > /home/devel/qemu/monitor.c:4743
> > #20 0x00005555557c6cc8 in qemu_chr_be_write (s=0x555556269040, 
> > buf=0x7fffffffccf0 "\n\315\377\377\377\177", len=1) at qemu-char.c:165
> > #21 0x00005555557cb026 in tcp_chr_read (chan=0x55555645fe40, cond=G_IO_IN, 
> > opaque=0x555556269040) at qemu-char.c:2487
> > #22 0x00007ffff76ede06 in g_main_context_dispatch () from 
> > /lib64/libglib-2.0.so.0
> > #23 0x000055555578ef33 in glib_pollfds_poll () at main-loop.c:189
> > #24 0x000055555578f028 in os_host_main_loop_wait (timeout=77312299) at 
> > main-loop.c:234
> > #25 0x000055555578f100 in main_loop_wait (nonblocking=0) at main-loop.c:483
> > #26 0x000055555582e234 in main_loop () at vl.c:2014
> > #27 0x0000555555835697 in main (argc=14, argv=0x7fffffffe298, 
> > envp=0x7fffffffe310) at vl.c:4362
> > 
> >> ---
> >>  qdev-monitor.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/qdev-monitor.c b/qdev-monitor.c
> >> index b1ce26a..531b258 100644
> >> --- a/qdev-monitor.c
> >> +++ b/qdev-monitor.c
> >> @@ -518,6 +518,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >>      }
> >>      if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
> >>          qdev_free(qdev);
> >> +        object_unref(OBJECT(qdev));
> >>          return NULL;
> >>      }
> >>      if (qdev->id) {
> >> @@ -531,6 +532,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> >>          g_free(name);
> >>      }        
> >>      if (qdev_init(qdev) < 0) {
> >> +        object_unref(OBJECT(qdev));
> >>          qerror_report(QERR_DEVICE_INIT_FAILED, driver);
> >>          return NULL;
> >>      }
> >> -- 
> >> 1.8.1.4
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
                        Amos.