[PULL 15/56] hw/qdev: Parent device before setting parent bus

From: Akihiko Odaki <[email protected]>

Commit 9940b2cfbc05 ("qdev: New qdev_new(), qdev_realize(), etc.") says
"device state 'no QOM parent, but plugged into bus' is dangerous". In
such a case, unrealizing the bus will hang in bus_unparent():

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

object_unparent() does nothing when its argument has no QOM parent,
and the loop spins forever.

However, that commit did not completely eliminate such a situation.
When the device is not parented, device_set_realized() lets
/machine/unattached parent it, but it happens after setting parent bus.
Therefore, any failure between the two operations can leave the device
in a dangerous state.

qdev_realize() at least asserts that the device is not already realized
and prevents one realization failure pattern, but it is not
comprehensive. Besides, it will trip with a command line like the
following:

    qemu-system-x86_64 -M none -nodefaults -nographic \
        -device ipmi-bmc-sim,realized=on

Eliminate the dangerous state by ensuring that the device is parented
before calling qdev_set_parent_bus(). Also, stop asserting that the
device is not already realized in qdev_realize(); it is broken and
no longer serves any purpose.

Fixes: 9940b2cfbc05 ("qdev: New qdev_new(), qdev_realize(), etc.")
Signed-off-by: Akihiko Odaki <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
Message-ID: <[email protected]>
---
 hw/core/qdev.c         | 51 ++++++++++++++++++++++++------------------
 tests/unit/test-qdev.c | 13 +++++++++++
 2 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e2aab3d1fc6..0b0f2f47fa7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -264,17 +264,43 @@ static void device_reset_child_foreach(Object *obj, ResettableChildCallback cb,
 
 bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
 {
-    assert(!dev->realized && !dev->parent_bus);
+    static int unattached_count;
+    bool unattached_parent = false;
+
+    assert(!dev->parent_bus);
+
+    if (!OBJECT(dev)->parent) {
+        gchar *name = g_strdup_printf("device[%d]", unattached_count++);
+
+        object_property_add_child(machine_get_container("unattached"),
+                                  name, OBJECT(dev));
+        unattached_parent = true;
+        g_free(name);
+    }
 
     if (bus) {
         if (!qdev_set_parent_bus(dev, bus, errp)) {
-            return false;
+            goto fail;
         }
     } else {
         assert(!DEVICE_GET_CLASS(dev)->bus_type);
     }
 
-    return object_property_set_bool(OBJECT(dev), "realized", true, errp);
+    if (object_property_set_bool(OBJECT(dev), "realized", true, errp)) {
+        return true;
+    }
+
+fail:
+    if (unattached_parent) {
+        /*
+         * Beware, this doesn't just revert
+         * object_property_add_child(), it also runs bus_remove()!
+         */
+        object_unparent(OBJECT(dev));
+        unattached_count--;
+    }
+
+    return false;
 }
 
 bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp)
@@ -479,8 +505,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     BusState *bus;
     NamedClockList *ncl;
     Error *local_err = NULL;
-    bool unattached_parent = false;
-    static int unattached_count;
 
     if (dev->hotplugged && !dc->hotpluggable) {
         error_setg(errp, "Device '%s' does not support hotplugging",
@@ -493,15 +517,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             goto fail;
         }
 
-        if (!obj->parent) {
-            gchar *name = g_strdup_printf("device[%d]", unattached_count++);
-
-            object_property_add_child(machine_get_container("unattached"),
-                                      name, obj);
-            unattached_parent = true;
-            g_free(name);
-        }
-
         hotplug_ctrl = qdev_get_hotplug_handler(dev);
         if (hotplug_ctrl) {
             hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err);
@@ -627,14 +642,6 @@ post_realize_fail:
 
 fail:
     error_propagate(errp, local_err);
-    if (unattached_parent) {
-        /*
-         * Beware, this doesn't just revert
-         * object_property_add_child(), it also runs bus_remove()!
-         */
-        object_unparent(OBJECT(dev));
-        unattached_count--;
-    }
 }
 
 static bool device_get_hotpluggable(Object *obj, Error **errp)
diff --git a/tests/unit/test-qdev.c b/tests/unit/test-qdev.c
index 20eae38e03f..77c3eee7171 100644
--- a/tests/unit/test-qdev.c
+++ b/tests/unit/test-qdev.c
@@ -78,6 +78,16 @@ static void test_qdev_free_properties(void)
     object_unref(mt);
 }
 
+static void test_qdev_double_realization(void)
+{
+    MyDev *mt = STATIC_TYPE(object_new(TYPE_MY_DEV));
+
+    qdev_realize(DEVICE(mt), NULL, &error_fatal);
+    qdev_realize(DEVICE(mt), NULL, &error_fatal);
+    object_unparent(OBJECT(mt));
+    object_unref(OBJECT(mt));
+}
+
 
 int main(int argc, char **argv)
 {
@@ -90,6 +100,9 @@ int main(int argc, char **argv)
     g_test_add_func("/qdev/free-properties",
                     test_qdev_free_properties);
 
+    g_test_add_func("/qdev/double-realization",
+                    test_qdev_double_realization);
+
     g_test_run();
 
     return 0;
-- 
2.53.0