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

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> On 24/09/15 20:57, Markus Armbruster wrote:
>> 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 during init (didn't debug, so I can't say why): "spapr-rng"
>> 
>> * Crash or hang during cleanup (didn't debug, so I can't say why):
>>   "pxa2xx-pcmcia", "realview_pci", "versatile_pci",
>>   "s390-sclp-event-facility", "sclp"
>> 
>> * Dangling pointers: 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 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-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" <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>
>> ---
> ...
>>  static void pxa2xx_pcmcia_register_types(void)
>> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
>> index ed43d5e..e1b115d 100644
>> --- a/hw/ppc/spapr_rng.c
>> +++ b/hw/ppc/spapr_rng.c
>> @@ -169,6 +169,11 @@ static void spapr_rng_class_init(ObjectClass *oc, void 
>> *data)
>>      dc->realize = spapr_rng_realize;
>>      set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>      dc->props = spapr_rng_properties;
>> +
>> +    /*
>> +     * Reason: crashes device-introspect-test for unknown reason.
>> +     */
>> +    dc->cannot_even_create_with_object_new_yet = true;
>>  }
>
> Please don't do that! That breaks the help output from
> "-device spapr-rng,?" which should help the user to see how to use this
> device!

Well, device-introspection-test makes qemu crash, with the backtrace
pointing squarely to this device.  Stands to reason that device
introspection could crash in normal usage, too.  Until the crash is
debugged, we better disable introspection of this device.

I quite agree that disabling introspection hurts users.  Just not as
much as crashes :)

> I tried to debug why this device breaks the test, but the test
> environment is giving me a hard time ... how do you best hook a gdb into
> that framework, so you can trace such problems?
> Anyway, with some trial and error, I found out that it seems like the
>
>   object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)
>
> in spapr_rng_instance_init() is causing the problems. Could it be that
> object_resolve_path_type is not working with the test environment?

I tried to figure out why this device breaks under this test, but
couldn't, so I posted with the "for unknown reason" comment.