Infinite loop in bus_unparent(), qdev bug or qdev misuse?

[Markus Armbruster]

I stumbled over this while working on a feature branch.  Instead of
throwing the whole branch at you as a reproducer, I give you a mock up.

This is fdctrl_connect_drives():

        dev = qdev_create(&fdctrl->bus.bus, "floppy");
        qdev_prop_set_uint32(dev, "unit", i);
        qdev_prop_set_enum(dev, "drive-type", fdctrl->qdev_for_drives[i].type);

        blk_ref(blk);
        blk_detach_dev(blk, fdc_dev);
        fdctrl->qdev_for_drives[i].blk = NULL;
        qdev_prop_set_drive(dev, "drive", blk, &local_err);
        blk_unref(blk);

        if (local_err) {
            error_propagate(errp, local_err);
            return;
        }

        object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
        if (local_err) {
            error_propagate(errp, local_err);
            return;
        }

What if qdev_prop_set_drive() fails?  I don't have a reproducer ready (I
do on my feature branch), so let's mock it, and also instrument the
place where things go wrong.  Patch appended.  To try it, run
qemu-system-x86_64 without arguments.

Turns out the failure bubbles up into device_set_realized() for the
isa-fdc, where the cleanup code calls object_unparent().  This unparents
children, and ends up in bus_unparent() for the isa-fdc's floppy-bus:

    #4  0x0000555555abdb7f in bus_unparent (obj=0x55555675a9f0)
        at /work/armbru/qemu/hw/core/bus.c:148
    #5  0x0000555555d2aea6 in object_finalize_child_property
        (obj=0x55555675a800, name=0x555557281230 "floppy-bus.0", 
opaque=0x55555675a9f0) at /work/armbru/qemu/qom/object.c:1672
    #6  0x0000555555d2872b in object_property_del_child
        (obj=0x55555675a800, child=0x55555675a9f0, errp=0x0)
        at /work/armbru/qemu/qom/object.c:628
    #7  0x0000555555d2880b in object_unparent (obj=0x55555675a9f0)
        at /work/armbru/qemu/qom/object.c:647
    #8  0x0000555555ab9e10 in device_unparent (obj=0x55555675a800)
        at /work/armbru/qemu/hw/core/qdev.c:1101

This loop there

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

makes no progreess because OBJECT(dev)->parent is still null, and
therefore object_unparent() does nothing.

Possible culprit: qdev_try_create() calls qdev_set_parent_bus(), which
adds the device to the bus, but leaves ->parent null.  If this isn't
wrong outright, it's at least a dangerous state.

Work-around: call qdev_set_id(dev, NULL) right after qdev_create().
This sets ->parent.


>From 2554db096866138a85482d683e57a38166bb425b Mon Sep 17 00:00:00 2001
From: Markus Armbruster <address@hidden>
Date: Mon, 4 May 2020 15:58:10 +0200
Subject: [PATCH] qdev: Hack to reproduce infinite loop in bus_unparent()

---
 hw/block/fdc.c | 4 ++++
 hw/core/bus.c  | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 9628cc171e..f57558eea4 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2523,7 +2523,11 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, 
DeviceState *fdc_dev,
         blk_ref(blk);
         blk_detach_dev(blk, fdc_dev);
         fdctrl->qdev_for_drives[i].blk = NULL;
+#if 0
         qdev_prop_set_drive(dev, "drive", blk, &local_err);
+#else
+        error_setg(&local_err, "hack");
+#endif
         blk_unref(blk);
 
         if (local_err) {
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 3dc0a825f0..3620a7be54 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -135,12 +135,17 @@ static void bus_unparent(Object *obj)
     BusState *bus = BUS(obj);
     BusChild *kid;
 
+    printf("### %s bus=%p %s\n",
+           __func__, obj, object_get_typename(obj));
     /* Only the main system bus has no parent, and that bus is never freed */
     assert(bus->parent);
 
     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
         DeviceState *dev = kid->child;
+        printf("### %s kid=%p %s\n",
+               __func__, OBJECT(dev), object_get_typename(OBJECT(dev)));
         object_unparent(OBJECT(dev));
+        assert(kid != QTAILQ_FIRST(&bus->children));
     }
     QLIST_REMOVE(bus, sibling);
     bus->parent->num_child_bus--;
-- 
2.21.1