[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-ppc] [PATCH 5/7] qdev: Protect device-list-properties against brok
|
From: |
Markus Armbruster |
|
Subject: |
[Qemu-ppc] [PATCH 5/7] qdev: Protect device-list-properties against broken devices |
|
Date: |
Fri, 18 Sep 2015 14:00:38 +0200 |
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_even_create_with_object_new_yet
to mark them:
* Crash or hang during cleanup (didn't debug them, so I can't say
why): "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
"s390-sclp-event-facility", "sclp"
* Dangling pointers: all CPUs, plus "allwinner-a10", "digic",
"fsl,imx25", "fsl,imx31", "xlnx,zynqmp", because they create CPUs
* Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
"host-powerpc64-cpu", "host-embedded-powerpc-cpu",
"host-powerpc-cpu"
Make qmp_device_list_properties() fail cleanly when the device is so
marked. This improves device-list-properties from "crashes or hangs"
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-* -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" <address@hidden>
Cc: Alexander Graf <address@hidden>
Cc: Alistair Francis <address@hidden>
Cc: Antony Pavlov <address@hidden>
Cc: Christian Borntraeger <address@hidden>
Cc: Cornelia Huck <address@hidden>
Cc: Eduardo Habkost <address@hidden>
Cc: Li Guang <address@hidden>
Cc: Paolo Bonzini <address@hidden>
Cc: Peter Crosthwaite <address@hidden>
Cc: Peter Maydell <address@hidden>
Cc: Richard Henderson <address@hidden>
Cc: address@hidden
Cc: address@hidden
Signed-off-by: Markus Armbruster <address@hidden>
---
hw/arm/allwinner-a10.c | 2 ++
hw/arm/digic.c | 2 ++
hw/arm/fsl-imx25.c | 2 ++
hw/arm/fsl-imx31.c | 2 ++
hw/arm/xlnx-zynqmp.c | 2 ++
hw/pci-host/versatile.c | 11 +++++++++++
hw/pcmcia/pxa2xx.c | 9 +++++++++
hw/s390x/event-facility.c | 3 +++
hw/s390x/sclp.c | 3 +++
include/hw/qdev-core.h | 13 +++++++++++++
qmp.c | 5 +++++
qom/cpu.c | 2 ++
target-i386/cpu.c | 2 ++
target-ppc/kvm.c | 4 ++++
tests/device-introspect-test.c | 29 -----------------------------
15 files changed, 62 insertions(+), 29 deletions(-)
diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index ff249af..7692090 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -103,6 +103,8 @@ static void aw_a10_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = aw_a10_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo aw_a10_type_info = {
diff --git a/hw/arm/digic.c b/hw/arm/digic.c
index ec8c330..3decef4 100644
--- a/hw/arm/digic.c
+++ b/hw/arm/digic.c
@@ -97,6 +97,8 @@ static void digic_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = digic_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo digic_type_info = {
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 86fde42..13c06b2 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -284,6 +284,8 @@ static void fsl_imx25_class_init(ObjectClass *oc, void
*data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = fsl_imx25_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_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..7cb8fd4 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -258,6 +258,8 @@ static void fsl_imx31_class_init(ObjectClass *oc, void
*data)
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = fsl_imx31_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo fsl_imx31_type_info = {
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 2185542..d558b10 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -271,6 +271,8 @@ static void xlnx_zynqmp_class_init(ObjectClass *oc, void
*data)
dc->props = xlnx_zynqmp_props;
dc->realize = xlnx_zynqmp_realize;
+ /* Reason: creates a CPU, thus use after free(), see cpu_class_init() */
+ dc->cannot_even_create_with_object_new_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..f28a115 100644
--- a/hw/pci-host/versatile.c
+++ b/hw/pci-host/versatile.c
@@ -500,6 +500,8 @@ static void pci_vpb_class_init(ObjectClass *klass, void
*data)
dc->reset = pci_vpb_reset;
dc->vmsd = &pci_vpb_vmstate;
dc->props = pci_vpb_properties;
+ /* Reason: object_unref() hangs */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo pci_vpb_info = {
@@ -521,10 +523,19 @@ static void pci_realview_init(Object *obj)
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_even_create_with_object_new_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/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index a7e1877..c050c41 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -249,11 +249,20 @@ void pxa2xx_pcmcia_set_irq_cb(void *opaque, qemu_irq irq,
qemu_irq cd_irq)
s->cd_irq = cd_irq;
}
+static void pxa2xx_pcmcia_class_init(ObjectClass *class, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(class);
+
+ /* Reason: object_unref() crashes */
+ dc->cannot_even_create_with_object_new_yet = true;
+}
+
static const TypeInfo pxa2xx_pcmcia_type_info = {
.name = TYPE_PXA2XX_PCMCIA,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(PXA2xxPCMCIAState),
.instance_init = pxa2xx_pcmcia_initfn,
+ .class_init = pxa2xx_pcmcia_class_init,
};
static void pxa2xx_pcmcia_register_types(void)
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index ef2a051..8fa361d 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -381,6 +381,9 @@ static void init_event_facility_class(ObjectClass *klass,
void *data)
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
k->command_handler = command_handler;
k->event_pending = event_pending;
+
+ /* Reason: object_unref() hangs */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo sclp_event_facility_info = {
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index fd277e1..b2b46c9 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -562,6 +562,9 @@ static void sclp_class_init(ObjectClass *oc, void *data)
sc->read_cpu_info = sclp_read_cpu_info;
sc->execute = sclp_execute;
sc->service_interrupt = service_interrupt;
+
+ /* Reason: object_unref() hangs */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static TypeInfo sclp_info = {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 038b54d..bc30cca 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -114,6 +114,19 @@ typedef struct DeviceClass {
* 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_even_create_with_object_new_yet;
+
bool hotpluggable;
/* callbacks */
diff --git a/qmp.c b/qmp.c
index 6f370d5..257f09f 100644
--- a/qmp.c
+++ b/qmp.c
@@ -526,6 +526,11 @@ DevicePropertyInfoList *qmp_device_list_properties(const
char *typename,
return NULL;
}
+ if (DEVICE_CLASS(klass)->cannot_even_create_with_object_new_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/qom/cpu.c b/qom/cpu.c
index fb80d13..5ff9ea7 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -361,6 +361,8 @@ static void cpu_class_init(ObjectClass *klass, void *data)
* IRQs, adding reset handlers, halting non-first CPUs, ...
*/
dc->cannot_instantiate_with_device_add_yet = true;
+ /* Reason: use after free: cpu_exec_init() saves CPUState in cpus */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static const TypeInfo cpu_type_info = {
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7c52714..32e7b84 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1449,6 +1449,8 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void
*data)
*/
dc->props = host_x86_cpu_properties;
+ /* Reason: host_x86_cpu_initfn() dies when !kvm_enabled() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
static void host_x86_cpu_initfn(Object *obj)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 110436d..9943bba 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2188,6 +2188,7 @@ static void kvmppc_host_cpu_initfn(Object *obj)
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();
@@ -2214,6 +2215,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc,
void *data)
if (icache_size != -1) {
pcc->l1_icache_size = icache_size;
}
+
+ /* Reason: kvmppc_host_cpu_initfn() dies when !kvm_enabled() */
+ dc->cannot_even_create_with_object_new_yet = true;
}
bool kvmppc_has_cap_epr(void)
diff --git a/tests/device-introspect-test.c b/tests/device-introspect-test.c
index 3e40877..ca82f0c 100644
--- a/tests/device-introspect-test.c
+++ b/tests/device-introspect-test.c
@@ -84,32 +84,6 @@ static void test_device_intro_abstract(void)
qtest_end();
}
-static bool blacklisted(const char *type)
-{
- static const char *blacklist[] = {
- /* crash in object_unref(): */
- "pxa2xx-pcmcia",
- /* hang in object_unref(): */
- "realview_pci", "versatile_pci", "s390-sclp-event-facility", "sclp",
- /* 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;
@@ -123,9 +97,6 @@ static void test_device_intro_concrete(void)
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);
}
--
2.4.3