Re: [Qemu-devel] [PATCH 5/7] qdev: Protect device-list-properties against broken devices

[Christian Borntraeger]

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" <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);
>      }
>