qdev: Protect device-list-properties against broken devices
Several devices don't survive object_unref(object_new(T)): they crash
or hang during cleanup, or they leave dangling pointers behind.
This breaks at least device-list-properties, because
qmp_device_list_properties() needs to create a device to find its
properties. Broken in commit f4eb32b "qmp: show QOM properties in
device-list-properties", v2.1. Example reproducer:
$ qemu-system-aarch64 -nodefaults -display none -machine none -S -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": ""}, "capabilities": []}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "device-list-properties", "arguments": { "typename": "pxa2xx-pcmcia" } }
qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
Aborted (core dumped)
[Exit 134 (SIGABRT)]
Unfortunately, I can't fix the problems in these devices right now.
Instead, add DeviceClass member cannot_destroy_with_object_finalize_yet
to mark them:
* Hang during cleanup (didn't debug, so I can't say why):
"realview_pci", "versatile_pci".
* Dangling pointer in cpus: most CPUs, plus "allwinner-a10", "digic",
"fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create such
CPUs
* Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
"host-powerpc64-cpu", "host-embedded-powerpc-cpu",
"host-powerpc-cpu" (the powerpc ones can't currently reach the
assertion, because the CPUs are only registered when KVM is enabled,
but the assertion is arguably in the wrong place all the same)
Make qmp_device_list_properties() fail cleanly when the device is so
marked. This improves device-list-properties from "crashes, hangs or
leaves dangling pointers behind" to "fails". Not a complete fix, just
a better-than-nothing work-around. In the above reproducer,
device-list-properties now fails with "Can't list properties of device
'pxa2xx-pcmcia'".
This also protects -device FOO,help, which uses the same machinery
since commit ef52358 "qdev-monitor: include QOM properties in -device
FOO, help output", v2.2. Example reproducer:
$ qemu-system-aarch64 -machine none -device pxa2xx-pcmcia,help
Before:
qemu-system-aarch64: .../memory.c:1307: memory_region_finalize: Assertion `((&mr->subregions)->tqh_first == ((void *)0))' failed.
After:
Can't list properties of device 'pxa2xx-pcmcia'
Cc: "Andreas Färber" <afaerber@suse.de>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Anthony Green <green@moxielogic.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Cc: Jia Liu <proljc@gmail.com>
Cc: Leon Alrae <leon.alrae@imgtec.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Michael Walle <michael@walle.cc>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Richard Henderson <rth@twiddle.net>
Cc: qemu-ppc@nongnu.org
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Message-Id: <1443689999-12182-10-git-send-email-armbru@redhat.com>
diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index ff249af..43dc0a1 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -103,6 +103,12 @@
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = aw_a10_realize;
+
+ /*
+ * Reason: creates an ARM CPU, thus use after free(), see
+ * arm_cpu_class_init()
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo aw_a10_type_info = {
diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index ec8c330..90f8190 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -97,6 +97,12 @@
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = digic_realize;
+
+ /*
+ * Reason: creates an ARM CPU, thus use after free(), see
+ * arm_cpu_class_init()
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo digic_type_info = {
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 86fde42..e1cadac 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -284,6 +284,12 @@
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = fsl_imx25_realize;
+
+ /*
+ * Reason: creates an ARM CPU, thus use after free(), see
+ * arm_cpu_class_init()
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo fsl_imx25_type_info = {
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 8e1ed48..53d4473 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -258,6 +258,12 @@
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = fsl_imx31_realize;
+
+ /*
+ * Reason: creates an ARM CPU, thus use after free(), see
+ * arm_cpu_class_init()
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo fsl_imx31_type_info = {
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index a9097f9..b36ca3d 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -271,6 +271,12 @@
dc->props = xlnx_zynqmp_props;
dc->realize = xlnx_zynqmp_realize;
+
+ /*
+ * Reason: creates an ARM CPU, thus use after free(), see
+ * arm_cpu_class_init()
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo xlnx_zynqmp_type_info = {
diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c
index 6d23553..7172b90 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -500,6 +500,8 @@
dc->reset = pci_vpb_reset;
dc->vmsd = &pci_vpb_vmstate;
dc->props = pci_vpb_properties;
+ /* Reason: object_unref() hangs */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo pci_vpb_info = {
@@ -521,10 +523,19 @@
s->mem_win_size[2] = 0x08000000;
}
+static void pci_realview_class_init(ObjectClass *class, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(class);
+
+ /* Reason: object_unref() hangs */
+ dc->cannot_destroy_with_object_finalize_yet = true;
+}
+
static const TypeInfo pci_realview_info = {
.name = "realview_pci",
.parent = TYPE_VERSATILE_PCI,
.instance_init = pci_realview_init,
+ .class_init = pci_realview_class_init,
};
static void versatile_pci_register_types(void)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 038b54d..8057aeda 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -114,6 +114,19 @@
* TODO remove once we're there
*/
bool cannot_instantiate_with_device_add_yet;
+ /*
+ * Does this device model survive object_unref(object_new(TNAME))?
+ * All device models should, and this flag shouldn't exist. Some
+ * devices crash in object_new(), some crash or hang in
+ * object_unref(). Makes introspecting properties with
+ * qmp_device_list_properties() dangerous. Bad, because it's used
+ * by -device FOO,help. This flag serves to protect that code.
+ * It should never be set without a comment explaining why it is
+ * set.
+ * TODO remove once we're there
+ */
+ bool cannot_destroy_with_object_finalize_yet;
+
bool hotpluggable;
/* callbacks */
diff --git a/qmp.c b/qmp.c
index 1413de4..d9ecede 100644
--- a/qmp.c
+++ b/qmp.c
@@ -521,6 +521,11 @@
return NULL;
}
+ if (DEVICE_CLASS(klass)->cannot_destroy_with_object_finalize_yet) {
+ error_setg(errp, "Can't list properties of device '%s'", typename);
+ return NULL;
+ }
+
obj = object_new(typename);
QTAILQ_FOREACH(prop, &obj->properties, node) {
diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
index 421d7e5..ff1926a 100644
--- a/target-alpha/cpu.c
+++ b/target-alpha/cpu.c
@@ -298,6 +298,13 @@
dc->vmsd = &vmstate_alpha_cpu;
#endif
cc->gdb_num_core_regs = 67;
+
+ /*
+ * Reason: alpha_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo alpha_cpu_type_info = {
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index d7b4445..30739fc 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1427,6 +1427,17 @@
cc->debug_excp_handler = arm_debug_excp_handler;
cc->disas_set_info = arm_disas_set_info;
+
+ /*
+ * Reason: arm_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ *
+ * Once this is fixed, the devices that create ARM CPUs should be
+ * updated not to set cannot_destroy_with_object_finalize_yet,
+ * unless they still screw up something else.
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static void cpu_register(const ARMCPUInfo *info)
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index d461e07..8eaf5a5 100644
--- a/target-cris/cpu.c
+++ b/target-cris/cpu.c
@@ -309,6 +309,13 @@
cc->gdb_stop_before_watchpoint = true;
cc->disas_set_info = cris_disas_set_info;
+
+ /*
+ * Reason: cris_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo cris_cpu_type_info = {
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c793812..05d7f26 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1453,6 +1453,8 @@
*/
dc->props = host_x86_cpu_properties;
+ /* Reason: host_x86_cpu_initfn() dies when !kvm_enabled() */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static void host_x86_cpu_initfn(Object *obj)
@@ -3190,6 +3192,12 @@
#endif
cc->cpu_exec_enter = x86_cpu_exec_enter;
cc->cpu_exec_exit = x86_cpu_exec_exit;
+
+ /*
+ * Reason: x86_cpu_initfn() calls cpu_exec_init(), which saves the
+ * object in cpus -> dangling pointer after final object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo x86_cpu_type_info = {
diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
index c2b77c6..d0ab278 100644
--- a/target-lm32/cpu.c
+++ b/target-lm32/cpu.c
@@ -275,6 +275,13 @@
cc->gdb_num_core_regs = 32 + 7;
cc->gdb_stop_before_watchpoint = true;
cc->debug_excp_handler = lm32_debug_excp_handler;
+
+ /*
+ * Reason: lm32_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static void lm32_register_cpu_type(const LM32CPUInfo *info)
diff --git a/target-m68k/cpu.c b/target-m68k/cpu.c
index 4f246da..97527ef 100644
--- a/target-m68k/cpu.c
+++ b/target-m68k/cpu.c
@@ -212,6 +212,13 @@
dc->vmsd = &vmstate_m68k_cpu;
cc->gdb_num_core_regs = 18;
cc->gdb_core_xml_file = "cf-core.xml";
+
+ /*
+ * Reason: m68k_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static void register_cpu_type(const M68kCPUInfo *info)
diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
index cbd84a2..52959e1 100644
--- a/target-microblaze/cpu.c
+++ b/target-microblaze/cpu.c
@@ -264,6 +264,12 @@
cc->gdb_num_core_regs = 32 + 5;
cc->disas_set_info = mb_disas_set_info;
+
+ /*
+ * Reason: mb_cpu_initfn() calls cpu_exec_init(), which saves the
+ * object in cpus -> dangling pointer after final object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo mb_cpu_type_info = {
diff --git a/target-mips/cpu.c b/target-mips/cpu.c
index 4027d0f..7fe1f04 100644
--- a/target-mips/cpu.c
+++ b/target-mips/cpu.c
@@ -153,6 +153,13 @@
cc->gdb_num_core_regs = 73;
cc->gdb_stop_before_watchpoint = true;
+
+ /*
+ * Reason: mips_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo mips_cpu_type_info = {
diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
index 6b035aa..3af3779 100644
--- a/target-moxie/cpu.c
+++ b/target-moxie/cpu.c
@@ -114,6 +114,13 @@
cc->get_phys_page_debug = moxie_cpu_get_phys_page_debug;
cc->vmsd = &vmstate_moxie_cpu;
#endif
+
+ /*
+ * Reason: moxie_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static void moxielite_initfn(Object *obj)
diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
index d97f3c0..cc5e2d1 100644
--- a/target-openrisc/cpu.c
+++ b/target-openrisc/cpu.c
@@ -177,6 +177,13 @@
dc->vmsd = &vmstate_openrisc_cpu;
#endif
cc->gdb_num_core_regs = 32 + 3;
+
+ /*
+ * Reason: openrisc_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static void cpu_register(const OpenRISCCPUInfo *info)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index f8ea783..7276299 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2192,6 +2192,7 @@
static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
{
+ DeviceClass *dc = DEVICE_CLASS(oc);
PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
uint32_t vmx = kvmppc_get_vmx();
uint32_t dfp = kvmppc_get_dfp();
@@ -2218,6 +2219,9 @@
if (icache_size != -1) {
pcc->l1_icache_size = icache_size;
}
+
+ /* Reason: kvmppc_host_cpu_initfn() dies when !kvm_enabled() */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
bool kvmppc_has_cap_epr(void)
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index c3e21b4..ccfaa8a 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -353,6 +353,13 @@
#endif
cc->gdb_num_core_regs = S390_NUM_CORE_REGS;
cc->gdb_core_xml_file = "s390x-core64.xml";
+
+ /*
+ * Reason: s390_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo s390_cpu_type_info = {
diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
index 5c65ab4..64e4467 100644
--- a/target-sh4/cpu.c
+++ b/target-sh4/cpu.c
@@ -290,6 +290,13 @@
#endif
dc->vmsd = &vmstate_sh_cpu;
cc->gdb_num_core_regs = 59;
+
+ /*
+ * Reason: superh_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo superh_cpu_type_info = {
diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index 9528e3a..82bb72a 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -854,6 +854,13 @@
#else
cc->gdb_num_core_regs = 72;
#endif
+
+ /*
+ * Reason: sparc_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo sparc_cpu_type_info = {
diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c
index 3c5481d..c249704 100644
--- a/target-tilegx/cpu.c
+++ b/target-tilegx/cpu.c
@@ -159,6 +159,13 @@
cc->set_pc = tilegx_cpu_set_pc;
cc->handle_mmu_fault = tilegx_cpu_handle_mmu_fault;
cc->gdb_num_core_regs = 0;
+
+ /*
+ * Reason: tilegx_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo tilegx_cpu_type_info = {
diff --git a/target-tricore/cpu.c b/target-tricore/cpu.c
index 2029ef6..ed8b030 100644
--- a/target-tricore/cpu.c
+++ b/target-tricore/cpu.c
@@ -170,6 +170,12 @@
cc->set_pc = tricore_cpu_set_pc;
cc->synchronize_from_tb = tricore_cpu_synchronize_from_tb;
+ /*
+ * Reason: tricore_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static void cpu_register(const TriCoreCPUInfo *info)
diff --git a/target-unicore32/cpu.c b/target-unicore32/cpu.c
index fc451a1..e5252eb 100644
--- a/target-unicore32/cpu.c
+++ b/target-unicore32/cpu.c
@@ -155,6 +155,13 @@
cc->get_phys_page_debug = uc32_cpu_get_phys_page_debug;
#endif
dc->vmsd = &vmstate_uc32_cpu;
+
+ /*
+ * Reason: uc32_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static void uc32_register_cpu_type(const UniCore32CPUInfo *info)
diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
index da8129d..4e49bee 100644
--- a/target-xtensa/cpu.c
+++ b/target-xtensa/cpu.c
@@ -155,6 +155,13 @@
#endif
cc->debug_excp_handler = xtensa_breakpoint_handler;
dc->vmsd = &vmstate_xtensa_cpu;
+
+ /*
+ * Reason: xtensa_cpu_initfn() calls cpu_exec_init(), which saves
+ * the object in cpus -> dangling pointer after final
+ * object_unref().
+ */
+ dc->cannot_destroy_with_object_finalize_yet = true;
}
static const TypeInfo xtensa_cpu_type_info = {
diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index 8a27b4a..11d5fea 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -91,30 +91,6 @@
qtest_end();
}
-static bool blacklisted(const char *type)
-{
- static const char *blacklist[] = {
- /* hang in object_unref(): */
- "realview_pci", "versatile_pci",
- /* create a CPU, thus use after free (see below): */
- "allwinner-a10", "digic", "fsl,imx25", "fsl,imx31", "xlnx,zynqmp",
- };
- size_t len = strlen(type);
- int i;
-
- if (len >= 4 && !strcmp(type + len - 4, "-cpu")) {
- /* use after free: cpu_exec_init() saves CPUState in cpus */
- return true;
- }
-
- for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
- if (!strcmp(blacklist[i], type)) {
- return true;
- }
- }
- return false;
-}
-
static void test_device_intro_concrete(void)
{
QList *types;
@@ -128,9 +104,6 @@
type = qdict_get_try_str(qobject_to_qdict(qlist_entry_obj(entry)),
"name");
g_assert(type);
- if (blacklisted(type)) {
- continue; /* FIXME broken device, skip */
- }
test_one_device(type);
}
