[Qemu-devel] bus_unparent() can go into infinite loop

[Qemu-devel] bus_unparent() can go into infinite loop

[Markus Armbruster]

Reproducer (don't ask):

    $ qemu-system-arm -M virt -S -display none -drive if=scsi,id=foo,bus=1,file=tmp.qcow2 -device nec-usb-xhci -device usb-storage,drive=foo -device virtio-scsi-pci
    qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Property 'scsi-disk.drive' can't take value 'foo', it's in use
    qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Setting drive property failed
    qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed

Nevermind the silly error reporting, I got patches to clean that up.

Stuck in bus_unparent()'s loop:

    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
        DeviceState *dev = kid->child;
        object_unparent(OBJECT(dev));
    }

(gdb) bt
#0  bus_unparent (obj=<optimized out>)
    at /home/armbru/work/qemu/hw/core/qdev.c:727
#1  0x000055555583a165 in object_finalize_child_property (obj=<optimized out>, 
    name=<optimized out>, opaque=0x555556450060)
    at /home/armbru/work/qemu/qom/object.c:1079
#2  0x000055555583a40c in object_property_del (obj=0x55555644ff50, 
    name=0x555556696450 "scsi.1", errp=<optimized out>)
    at /home/armbru/work/qemu/qom/object.c:800
#3  0x000055555577f759 in device_unparent (obj=0x55555644ff50)
    at /home/armbru/work/qemu/hw/core/qdev.c:1230
#4  0x000055555583a165 in object_finalize_child_property (obj=<optimized out>, 
    name=<optimized out>, opaque=0x55555644ff50)
    at /home/armbru/work/qemu/qom/object.c:1079
#5  0x000055555583a40c in object_property_del (obj=0x55555644f540, 
    name=0x5555566b9620 "virtio-backend", errp=<optimized out>)
    at /home/armbru/work/qemu/qom/object.c:800
#6  0x00005555557800c6 in qdev_init (dev=dev@entry=0x55555644ff50)
    at /home/armbru/work/qemu/hw/core/qdev.c:186
#7  0x000055555582440e in virtio_scsi_pci_init_pci (vpci_dev=0x55555644f540)
    at /home/armbru/work/qemu/hw/virtio/virtio-pci.c:1157
#8  0x0000555555825fc8 in virtio_pci_init (pci_dev=<optimized out>)
    at /home/armbru/work/qemu/hw/virtio/virtio-pci.c:1018
#9  0x00005555557d6df7 in pci_qdev_init (qdev=0x55555644f540)
    at /home/armbru/work/qemu/hw/pci/pci.c:1775
#10 0x000055555577fbc4 in device_realize (dev=0x55555644f540, 
    errp=0x7fffffffda60) at /home/armbru/work/qemu/hw/core/qdev.c:247
#11 0x000055555578125d in device_set_realized (obj=0x55555644f540, 
    value=<optimized out>, errp=0x7fffffffdb98)
    at /home/armbru/work/qemu/hw/core/qdev.c:1040
#12 0x000055555583927e in property_set_bool (obj=0x55555644f540, 
    v=<optimized out>, opaque=0x555556698850, name=<optimized out>, 
    errp=0x7fffffffdb98) at /home/armbru/work/qemu/qom/object.c:1514
#13 0x000055555583bc67 in object_property_set_qobject (obj=0x55555644f540, 
    value=<optimized out>, name=0x5555559482fd "realized", errp=0x7fffffffdb98)
    at /home/armbru/work/qemu/qom/qom-qobject.c:24
#14 0x000055555583a7b0 in object_property_set_bool (
    obj=obj@entry=0x55555644f540, value=value@entry=true, 
    name=name@entry=0x5555559482fd "realized", errp=errp@entry=0x7fffffffdb98)
    at /home/armbru/work/qemu/qom/object.c:905
#15 0x000055555570d774 in qdev_device_add (opts=0x5555562a38c0)
    at /home/armbru/work/qemu/qdev-monitor.c:574
#16 0x0000555555716c79 in device_init_func (opts=<optimized out>, 
    opaque=<optimized out>) at /home/armbru/work/qemu/vl.c:2127
#17 0x00005555558f48eb in qemu_opts_foreach (list=<optimized out>, func=
    0x555555716c70 <device_init_func>, opaque=0x0, 
    abort_on_failure=<optimized out>)
    at /home/armbru/work/qemu/util/qemu-option.c:1057
#18 0x000055555560e39d in main (argc=<optimized out>, argv=<optimized out>, 
    envp=<optimized out>) at /home/armbru/work/qemu/vl.c:4244
(gdb) p dev->parent_obj.class->type->name
$5 = 0x55555626c3b0 "scsi-disk"
(gdb) p bus->name
$8 = 0x555556696430 "scsi.1"

Re: [Qemu-devel] bus_unparent() can go into infinite loop

[Andreas Färber]

Am 19.02.2015 um 11:45 schrieb Markus Armbruster:
> Reproducer (don't ask):
> 
>     $ qemu-system-arm -M virt -S -display none -drive if=scsi,id=foo,bus=1,file=tmp.qcow2 -device nec-usb-xhci -device usb-storage,drive=foo -device virtio-scsi-pci
>     qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Property 'scsi-disk.drive' can't take value 'foo', it's in use
>     qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Setting drive property failed
>     qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed
> 
> Nevermind the silly error reporting, I got patches to clean that up.

I'm actually more confused about the use of PCI devices with the virt
machine. Does that now feature Alex' PCI controller by default?
Otherwise there would be no bus for those devices.

Is this on master or on top of your PCI realize changes or anything?

> Stuck in bus_unparent()'s loop:
> 
>     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>         DeviceState *dev = kid->child;
>         object_unparent(OBJECT(dev));
>     }

Logically speaking, unparenting on QOM level has nothing to with the bus
children list.
The parent may well be /machine/{unassigned,peripheral{,-anon}}
container objects rather than some BusState object, the latter usually
has link<> properties only for its qdev-level "children".

However I vaguely recall that we shoehorned the unparenting callback to
invoke unrealizing the device. What might happen here is that after
realizing the device failed, realized == false; realized = false in the
unparenting path becomes a no-op then. I.e., the realize error handling
may need to be reviewed to not just return but to undo any changes such
as attaching to some bus (or MemoryRegion, VMState, etc.).

Regards,
Andreas

> (gdb) bt
> #0  bus_unparent (obj=<optimized out>)
>     at /home/armbru/work/qemu/hw/core/qdev.c:727
> #1  0x000055555583a165 in object_finalize_child_property (obj=<optimized out>, 
>     name=<optimized out>, opaque=0x555556450060)
>     at /home/armbru/work/qemu/qom/object.c:1079
> #2  0x000055555583a40c in object_property_del (obj=0x55555644ff50, 
>     name=0x555556696450 "scsi.1", errp=<optimized out>)
>     at /home/armbru/work/qemu/qom/object.c:800
> #3  0x000055555577f759 in device_unparent (obj=0x55555644ff50)
>     at /home/armbru/work/qemu/hw/core/qdev.c:1230
> #4  0x000055555583a165 in object_finalize_child_property (obj=<optimized out>, 
>     name=<optimized out>, opaque=0x55555644ff50)
>     at /home/armbru/work/qemu/qom/object.c:1079
> #5  0x000055555583a40c in object_property_del (obj=0x55555644f540, 
>     name=0x5555566b9620 "virtio-backend", errp=<optimized out>)
>     at /home/armbru/work/qemu/qom/object.c:800
> #6  0x00005555557800c6 in qdev_init (dev=dev@entry=0x55555644ff50)
>     at /home/armbru/work/qemu/hw/core/qdev.c:186
> #7  0x000055555582440e in virtio_scsi_pci_init_pci (vpci_dev=0x55555644f540)
>     at /home/armbru/work/qemu/hw/virtio/virtio-pci.c:1157
> #8  0x0000555555825fc8 in virtio_pci_init (pci_dev=<optimized out>)
>     at /home/armbru/work/qemu/hw/virtio/virtio-pci.c:1018
> #9  0x00005555557d6df7 in pci_qdev_init (qdev=0x55555644f540)
>     at /home/armbru/work/qemu/hw/pci/pci.c:1775
> #10 0x000055555577fbc4 in device_realize (dev=0x55555644f540, 
>     errp=0x7fffffffda60) at /home/armbru/work/qemu/hw/core/qdev.c:247
> #11 0x000055555578125d in device_set_realized (obj=0x55555644f540, 
>     value=<optimized out>, errp=0x7fffffffdb98)
>     at /home/armbru/work/qemu/hw/core/qdev.c:1040
> #12 0x000055555583927e in property_set_bool (obj=0x55555644f540, 
>     v=<optimized out>, opaque=0x555556698850, name=<optimized out>, 
>     errp=0x7fffffffdb98) at /home/armbru/work/qemu/qom/object.c:1514
> #13 0x000055555583bc67 in object_property_set_qobject (obj=0x55555644f540, 
>     value=<optimized out>, name=0x5555559482fd "realized", errp=0x7fffffffdb98)
>     at /home/armbru/work/qemu/qom/qom-qobject.c:24
> #14 0x000055555583a7b0 in object_property_set_bool (
>     obj=obj@entry=0x55555644f540, value=value@entry=true, 
>     name=name@entry=0x5555559482fd "realized", errp=errp@entry=0x7fffffffdb98)
>     at /home/armbru/work/qemu/qom/object.c:905
> #15 0x000055555570d774 in qdev_device_add (opts=0x5555562a38c0)
>     at /home/armbru/work/qemu/qdev-monitor.c:574
> #16 0x0000555555716c79 in device_init_func (opts=<optimized out>, 
>     opaque=<optimized out>) at /home/armbru/work/qemu/vl.c:2127
> #17 0x00005555558f48eb in qemu_opts_foreach (list=<optimized out>, func=
>     0x555555716c70 <device_init_func>, opaque=0x0, 
>     abort_on_failure=<optimized out>)
>     at /home/armbru/work/qemu/util/qemu-option.c:1057
> #18 0x000055555560e39d in main (argc=<optimized out>, argv=<optimized out>, 
>     envp=<optimized out>) at /home/armbru/work/qemu/vl.c:4244
> (gdb) p dev->parent_obj.class->type->name
> $5 = 0x55555626c3b0 "scsi-disk"
> (gdb) p bus->name
> $8 = 0x555556696430 "scsi.1"

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] bus_unparent() can go into infinite loop

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 19.02.2015 um 11:45 schrieb Markus Armbruster:
>> Reproducer (don't ask):
>> 
>>     $ qemu-system-arm -M virt -S -display none -drive
>> if=scsi,id=foo,bus=1,file=tmp.qcow2 -device nec-usb-xhci -device
>> usb-storage,drive=foo -device virtio-scsi-pci
>>     qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2:
>> Property 'scsi-disk.drive' can't take value 'foo', it's in use
>>     qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2:
>> Setting drive property failed
>>     qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed
>> 
>> Nevermind the silly error reporting, I got patches to clean that up.
>
> I'm actually more confused about the use of PCI devices with the virt
> machine. Does that now feature Alex' PCI controller by default?
> Otherwise there would be no bus for those devices.

"info qtree" shows a PCIE bus:

  dev: gpex-pcihost, id ""
    gpio-out "sysbus-irq" 4
    mmio ffffffffffffffff/0000000010000000
    mmio ffffffffffffffff/ffffffffffffffff
    mmio 000000003eff0000/0000000000010000
    bus: pcie.0
      type PCIE
      dev: gpex-root, id ""
        addr = 00.0
        romfile = ""
        rombar = 1 (0x1)
        multifunction = false
        command_serr_enable = true
        class Host bridge, addr 00:00.0, pci id 1b36:0008 (sub 1af4:1100)

> Is this on master or on top of your PCI realize changes or anything?

Unadulterated master (commit cd2d554).

>> Stuck in bus_unparent()'s loop:
>> 
>>     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>>         DeviceState *dev = kid->child;
>>         object_unparent(OBJECT(dev));
>>     }
>
> Logically speaking, unparenting on QOM level has nothing to with the bus
> children list.
> The parent may well be /machine/{unassigned,peripheral{,-anon}}
> container objects rather than some BusState object, the latter usually
> has link<> properties only for its qdev-level "children".
>
> However I vaguely recall that we shoehorned the unparenting callback to
> invoke unrealizing the device. What might happen here is that after
> realizing the device failed, realized == false; realized = false in the
> unparenting path becomes a no-op then. I.e., the realize error handling
> may need to be reviewed to not just return but to undo any changes such
> as attaching to some bus (or MemoryRegion, VMState, etc.).

(gdb) p *dev
$2 = {parent_obj = {class = 0x555556282140, free = 0x7ffff64dcf70 <g_free>, 
    properties = {tqh_first = 0x55555669d860, tqh_last = 0x5555566a1d90}, 
    ref = 1, parent = 0x0}, id = 0x0, realized = false, 
  pending_deleted_event = false, opts = 0x0, hotplugged = 0, 
  parent_bus = 0x555556450060, gpios = {lh_first = 0x0}, child_bus = {
    lh_first = 0x0}, num_child_bus = 0, instance_id_alias = -1, 
  alias_required_for_version = 0}

This is right before object_unparent().

Re: [Qemu-devel] bus_unparent() can go into infinite loop

[Paolo Bonzini]

On 19/02/2015 14:05, Markus Armbruster wrote:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 19.02.2015 um 11:45 schrieb Markus Armbruster:
>>> Reproducer (don't ask):
>>>
>>>     $ qemu-system-arm -M virt -S -display none -drive
>>> if=scsi,id=foo,bus=1,file=tmp.qcow2 -device nec-usb-xhci -device
>>> usb-storage,drive=foo -device virtio-scsi-pci
>>>     qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2:
>>> Property 'scsi-disk.drive' can't take value 'foo', it's in use
>>>     qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2:
>>> Setting drive property failed
>>>     qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed
>>>
>>> Nevermind the silly error reporting, I got patches to clean that up.
>>
>> I'm actually more confused about the use of PCI devices with the virt
>> machine. Does that now feature Alex' PCI controller by default?
>> Otherwise there would be no bus for those devices.
> 
> "info qtree" shows a PCIE bus:
> 
>   dev: gpex-pcihost, id ""
>     gpio-out "sysbus-irq" 4
>     mmio ffffffffffffffff/0000000010000000
>     mmio ffffffffffffffff/ffffffffffffffff
>     mmio 000000003eff0000/0000000000010000
>     bus: pcie.0
>       type PCIE
>       dev: gpex-root, id ""
>         addr = 00.0
>         romfile = ""
>         rombar = 1 (0x1)
>         multifunction = false
>         command_serr_enable = true
>         class Host bridge, addr 00:00.0, pci id 1b36:0008 (sub 1af4:1100)
> 
>> Is this on master or on top of your PCI realize changes or anything?
> 
> Unadulterated master (commit cd2d554).
> 
>>> Stuck in bus_unparent()'s loop:
>>>
>>>     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>>>         DeviceState *dev = kid->child;
>>>         object_unparent(OBJECT(dev));
>>>     }
>>
>> Logically speaking, unparenting on QOM level has nothing to with the bus
>> children list.
>> The parent may well be /machine/{unassigned,peripheral{,-anon}}
>> container objects rather than some BusState object, the latter usually
>> has link<> properties only for its qdev-level "children".
>>
>> However I vaguely recall that we shoehorned the unparenting callback to
>> invoke unrealizing the device. What might happen here is that after
>> realizing the device failed, realized == false; realized = false in the
>> unparenting path becomes a no-op then. I.e., the realize error handling
>> may need to be reviewed to not just return but to undo any changes such
>> as attaching to some bus (or MemoryRegion, VMState, etc.).
> 
> (gdb) p *dev
> $2 = {parent_obj = {class = 0x555556282140, free = 0x7ffff64dcf70 <g_free>, 
>     properties = {tqh_first = 0x55555669d860, tqh_last = 0x5555566a1d90}, 
>     ref = 1, parent = 0x0}, id = 0x0, realized = false, 
>   pending_deleted_event = false, opts = 0x0, hotplugged = 0, 
>   parent_bus = 0x555556450060, gpios = {lh_first = 0x0}, child_bus = {
>     lh_first = 0x0}, num_child_bus = 0, instance_id_alias = -1, 
>   alias_required_for_version = 0}
> 
> This is right before object_unparent().

The bug is that the device is not given a parent at all by
scsi_bus_legacy_add_drive, unlike for example qdev_device_add.

There is a "safety" net that adds the drive to the QOM tree in
qdev_init, but it isn't enough if you abort creation of the device
before qdev_init.  This applies to qdev_create and its callers
isa_create, usb_create, and also to qdev_try_create/isa_try_create.

I tried "git grep -lw FUNC_NAME | xargs grep object_unparent" and there
should be no other problematic case than scsi_bus_legacy_add_drive.

Patch will follow.

Paolo