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

[David Hildenbrand]

> No knowledge should be required for object_new(). Classes' instance_init
> functions should have no side-effects outside the object itself.

While this should theoretically be true, I can guarantee to you that this is
not the case for all devices :) (especially as there are too many unwritten
laws related to that whole qmp/qom thing flying around).

E.g. we can save ourselves from the creation of other devices (init + realize)
by setting a device to !hotpluggable in the device class. This code simply
bypasses such checks and assumes that QEMUs internal structure can deal with
that. We saw that this doesn't work this far.

Other interfaces (object_add) seem to rely on TYPE_USER_CREATABLE, so we can't
trigger "everything" from outside QEMU.

One can argue that we simply have to fix the devices - nevertheless I think this
is the wrong approach to the problem.

> 
> > 
> > Feels a little like hacking an interface to a java program, which allows to
> > create any object of a special kind dynamically (constructor arguments?),
> > reading some member variable (names) via reflections and then throwing that
> > object away. Which sounds very wrong to me.
> 
> I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
> and prototype-based approaches to inheritance and property
> introspection.
> 
> > 
> > Wonder if it wouldn't make more sense to query only the static properties 
> > of a
> > device. I mean if the dynamic properties are dynamic by definition (and can
> > change during runtime). So looking up the static properties via the typename
> > feels more KIS-style to me. Of course, the relevant properties would have 
> > to be
> > defined statically then, which shouldn't be a problem.
> 
> It depends on your definition of "shouldn't be a problem". :)

Well, it might require some internal rework of that property thingy, hehe ;)

> 
> The static property system is more limited than the QOM dynamic property
> system, today. We already depend on introspection of dynamic properties
> registered by instance_init functions, so we would need to move
> everything to a better static property system if we want to stop doing
> object_new() during class introspection without regressions.
> 
> > 
> > Dynamic properties that are actually created could depend on almost
> > everything in the system - why fake something that we don't know.
> 
> Properties registered on instance_init shouldn't depend on anything else
> on the system. Properties registered later in the object lifetime (e.g.
> on realize) can.
> 

Okay, so if the properties in instance_init should depend on nothing else in
the system, they are always the same for one QEMU version + device.

So maybe it would make sense to define all these "fixed properties" on a device
class level (e.g. via dc->props) and fill them with life afterwards (if we can't
completely specify them at that point). Of course this would need some
rework/careful thinking (e.g. concept of uninitialized properties).

Looking at the errors we get with that approach tells me that it will easily
break again in the future (we need tests for all devices - do we already have
it?) and makes me wonder if a definition on a device class level wouldn't be the
right thing to do - a clean interface description that is valid for each device
instance.

David

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

[Markus Armbruster]

Eduardo Habkost  writes:

> On Mon, Sep 21, 2015 at 10:30:50AM +0200, David Hildenbrand wrote:
>> > 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 `((>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?
>> > 
>> 
>> The problem was that the quiesce and cpu hotplug sclp event devices had no
>> parent (onoly a parent bus). So when the bus (inside the event facility) was
>> removed, it looped forever trying remove/unparent it's "bus childs" (which 
>> had
>> no parent).
>> 
>> sclp (parent=machine)
>> -> even-facility (parent=sclp)
>> -> bus (parent=event-facility)
>>   -> quiesce (parent=null)
>>   -> cpu hotplug (parent=null)
>> 
>> Maybe that helps others struggling with the same symptoms.
>> 
>> 
>> Just a quick comment on the introspection:
>> 
>> I don't think it is a good idea to expose properties that way. Temporarily
>> creating devices for the sake of querying property names sounds very wrong to
>> me, because certain devices require certain knowledge about how and when they
>> can be created.

I sympathize.  However, QOM is what it is.  Its design assumption
include:

  Properties are dynamically added.  To introspect them, you need to
  create the object.

and:

> No knowledge should be required for object_new(). Classes' instance_init
> functions should have no side-effects outside the object itself.

I'd tighten this to "object_unref(object_new(...)) works and has no
visible side effect".

See also review of [PATCH RFC 1/7] qom: allow properties to be
registered against classes
Message-ID: <55e72154.7070...@suse.de>
http://lists.gnu.org/archive/html/qemu-devel/2015-09/msg00517.html

>> Feels a little like hacking an interface to a java program, which allows to
>> create any object of a special kind dynamically (constructor arguments?),
>> reading some member variable (names) via reflections and then throwing that
>> object away. Which sounds very wrong to me.
>
> I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
> and prototype-based approaches to inheritance and property
> introspection.

I'm no friend of this aspect of QOM.  But QOM experts assure us it is
necessary (see thread quoted above).

>> 
>> Wonder if it wouldn't make more sense to query only the static properties of 
>> a
>> device. I mean if the dynamic properties are dynamic by definition (and can
>> change during runtime). So looking up the static properties via the typename
>> feels more KIS-style to me. Of course, the relevant properties would
>> have to be
>> defined statically then, which shouldn't be a problem.
>
> It depends on your definition of "shouldn't be a problem". :)
>
> The static property system is more limited than the QOM dynamic property
> system, today. We already depend on introspection of dynamic properties
> registered by instance_init functions, so we would need to move
> everything to a better static property system if we want to stop doing
> object_new() during class introspection without regressions.

If Dan's patch "qom: allow properties to be registered against classes"
goes in, we can certainly consider the idea to limit
device-list-properties to properties registered against classes.

Even then, the assumption "object_unref(object_new(...)) works and has

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

[Markus Armbruster]

Eduardo Habkost  writes:

> On Fri, Sep 18, 2015 at 02:00:38PM +0200, 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 `((>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
>
> That's isn't true for all CPU classes, only the ones that (incorrectly)
> call cpu_exec_init() on instance_init instead of realize. I believe at
> least TYPE_POWERPC_CPU is safe already.

Okay, I'll try to mark only the ones that actually screw up.

Thanks!

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

[Markus Armbruster]

Eric Blake  writes:

> On 09/18/2015 06:00 AM, 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.
>> 
>
>> 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:
>
> (intentionally) Ugly because it is a workaround, but then again, giving
> the attribute an ugly name will help call attention to the specific
> device owners to fix the mess.  I can live with that.

Named in Anthony's memory (see commit efec3dd) ;-P

>> 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 `((>subregions)->tqh_first == ((void *)0))' failed.
>> 
>> After:
>> 
>> Can't list properties of device 'pxa2xx-pcmcia'
>
> Not ideal, but much better than a crash, so it gets my vote for
> inclusion as an incremental improvement.
>
>
>> +++ 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;
>
> And a sufficiently verbose explanation for why the code is so ugly.
>
>> @@ -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 */
>> -}
>
> The devices are still broken, but the testsuite no longer flags them as
> broken because it no longer crashes or hangs, and you intentionally
> wrote the test to treat any output (including a graceful error message)
> as successful use of the probe.  The ugly attribute is now our only
> documentation of the problems, but that is still something sufficient to
> hopefully get it fixed.
>
> Reviewed-by: Eric Blake 

Thanks!

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

[Markus Armbruster]

Eduardo Habkost  writes:

> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>> On 18/09/15 14:00, 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 `((>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"
>> 
>> I just had a look at the powerpc code - you're likely talking about
>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>> target-ppc/kvm.c ? That should be fine, I think, because
>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>> enabled.

Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
and off, where C is the appropriate host CPU.

> It's true that currently the assert() will never trigger, but we will
> have to eventually move class registration to type_init if we want to
> make a generic query-cpu-definitions implementation work properly
> without depending on global machine accel configuration.

Good point.  These CPUs need a TODO marker.

> It won't hurt to set cannot_even_create_with_object_new_yet properly to
> reflect that the class isn't ready yet.

Marking cannot_even_create_with_object_new_yet() is the obvious TODO
marker.  It has an unnecessary side effect: we disable introspection for
these CPUs needlessly.  Tolerable?

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

[Eduardo Habkost]

On Mon, Sep 21, 2015 at 10:30:50AM +0200, David Hildenbrand wrote:
> > 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 `((>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?
> > 
> 
> The problem was that the quiesce and cpu hotplug sclp event devices had no
> parent (onoly a parent bus). So when the bus (inside the event facility) was
> removed, it looped forever trying remove/unparent it's "bus childs" (which had
> no parent).
> 
> sclp (parent=machine)
> -> even-facility (parent=sclp)
> -> bus (parent=event-facility)
>   -> quiesce (parent=null)
>   -> cpu hotplug (parent=null)
> 
> Maybe that helps others struggling with the same symptoms.
> 
> 
> Just a quick comment on the introspection:
> 
> I don't think it is a good idea to expose properties that way. Temporarily
> creating devices for the sake of querying property names sounds very wrong to
> me, because certain devices require certain knowledge about how and when they
> can be created.

No knowledge should be required for object_new(). Classes' instance_init
functions should have no side-effects outside the object itself.

> 
> Feels a little like hacking an interface to a java program, which allows to
> create any object of a special kind dynamically (constructor arguments?),
> reading some member variable (names) via reflections and then throwing that
> object away. Which sounds very wrong to me.

I wouldn't call it wrong. Confusing, maybe. We use a mix of class-based
and prototype-based approaches to inheritance and property
introspection.

> 
> Wonder if it wouldn't make more sense to query only the static properties of a
> device. I mean if the dynamic properties are dynamic by definition (and can
> change during runtime). So looking up the static properties via the typename
> feels more KIS-style to me. Of course, the relevant properties would have to 
> be
> defined statically then, which shouldn't be a problem.

It depends on your definition of "shouldn't be a problem". :)

The static property system is more limited than the QOM dynamic property
system, today. We already depend on introspection of dynamic properties
registered by instance_init functions, so we would need to move
everything to a better static property system if we want to stop doing
object_new() during class introspection without regressions.

> 
> Dynamic properties that are actually created could depend on almost
> everything in the system - why fake something that we don't know.

Properties registered on instance_init shouldn't depend on anything else
on the system. Properties registered later in the object lifetime (e.g.
on realize) can.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Sep 21, 2015 at 08:14:56AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> >> On 18/09/15 14:00, 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 `((>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"
> >> 
> >> I just had a look at the powerpc code - you're likely talking about
> >> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> >> target-ppc/kvm.c ? That should be fine, I think, because
> >> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> >> enabled.
> 
> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
> and off, where C is the appropriate host CPU.
> 
> > It's true that currently the assert() will never trigger, but we will
> > have to eventually move class registration to type_init if we want to
> > make a generic query-cpu-definitions implementation work properly
> > without depending on global machine accel configuration.
> 
> Good point.  These CPUs need a TODO marker.
> 
> > It won't hurt to set cannot_even_create_with_object_new_yet properly to
> > reflect that the class isn't ready yet.
> 
> Marking cannot_even_create_with_object_new_yet() is the obvious TODO
> marker.  It has an unnecessary side effect: we disable introspection for
> these CPUs needlessly.  Tolerable?

To me, it is. We already disable introspection of these CPUs needlessly
in all cases except when KVM is explicitly enabled in the command-line.

-- 
Eduardo

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

[Thomas Huth]

On 21/09/15 08:14, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
>>> On 18/09/15 14:00, 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 `((>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"
>>>
>>> I just had a look at the powerpc code - you're likely talking about
>>> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
>>> target-ppc/kvm.c ? That should be fine, I think, because
>>> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
>>> enabled.
> 
> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
> and off, where C is the appropriate host CPU.

In both cases (KVM on and off), I simply get:

# qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
'host-powerpc64-cpu' is not a valid device model name
# qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
'host-powerpc64-cpu' is not a valid device model name

No crash/abort here.

 Thomas

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

[Thomas Huth]

On 21/09/15 18:39, Markus Armbruster wrote:
> Thomas Huth  writes:
> 
>> On 21/09/15 08:14, Markus Armbruster wrote:
>>> Eduardo Habkost  writes:
>>>
 On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> On 18/09/15 14:00, 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 `((>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"
>
> I just had a look at the powerpc code - you're likely talking about
> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> target-ppc/kvm.c ? That should be fine, I think, because
> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> enabled.
>>>
>>> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
>>> and off, where C is the appropriate host CPU.
>>
>> In both cases (KVM on and off), I simply get:
>>
>> # qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
>> 'host-powerpc64-cpu' is not a valid device model name
>> # qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
>> 'host-powerpc64-cpu' is not a valid device model name
>>
>> No crash/abort here.
> 
> No introspection, either.  Can you try the same with QMP command
> device-list-properties?

# ppc64-softmmu/qemu-system-ppc64 -nodefaults -S -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": 
""}, "capabilities": []}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "device-list-properties", "arguments": { "typename": 
"host-powerpc64-cpu" } }
{"error": {"class": "DeviceNotFound", "desc": "Device 'host-powerpc64-cpu' not 
found"}}
{ "execute": "quit" }
{"return": {}}
{"timestamp": {"seconds": 1442856053, "microseconds": 732079}, "event": 
"SHUTDOWN"}

# ppc64-softmmu/qemu-system-ppc64 -machine accel=kvm -nodefaults -S -display 
none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 4, "major": 2}, "package": 
""}, "capabilities": []}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{ "execute": "device-list-properties", "arguments": { "typename": 
"host-powerpc64-cpu" } }
{"return": [{"name": "compat", "description": "compatibility mode, 
power6/power7/power8", "type": "str"}]}
{ "execute": "quit" }
{"return": {}}
{"timestamp": {"seconds": 1442856113, "microseconds": 913588}, "event": 
"SHUTDOWN"}

 Thomas

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

[Markus Armbruster]

Thomas Huth  writes:

> On 21/09/15 08:14, Markus Armbruster wrote:
>> Eduardo Habkost  writes:
>> 
>>> On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
 On 18/09/15 14:00, 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 `((>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"

 I just had a look at the powerpc code - you're likely talking about
 the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
 target-ppc/kvm.c ? That should be fine, I think, because
 kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
 enabled.
>> 
>> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
>> and off, where C is the appropriate host CPU.
>
> In both cases (KVM on and off), I simply get:
>
> # qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
> 'host-powerpc64-cpu' is not a valid device model name
> # qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
> 'host-powerpc64-cpu' is not a valid device model name
>
> No crash/abort here.

No introspection, either.  Can you try the same with QMP command
device-list-properties?

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

[Eduardo Habkost]

On Mon, Sep 21, 2015 at 08:09:48AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > On Fri, Sep 18, 2015 at 02:00:38PM +0200, 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 `((>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
> >
> > That's isn't true for all CPU classes, only the ones that (incorrectly)
> > call cpu_exec_init() on instance_init instead of realize. I believe at
> > least TYPE_POWERPC_CPU is safe already.
> 
> Okay, I'll try to mark only the ones that actually screw up.

Most of them screw up, today. If you prefer to simply set it to true on
TYPE_CPU and then explicitly override it to false only on the few
subclasses that are already fixed, I think it would be OK.

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Sep 21, 2015 at 07:22:51PM +0200, Thomas Huth wrote:
> On 21/09/15 18:39, Markus Armbruster wrote:
> > Thomas Huth  writes:
> >> On 21/09/15 08:14, Markus Armbruster wrote:
> >>> Eduardo Habkost  writes:
>  On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> > On 18/09/15 14:00, Markus Armbruster wrote:
[...]
> >> * Assert kvm_enabled(): "host-x86_64-cpu", host-i386-cpu",
> >>   "host-powerpc64-cpu", "host-embedded-powerpc-cpu",
> >>   "host-powerpc-cpu"
> >
> > I just had a look at the powerpc code - you're likely talking about
> > the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> > target-ppc/kvm.c ? That should be fine, I think, because
> > kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> > enabled.
> >>>
> >>> Easy to verify on a KVM-capable PPC host: try -device C,help with KVM on
> >>> and off, where C is the appropriate host CPU.
> >>
> >> In both cases (KVM on and off), I simply get:
> >>
> >> # qemu-system-ppc64 -machine accel=tcg -device host-powerpc64-cpu,help
> >> 'host-powerpc64-cpu' is not a valid device model name
> >> # qemu-system-ppc64 -machine accel=kvm -device host-powerpc64-cpu,help
> >> 'host-powerpc64-cpu' is not a valid device model name
> >>
> >> No crash/abort here.
> > 
> > No introspection, either.  Can you try the same with QMP command
> > device-list-properties?
> 
> # ppc64-softmmu/qemu-system-ppc64 -nodefaults -S -display none -qmp stdio
[...]
> { "execute": "device-list-properties", "arguments": { "typename": 
> "host-powerpc64-cpu" } }
> {"error": {"class": "DeviceNotFound", "desc": "Device 'host-powerpc64-cpu' 
> not found"}}
> 
> # ppc64-softmmu/qemu-system-ppc64 -machine accel=kvm -nodefaults -S -display 
> none -qmp stdio
[...]
> { "execute": "device-list-properties", "arguments": { "typename": 
> "host-powerpc64-cpu" } }
> {"return": [{"name": "compat", "description": "compatibility mode, 
> power6/power7/power8", "type": "str"}]}

So, introspection is already broken, but inconsistently broken (because
it depends on machine arguments). Setting
cannot_even_create_with_object_new_yet will make it more consistently
broken.

-- 
Eduardo

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

[David Hildenbrand]

> 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 `((>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?
> 

The problem was that the quiesce and cpu hotplug sclp event devices had no
parent (onoly a parent bus). So when the bus (inside the event facility) was
removed, it looped forever trying remove/unparent it's "bus childs" (which had
no parent).

sclp (parent=machine)
-> even-facility (parent=sclp)
-> bus (parent=event-facility)
  -> quiesce (parent=null)
  -> cpu hotplug (parent=null)

Maybe that helps others struggling with the same symptoms.


Just a quick comment on the introspection:

I don't think it is a good idea to expose properties that way. Temporarily
creating devices for the sake of querying property names sounds very wrong to
me, because certain devices require certain knowledge about how and when they
can be created.

Feels a little like hacking an interface to a java program, which allows to
create any object of a special kind dynamically (constructor arguments?),
reading some member variable (names) via reflections and then throwing that
object away. Which sounds very wrong to me.

Wonder if it wouldn't make more sense to query only the static properties of a
device. I mean if the dynamic properties are dynamic by definition (and can
change during runtime). So looking up the static properties via the typename
feels more KIS-style to me. Of course, the relevant properties would have to be
defined statically then, which shouldn't be a problem.

Dynamic properties that are actually created could depend on almost
everything in the system - why fake something that we don't know.

David

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

[Eduardo Habkost]

On Fri, Sep 18, 2015 at 08:42:54PM +0200, Thomas Huth wrote:
> On 18/09/15 14:00, 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 `((>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"
> 
> I just had a look at the powerpc code - you're likely talking about
> the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
> target-ppc/kvm.c ? That should be fine, I think, because
> kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
> enabled.

It's true that currently the assert() will never trigger, but we will
have to eventually move class registration to type_init if we want to
make a generic query-cpu-definitions implementation work properly
without depending on global machine accel configuration.

It won't hurt to set cannot_even_create_with_object_new_yet properly to
reflect that the class isn't ready yet.

-- 
Eduardo

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

[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 `((>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 
`((>subregions)->tqh_first == ((void *)0))' failed.

After:

Can't list properties of device 'pxa2xx-pcmcia'

Cc: "Andreas Färber" 
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-...@nongnu.org
Cc: qemu-sta...@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 = 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;
 }
 
 

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 `((>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 
> `((>subregions)->tqh_first == ((void *)0))' failed.
> 
> After:
> 
> Can't list properties of device 'pxa2xx-pcmcia'
> 
> Cc: "Andreas Färber" 
> 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-...@nongnu.org
> Cc: qemu-sta...@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 = 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() */
> +

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

[Eduardo Habkost]

On Fri, Sep 18, 2015 at 02:00:38PM +0200, 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 `((>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

That's isn't true for all CPU classes, only the ones that (incorrectly)
call cpu_exec_init() on instance_init instead of realize. I believe at
least TYPE_POWERPC_CPU is safe already.

-- 
Eduardo

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

[Eric Blake]

On 09/18/2015 06:00 AM, 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.
> 

> 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:

(intentionally) Ugly because it is a workaround, but then again, giving
the attribute an ugly name will help call attention to the specific
device owners to fix the mess.  I can live with that.

> 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 
> `((>subregions)->tqh_first == ((void *)0))' failed.
> 
> After:
> 
> Can't list properties of device 'pxa2xx-pcmcia'

Not ideal, but much better than a crash, so it gets my vote for
inclusion as an incremental improvement.


> +++ 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;

And a sufficiently verbose explanation for why the code is so ugly.

> @@ -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 */
> -}

The devices are still broken, but the testsuite no longer flags them as
broken because it no longer crashes or hangs, and you intentionally
wrote the test to treat any output (including a graceful error message)
as successful use of the probe.  The ugly attribute is now our only
documentation of the problems, but that is still something sufficient to
hopefully get it fixed.

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

[Thomas Huth]

On 18/09/15 14:00, 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 `((>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"

I just had a look at the powerpc code - you're likely talking about
the "assert(kvm_enabled());" in the kvmppc_host_cpu_initfn() in
target-ppc/kvm.c ? That should be fine, I think, because
kvm_ppc_register_host_cpu_type() is only done on ppc when KVM has been
enabled.

 Thomas