From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44683) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcuLR-0005om-RP for qemu-devel@nongnu.org; Fri, 18 Sep 2015 08:00:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZcuLP-0007Eg-1H for qemu-devel@nongnu.org; Fri, 18 Sep 2015 08:00:49 -0400 From: Markus Armbruster Date: Fri, 18 Sep 2015 14:00:38 +0200 Message-Id: <1442577640-11612-6-git-send-email-armbru@redhat.com> In-Reply-To: <1442577640-11612-1-git-send-email-armbru@redhat.com> References: <1442577640-11612-1-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Peter Maydell , ehabkost@redhat.com, Peter Crosthwaite , qemu-stable@nongnu.org, Alexander Graf , Alistair Francis , Christian Borntraeger , qemu-ppc@nongnu.org, Antony Pavlov , stefanha@redhat.com, Cornelia Huck , Paolo Bonzini , afaerber@suse.de, Li Guang , Richard Henderson 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": "px= a2xx-pcmcia" } } qemu-system-aarch64: /home/armbru/work/qemu/memory.c:1307: memory_reg= ion_finalize: Assertion `((&mr->subregions)->tqh_first =3D=3D ((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: Asser= tion `((&mr->subregions)->tqh_first =3D=3D ((void *)0))' failed. After: Can't list properties of device 'pxa2xx-pcmcia' Cc: "Andreas F=C3=A4rber" Cc: Alexander Graf Cc: Alistair Francis Cc: Antony Pavlov Cc: Christian Borntraeger Cc: Cornelia Huck Cc: Eduardo Habkost Cc: Li Guang Cc: Paolo Bonzini Cc: Peter Crosthwaite Cc: Peter Maydell Cc: Richard Henderson Cc: qemu-ppc@nongnu.org Cc: qemu-stable@nongnu.org Signed-off-by: Markus Armbruster --- 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 =3D DEVICE_CLASS(oc); =20 dc->realize =3D aw_a10_realize; + /* Reason: creates a CPU, thus use after free(), see cpu_class_init(= ) */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 static const TypeInfo aw_a10_type_info =3D { 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 *dat= a) DeviceClass *dc =3D DEVICE_CLASS(oc); =20 dc->realize =3D digic_realize; + /* Reason: creates a CPU, thus use after free(), see cpu_class_init(= ) */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 static const TypeInfo digic_type_info =3D { 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, voi= d *data) DeviceClass *dc =3D DEVICE_CLASS(oc); =20 dc->realize =3D fsl_imx25_realize; + /* Reason: creates a CPU, thus use after free(), see cpu_class_init(= ) */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 static const TypeInfo fsl_imx25_type_info =3D { 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, voi= d *data) DeviceClass *dc =3D DEVICE_CLASS(oc); =20 dc->realize =3D fsl_imx31_realize; + /* Reason: creates a CPU, thus use after free(), see cpu_class_init(= ) */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 static const TypeInfo fsl_imx31_type_info =3D { 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, v= oid *data) =20 dc->props =3D xlnx_zynqmp_props; dc->realize =3D xlnx_zynqmp_realize; + /* Reason: creates a CPU, thus use after free(), see cpu_class_init(= ) */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 static const TypeInfo xlnx_zynqmp_type_info =3D { 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, vo= id *data) dc->reset =3D pci_vpb_reset; dc->vmsd =3D &pci_vpb_vmstate; dc->props =3D pci_vpb_properties; + /* Reason: object_unref() hangs */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 static const TypeInfo pci_vpb_info =3D { @@ -521,10 +523,19 @@ static void pci_realview_init(Object *obj) s->mem_win_size[2] =3D 0x08000000; } =20 +static void pci_realview_class_init(ObjectClass *class, void *data) +{ + DeviceClass *dc =3D DEVICE_CLASS(class); + + /* Reason: object_unref() hangs */ + dc->cannot_even_create_with_object_new_yet =3D true; +} + static const TypeInfo pci_realview_info =3D { .name =3D "realview_pci", .parent =3D TYPE_VERSATILE_PCI, .instance_init =3D pci_realview_init, + .class_init =3D pci_realview_class_init, }; =20 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_ir= q irq, qemu_irq cd_irq) s->cd_irq =3D cd_irq; } =20 +static void pxa2xx_pcmcia_class_init(ObjectClass *class, void *data) +{ + DeviceClass *dc =3D DEVICE_CLASS(class); + + /* Reason: object_unref() crashes */ + dc->cannot_even_create_with_object_new_yet =3D true; +} + static const TypeInfo pxa2xx_pcmcia_type_info =3D { .name =3D TYPE_PXA2XX_PCMCIA, .parent =3D TYPE_SYS_BUS_DEVICE, .instance_size =3D sizeof(PXA2xxPCMCIAState), .instance_init =3D pxa2xx_pcmcia_initfn, + .class_init =3D pxa2xx_pcmcia_class_init, }; =20 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 *kl= ass, void *data) set_bit(DEVICE_CATEGORY_MISC, dc->categories); k->command_handler =3D command_handler; k->event_pending =3D event_pending; + + /* Reason: object_unref() hangs */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 static const TypeInfo sclp_event_facility_info =3D { 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 *da= ta) sc->read_cpu_info =3D sclp_read_cpu_info; sc->execute =3D sclp_execute; sc->service_interrupt =3D service_interrupt; + + /* Reason: object_unref() hangs */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 static TypeInfo sclp_info =3D { 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; =20 /* 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(c= onst char *typename, return NULL; } =20 + if (DEVICE_CLASS(klass)->cannot_even_create_with_object_new_yet) { + error_setg(errp, "Can't list properties of device '%s'", typenam= e); + return NULL; + } + obj =3D object_new(typename); =20 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 =3D true; + /* Reason: use after free: cpu_exec_init() saves CPUState in cpus */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 static const TypeInfo cpu_type_info =3D { 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) */ =20 dc->props =3D host_x86_cpu_properties; + /* Reason: host_x86_cpu_initfn() dies when !kvm_enabled() */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 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) =20 static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) { + DeviceClass *dc =3D DEVICE_CLASS(oc); PowerPCCPUClass *pcc =3D POWERPC_CPU_CLASS(oc); uint32_t vmx =3D kvmppc_get_vmx(); uint32_t dfp =3D kvmppc_get_dfp(); @@ -2214,6 +2215,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass = *oc, void *data) if (icache_size !=3D -1) { pcc->l1_icache_size =3D icache_size; } + + /* Reason: kvmppc_host_cpu_initfn() dies when !kvm_enabled() */ + dc->cannot_even_create_with_object_new_yet =3D true; } =20 bool kvmppc_has_cap_epr(void) diff --git a/tests/device-introspect-test.c b/tests/device-introspect-tes= t.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(); } =20 -static bool blacklisted(const char *type) -{ - static const char *blacklist[] =3D { - /* crash in object_unref(): */ - "pxa2xx-pcmcia", - /* hang in object_unref(): */ - "realview_pci", "versatile_pci", "s390-sclp-event-facility", "sc= lp", - /* create a CPU, thus use after free (see below): */ - "allwinner-a10", "digic", "fsl,imx25", "fsl,imx31", "xlnx,zynqmp= ", - }; - size_t len =3D strlen(type); - int i; - - if (len >=3D 4 && !strcmp(type + len - 4, "-cpu")) { - /* use after free: cpu_exec_init() saves CPUState in cpus */ - return true; - } - - for (i =3D 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 =3D qdict_get_try_str(qobject_to_qdict(qlist_entry_obj(entr= y)), "name"); g_assert(type); - if (blacklisted(type)) { - continue; /* FIXME broken device, skip */ - } test_one_device(type); } =20 --=20 2.4.3