Am 18.09.2015 um 14:00 schrieb Markus Armbruster:
> 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"
Ack for the sclp things. Theses devices are created by the machine and
sclp creates the event-facility, so not having a way to query properties
for these devices is better than a hang.
David, can you have a look on why these devices fail as outlined?
>
> * 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" <afaer...@suse.de>
> Cc: Alexander Graf <ag...@suse.de>
> Cc: Alistair Francis <alistair.fran...@xilinx.com>
> Cc: Antony Pavlov <antonynpav...@gmail.com>
> Cc: Christian Borntraeger <borntrae...@de.ibm.com>
> Cc: Cornelia Huck <cornelia.h...@de.ibm.com>
> Cc: Eduardo Habkost <ehabk...@redhat.com>
> Cc: Li Guang <lig.f...@cn.fujitsu.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Peter Crosthwaite <crosthwaite.pe...@gmail.com>
> Cc: Peter Maydell <peter.mayd...@linaro.org>
> Cc: Richard Henderson <r...@twiddle.net>
> Cc: qemu-...@nongnu.org
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> 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);
> }
>
