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]>
---
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 e2aab3d1fc61..0b0f2f47fa78 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 20eae38e03f4..77c3eee71713 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.55.0
