[Qemu-devel] [PATCH v1 0/5] Fix crashes with introspection

[Qemu-devel] [PATCH v1 0/5] Fix crashes with introspection

[Thomas Huth]

As discovered recently, you can crash QEMU with a lot of devices
that do not get the reference counting of child objects right.
You just have to run 'device-list-properties' and call 'info qtree'
afterwards.
This patch series fixes a bunch of these problems in the ARM code.
I did not fix all problems yet, since it is quite time consuming
and I first want to get some feedback whether this is the right
way to go or not.

Thomas Huth (5):
  qom/object: Add a new function object_initialize_as_child()
  hw/core/sysbus: Add a function for creating and attaching an object
  hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported
    machines
  hw/arm/armv7: Fix crash when introspecting the "iotkit" device
  hw/cpu/a15mpcore: Fix introspection problem with the a15mpcore_priv
    device

 hw/arm/armv7m.c       |  9 ++++---
 hw/arm/bcm2836.c      | 19 +++++--------
 hw/arm/iotkit.c       | 74 ++++++++++++++++++++++-----------------------------
 hw/core/sysbus.c      |  8 ++++++
 hw/cpu/a15mpcore.c    |  8 +++---
 hw/intc/armv7m_nvic.c |  5 ++--
 include/hw/sysbus.h   |  3 +++
 include/qom/object.h  | 19 +++++++++++++
 qom/object.c          | 14 ++++++++++
 9 files changed, 93 insertions(+), 66 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Thomas Huth]

A lot of code is using the object_initialize() function followed by a call to
object_property_add_child() to add the newly initialized object as a child of
the current object. Both functions increase the reference counter of the new
object, but many spots that call these two functions then forget to drop one
of the superfluous references. So the newly created object is often not cleaned
up correctly when the parent is destroyed. In the worst case, this can cause
crashes, e.g. because device objects are not correctly removed from their
parent_bus.
Since this is a common pattern between many code spots, let's introdcue a
new function that takes care of calling all three required initialization
functions, first object_initialize(), then object_property_add_child() and
finally object_unref().

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/qom/object.h | 19 +++++++++++++++++++
 qom/object.c         | 14 ++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index f3d2308..c1b254c 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
 void object_initialize(void *obj, size_t size, const char *typename);
 
 /**
+ * object_initialize_as_child:
+ * @parentobj: The parent object to add a property to
+ * @propname: The name of the property
+ * @childobj: A pointer to the memory to be used for the object.
+ * @size: The maximum size available at @obj for the object.
+ * @type: The name of the type of the object to instantiate.
+ * @errp: If an error occurs, a pointer to an area to store the area
+ *
+ * This function will initialize an object. The memory for the object should
+ * have already been allocated. The object will then be added as child property
+ * to a parent with object_property_add_child() function. The returned object
+ * has a reference count of 1, and will be finalized when the last reference is
+ * dropped.
+ */
+void object_initialize_as_child(Object *parentobj, const char *propname,
+                                void *childobj, size_t size, const char *type,
+                                Error **errp);
+
+/**
  * object_dynamic_cast:
  * @obj: The object to cast.
  * @typename: The @typename to cast to.
diff --git a/qom/object.c b/qom/object.c
index 4609e34..de2ded0 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -392,6 +392,20 @@ void object_initialize(void *data, size_t size, const char *typename)
     object_initialize_with_type(data, size, type);
 }
 
+void object_initialize_as_child(Object *parentobj, const char *propname,
+                                void *childobj, size_t size, const char *type,
+                                Error **errp)
+{
+    object_initialize(childobj, size, type);
+    object_property_add_child(parentobj, propname, OBJECT(childobj), errp);
+    /*
+     * Since object_property_add_child added a reference to the child object,
+     * we can drop the initial reference from object_initialize now.
+     */
+    object_unref(OBJECT(childobj));
+}
+
+
 static inline bool object_property_is_child(ObjectProperty *prop)
 {
     return strstart(prop->type, "child<", NULL);
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/5] hw/core/sysbus: Add a function for creating and attaching an object

[Thomas Huth]

A lot of functions are initializing an object and attach it immediately
afterwards to the system bus. Provide a common function for this, which
also uses object_initialize_as_child() to make sure that the reference
counter is correctly initialized to 1 afterwards.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/core/sysbus.c    | 8 ++++++++
 include/hw/sysbus.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index ecfb0cf..c164318 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -376,6 +376,14 @@ BusState *sysbus_get_default(void)
     return main_system_bus;
 }
 
+void sysbus_init_child_obj(Object *parent, const char *childname, void *child,
+                           size_t childsize, const char *childtype)
+{
+    object_initialize_as_child(parent, childname, child, childsize, childtype,
+                               &error_abort);
+    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
+}
+
 static void sysbus_register_types(void)
 {
     type_register_static(&system_bus_info);
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index e88bb6d..e405232 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -96,6 +96,9 @@ void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
                    MemoryRegion *mem);
 MemoryRegion *sysbus_address_space(SysBusDevice *dev);
 
+void sysbus_init_child_obj(Object *parent, const char *childname, void *child,
+                           size_t childsize, const char *childtype);
+
 /* Call func for every dynamically created sysbus device in the system */
 void foreach_dynamic_sysbus_device(FindSysbusDeviceFunc *func, void *opaque);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/5] hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported machines

[Thomas Huth]

When trying to "device_add bcm2837" on a machine that is not suitable for
this device, you can quickly crash QEMU afterwards, e.g. with "info qtree":

echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
 "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp stdio

{"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{"return": {}}
{"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
 hotplugged on this machine"}}
Segmentation fault (core dumped)

The qdev_set_parent_bus() from instance_init adds a link to the child devices
which is not valid anymore after the bcm2837 instance has been destroyed.
Unfortunately, the child devices do not get destroyed / unlinked correctly
because both object_initialize() and object_property_add_child() increase
the reference count of the child objects by one, but only one reference
is dropped when the parent gets removed. So let's use the new functions
object_initialize_as_child() and sysbus_init_child_obj() instead to create
the objects, which will take care of creating the child objects with the
correct reference count of one.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/bcm2836.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 6805a7d..b62adeb 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -51,25 +51,20 @@ static void bcm2836_init(Object *obj)
     int n;
 
     for (n = 0; n < BCM283X_NCPUS; n++) {
-        object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
-                          info->cpu_type);
-        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
-                                  &error_abort);
+        object_initialize_as_child(obj, "cpu[*]", &s->cpus[n],
+                                   sizeof(s->cpus[n]), info->cpu_type,
+                                   &error_abort);
     }
 
-    object_initialize(&s->control, sizeof(s->control), TYPE_BCM2836_CONTROL);
-    object_property_add_child(obj, "control", OBJECT(&s->control), NULL);
-    qdev_set_parent_bus(DEVICE(&s->control), sysbus_get_default());
+    sysbus_init_child_obj(obj, "control", &s->control, sizeof(s->control),
+                          TYPE_BCM2836_CONTROL);
 
-    object_initialize(&s->peripherals, sizeof(s->peripherals),
-                      TYPE_BCM2835_PERIPHERALS);
-    object_property_add_child(obj, "peripherals", OBJECT(&s->peripherals),
-                              &error_abort);
+    sysbus_init_child_obj(obj, "peripherals", &s->peripherals,
+                          sizeof(s->peripherals), TYPE_BCM2835_PERIPHERALS);
     object_property_add_alias(obj, "board-rev", OBJECT(&s->peripherals),
                               "board-rev", &error_abort);
     object_property_add_alias(obj, "vcram-size", OBJECT(&s->peripherals),
                               "vcram-size", &error_abort);
-    qdev_set_parent_bus(DEVICE(&s->peripherals), sysbus_get_default());
 }
 
 static void bcm2836_realize(DeviceState *dev, Error **errp)
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/5] hw/arm/armv7: Fix crash when introspecting the "iotkit" device

[Thomas Huth]

QEMU currently crashes when introspecting the "iotkit" device and
runnint "info qtree" afterwards, e.g. when running QEMU like this:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'iotkit'}}" "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}" | \
 aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio

Use the new functions object_initialize_as_child() and sysbus_init_child_obj()
to make sure that all objects get cleaned up correctly when the instances
are destroyed.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/armv7m.c       |  9 ++++---
 hw/arm/iotkit.c       | 74 ++++++++++++++++++++++-----------------------------
 hw/intc/armv7m_nvic.c |  5 ++--
 3 files changed, 39 insertions(+), 49 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 9e00d40..7164e04 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -134,14 +134,15 @@ static void armv7m_instance_init(Object *obj)
 
     memory_region_init(&s->container, obj, "armv7m-container", UINT64_MAX);
 
-    object_initialize(&s->nvic, sizeof(s->nvic), TYPE_NVIC);
-    qdev_set_parent_bus(DEVICE(&s->nvic), sysbus_get_default());
+    sysbus_init_child_obj(obj, "nvnic", &s->nvic, sizeof(s->nvic), TYPE_NVIC);
     object_property_add_alias(obj, "num-irq",
                               OBJECT(&s->nvic), "num-irq", &error_abort);
 
     for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
-        object_initialize(&s->bitband[i], sizeof(s->bitband[i]), TYPE_BITBAND);
-        qdev_set_parent_bus(DEVICE(&s->bitband[i]), sysbus_get_default());
+        char *name = g_strdup_printf("bitband-%d", i);
+        sysbus_init_child_obj(obj, name, &s->bitband[i], sizeof(s->bitband[i]),
+                              TYPE_BITBAND);
+        g_free(name);
     }
 }
 
diff --git a/hw/arm/iotkit.c b/hw/arm/iotkit.c
index 133d5bb..d479323 100644
--- a/hw/arm/iotkit.c
+++ b/hw/arm/iotkit.c
@@ -30,15 +30,6 @@ static void make_alias(IoTKit *s, MemoryRegion *mr, const char *name,
     memory_region_add_subregion_overlap(&s->container, base, mr, -1500);
 }
 
-static void init_sysbus_child(Object *parent, const char *childname,
-                              void *child, size_t childsize,
-                              const char *childtype)
-{
-    object_initialize(child, childsize, childtype);
-    object_property_add_child(parent, childname, OBJECT(child), &error_abort);
-    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());
-}
-
 static void irq_status_forwarder(void *opaque, int n, int level)
 {
     qemu_irq destirq = opaque;
@@ -119,53 +110,52 @@ static void iotkit_init(Object *obj)
 
     memory_region_init(&s->container, obj, "iotkit-container", UINT64_MAX);
 
-    init_sysbus_child(obj, "armv7m", &s->armv7m, sizeof(s->armv7m),
-                      TYPE_ARMV7M);
+    sysbus_init_child_obj(obj, "armv7m", &s->armv7m, sizeof(s->armv7m),
+                          TYPE_ARMV7M);
     qdev_prop_set_string(DEVICE(&s->armv7m), "cpu-type",
                          ARM_CPU_TYPE_NAME("cortex-m33"));
 
-    init_sysbus_child(obj, "secctl", &s->secctl, sizeof(s->secctl),
-                      TYPE_IOTKIT_SECCTL);
-    init_sysbus_child(obj, "apb-ppc0", &s->apb_ppc0, sizeof(s->apb_ppc0),
-                      TYPE_TZ_PPC);
-    init_sysbus_child(obj, "apb-ppc1", &s->apb_ppc1, sizeof(s->apb_ppc1),
-                      TYPE_TZ_PPC);
-    init_sysbus_child(obj, "mpc", &s->mpc, sizeof(s->mpc), TYPE_TZ_MPC);
-    object_initialize(&s->mpc_irq_orgate, sizeof(s->mpc_irq_orgate),
-                      TYPE_OR_IRQ);
-    object_property_add_child(obj, "mpc-irq-orgate",
-                              OBJECT(&s->mpc_irq_orgate), &error_abort);
+    sysbus_init_child_obj(obj, "secctl", &s->secctl, sizeof(s->secctl),
+                          TYPE_IOTKIT_SECCTL);
+    sysbus_init_child_obj(obj, "apb-ppc0", &s->apb_ppc0, sizeof(s->apb_ppc0),
+                          TYPE_TZ_PPC);
+    sysbus_init_child_obj(obj, "apb-ppc1", &s->apb_ppc1, sizeof(s->apb_ppc1),
+                          TYPE_TZ_PPC);
+    sysbus_init_child_obj(obj, "mpc", &s->mpc, sizeof(s->mpc), TYPE_TZ_MPC);
+    object_initialize_as_child(obj, "mpc-irq-orgate", &s->mpc_irq_orgate,
+                               sizeof(s->mpc_irq_orgate), TYPE_OR_IRQ,
+                               &error_abort);
+
     for (i = 0; i < ARRAY_SIZE(s->mpc_irq_splitter); i++) {
         char *name = g_strdup_printf("mpc-irq-splitter-%d", i);
         SplitIRQ *splitter = &s->mpc_irq_splitter[i];
 
-        object_initialize(splitter, sizeof(*splitter), TYPE_SPLIT_IRQ);
-        object_property_add_child(obj, name, OBJECT(splitter), &error_abort);
+        object_initialize_as_child(obj, name, splitter, sizeof(*splitter),
+                                   TYPE_SPLIT_IRQ, &error_abort);
         g_free(name);
     }
-    init_sysbus_child(obj, "timer0", &s->timer0, sizeof(s->timer0),
-                      TYPE_CMSDK_APB_TIMER);
-    init_sysbus_child(obj, "timer1", &s->timer1, sizeof(s->timer1),
-                      TYPE_CMSDK_APB_TIMER);
-    init_sysbus_child(obj, "dualtimer", &s->dualtimer, sizeof(s->dualtimer),
-                      TYPE_UNIMPLEMENTED_DEVICE);
-    object_initialize(&s->ppc_irq_orgate, sizeof(s->ppc_irq_orgate),
-                      TYPE_OR_IRQ);
-    object_property_add_child(obj, "ppc-irq-orgate",
-                              OBJECT(&s->ppc_irq_orgate), &error_abort);
-    object_initialize(&s->sec_resp_splitter, sizeof(s->sec_resp_splitter),
-                      TYPE_SPLIT_IRQ);
-    object_property_add_child(obj, "sec-resp-splitter",
-                              OBJECT(&s->sec_resp_splitter), &error_abort);
+    sysbus_init_child_obj(obj, "timer0", &s->timer0, sizeof(s->timer0),
+                          TYPE_CMSDK_APB_TIMER);
+    sysbus_init_child_obj(obj, "timer1", &s->timer1, sizeof(s->timer1),
+                          TYPE_CMSDK_APB_TIMER);
+    sysbus_init_child_obj(obj, "dualtimer", &s->dualtimer, sizeof(s->dualtimer),
+                          TYPE_UNIMPLEMENTED_DEVICE);
+    object_initialize_as_child(obj, "ppc-irq-orgate", &s->ppc_irq_orgate,
+                               sizeof(s->ppc_irq_orgate), TYPE_OR_IRQ,
+                               &error_abort);
+    object_initialize_as_child(obj, "sec-resp-splitter", &s->sec_resp_splitter,
+                               sizeof(s->sec_resp_splitter), TYPE_SPLIT_IRQ,
+                               &error_abort);
     for (i = 0; i < ARRAY_SIZE(s->ppc_irq_splitter); i++) {
         char *name = g_strdup_printf("ppc-irq-splitter-%d", i);
         SplitIRQ *splitter = &s->ppc_irq_splitter[i];
 
-        object_initialize(splitter, sizeof(*splitter), TYPE_SPLIT_IRQ);
-        object_property_add_child(obj, name, OBJECT(splitter), &error_abort);
+        object_initialize_as_child(obj, name, splitter, sizeof(*splitter),
+                                   TYPE_SPLIT_IRQ, &error_abort);
+        g_free(name);
     }
-    init_sysbus_child(obj, "s32ktimer", &s->s32ktimer, sizeof(s->s32ktimer),
-                      TYPE_UNIMPLEMENTED_DEVICE);
+    sysbus_init_child_obj(obj, "s32ktimer", &s->s32ktimer, sizeof(s->s32ktimer),
+                          TYPE_UNIMPLEMENTED_DEVICE);
 }
 
 static void iotkit_exp_irq(void *opaque, int n, int level)
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 661be88..7a5330f 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -2296,9 +2296,8 @@ static void armv7m_nvic_instance_init(Object *obj)
     NVICState *nvic = NVIC(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    object_initialize(&nvic->systick[M_REG_NS],
-                      sizeof(nvic->systick[M_REG_NS]), TYPE_SYSTICK);
-    qdev_set_parent_bus(DEVICE(&nvic->systick[M_REG_NS]), sysbus_get_default());
+    sysbus_init_child_obj(obj, "systick-reg-ns", &nvic->systick[M_REG_NS],
+                          sizeof(nvic->systick[M_REG_NS]), TYPE_SYSTICK);
     /* We can't initialize the secure systick here, as we don't know
      * yet if we need it.
      */
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/5] hw/cpu/a15mpcore: Fix introspection problem with the a15mpcore_priv device

[Thomas Huth]

There is a memory management problem when introspecting the a15mpcore_priv
device. It can be seen with valgrind when running QEMU like this:

echo "{'execute':'qmp_capabilities'} {'execute':'device-list-properties'," \
 "'arguments':{'typename':'a15mpcore_priv'}}"\
 "{'execute': 'human-monitor-command', " \
 "'arguments': {'command-line': 'info qtree'}}"  | \
 valgrind -q aarch64-softmmu/qemu-system-aarch64 -M none,accel=qtest -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
 "package": "build-all"}, "capabilities": []}}
{"return": {}}
{"return": [{"name": "num-cpu", "type": "uint32"}, {"name": "num-irq",
 "type": "uint32"}, {"name": "a15mp-priv-container[0]", "type":
  "child<qemu:memory-region>"}]}
==24978== Invalid read of size 8
==24978==    at 0x618EBA: qdev_print (qdev-monitor.c:686)
==24978==    by 0x618EBA: qbus_print (qdev-monitor.c:719)
[...]

Use the new sysbus_init_child_obj() function to make sure that we get
the reference counting of the child objects right.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/cpu/a15mpcore.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index bc05152..43c1079 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -35,15 +35,13 @@ static void a15mp_priv_initfn(Object *obj)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     A15MPPrivState *s = A15MPCORE_PRIV(obj);
-    DeviceState *gicdev;
 
     memory_region_init(&s->container, obj, "a15mp-priv-container", 0x8000);
     sysbus_init_mmio(sbd, &s->container);
 
-    object_initialize(&s->gic, sizeof(s->gic), gic_class_name());
-    gicdev = DEVICE(&s->gic);
-    qdev_set_parent_bus(gicdev, sysbus_get_default());
-    qdev_prop_set_uint32(gicdev, "revision", 2);
+    sysbus_init_child_obj(obj, "gic", &s->gic, sizeof(s->gic),
+                          gic_class_name());
+    qdev_prop_set_uint32(DEVICE(&s->gic), "revision", 2);
 }
 
 static void a15mp_priv_realize(DeviceState *dev, Error **errp)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v1 0/5] Fix crashes with introspection

[Paolo Bonzini]

On 12/07/2018 17:30, Thomas Huth wrote:
> As discovered recently, you can crash QEMU with a lot of devices
> that do not get the reference counting of child objects right.
> You just have to run 'device-list-properties' and call 'info qtree'
> afterwards.
> This patch series fixes a bunch of these problems in the ARM code.
> I did not fix all problems yet, since it is quite time consuming
> and I first want to get some feedback whether this is the right
> way to go or not.

Patches 1-3 look like the way to go to me.

(FWIW, this is indeed an instance of the bug that Eduardo envisioned:
you have a reference to the inner object, and it becomes invalid after
the outer object is freed.  However, in this case the reference is
unnecessary so this is the right fix).

Thanks,

Paolo

> Thomas Huth (5):
>   qom/object: Add a new function object_initialize_as_child()
>   hw/core/sysbus: Add a function for creating and attaching an object
>   hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported
>     machines
>   hw/arm/armv7: Fix crash when introspecting the "iotkit" device
>   hw/cpu/a15mpcore: Fix introspection problem with the a15mpcore_priv
>     device
> 
>  hw/arm/armv7m.c       |  9 ++++---
>  hw/arm/bcm2836.c      | 19 +++++--------
>  hw/arm/iotkit.c       | 74 ++++++++++++++++++++++-----------------------------
>  hw/core/sysbus.c      |  8 ++++++
>  hw/cpu/a15mpcore.c    |  8 +++---
>  hw/intc/armv7m_nvic.c |  5 ++--
>  include/hw/sysbus.h   |  3 +++
>  include/qom/object.h  | 19 +++++++++++++
>  qom/object.c          | 14 ++++++++++
>  9 files changed, 93 insertions(+), 66 deletions(-)
> 

Re: [Qemu-devel] [PATCH v1 0/5] Fix crashes with introspection

[Peter Maydell]

On 12 July 2018 at 16:30, Thomas Huth <thuth@redhat.com> wrote:
> As discovered recently, you can crash QEMU with a lot of devices
> that do not get the reference counting of child objects right.
> You just have to run 'device-list-properties' and call 'info qtree'
> afterwards.
> This patch series fixes a bunch of these problems in the ARM code.
> I did not fix all problems yet, since it is quite time consuming
> and I first want to get some feedback whether this is the right
> way to go or not.
>
> Thomas Huth (5):
>   qom/object: Add a new function object_initialize_as_child()
>   hw/core/sysbus: Add a function for creating and attaching an object
>   hw/arm/bcm2836: Fix crash with device_add bcm2837 on unsupported
>     machines
>   hw/arm/armv7: Fix crash when introspecting the "iotkit" device
>   hw/cpu/a15mpcore: Fix introspection problem with the a15mpcore_priv
>     device

Seems good to me. I'm cc'ing Igor since he had some opinions when
I sent a previous (and rather more ad-hoc) attempt at reducing
the boilerplate around creating and parenting sysbus objects:
http://lists.gnu.org/archive/html/qemu-devel/2018-02/msg04493.html

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Eduardo Habkost]

On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
> A lot of code is using the object_initialize() function followed by a call to
> object_property_add_child() to add the newly initialized object as a child of
> the current object. Both functions increase the reference counter of the new
> object, but many spots that call these two functions then forget to drop one
> of the superfluous references. So the newly created object is often not cleaned
> up correctly when the parent is destroyed. In the worst case, this can cause
> crashes, e.g. because device objects are not correctly removed from their
> parent_bus.
> Since this is a common pattern between many code spots, let's introdcue a
> new function that takes care of calling all three required initialization
> functions, first object_initialize(), then object_property_add_child() and
> finally object_unref().
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/qom/object.h | 19 +++++++++++++++++++
>  qom/object.c         | 14 ++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3d2308..c1b254c 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>  void object_initialize(void *obj, size_t size, const char *typename);
>  
>  /**
> + * object_initialize_as_child:
> + * @parentobj: The parent object to add a property to
> + * @propname: The name of the property
> + * @childobj: A pointer to the memory to be used for the object.
> + * @size: The maximum size available at @obj for the object.
> + * @type: The name of the type of the object to instantiate.
> + * @errp: If an error occurs, a pointer to an area to store the area
> + *
> + * This function will initialize an object. The memory for the object should
> + * have already been allocated. The object will then be added as child property
> + * to a parent with object_property_add_child() function. The returned object
> + * has a reference count of 1, and will be finalized when the last reference is
> + * dropped.
> + */
> +void object_initialize_as_child(Object *parentobj, const char *propname,
> +                                void *childobj, size_t size, const char *type,
> +                                Error **errp);

Why did you use void* instead of Object*?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Thomas Huth]

On 12.07.2018 18:52, Eduardo Habkost wrote:
> On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
>> A lot of code is using the object_initialize() function followed by a call to
>> object_property_add_child() to add the newly initialized object as a child of
>> the current object. Both functions increase the reference counter of the new
>> object, but many spots that call these two functions then forget to drop one
>> of the superfluous references. So the newly created object is often not cleaned
>> up correctly when the parent is destroyed. In the worst case, this can cause
>> crashes, e.g. because device objects are not correctly removed from their
>> parent_bus.
>> Since this is a common pattern between many code spots, let's introdcue a
>> new function that takes care of calling all three required initialization
>> functions, first object_initialize(), then object_property_add_child() and
>> finally object_unref().
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  include/qom/object.h | 19 +++++++++++++++++++
>>  qom/object.c         | 14 ++++++++++++++
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index f3d2308..c1b254c 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>>  void object_initialize(void *obj, size_t size, const char *typename);
>>  
>>  /**
>> + * object_initialize_as_child:
>> + * @parentobj: The parent object to add a property to
>> + * @propname: The name of the property
>> + * @childobj: A pointer to the memory to be used for the object.
>> + * @size: The maximum size available at @obj for the object.
>> + * @type: The name of the type of the object to instantiate.
>> + * @errp: If an error occurs, a pointer to an area to store the area
>> + *
>> + * This function will initialize an object. The memory for the object should
>> + * have already been allocated. The object will then be added as child property
>> + * to a parent with object_property_add_child() function. The returned object
>> + * has a reference count of 1, and will be finalized when the last reference is
>> + * dropped.
>> + */
>> +void object_initialize_as_child(Object *parentobj, const char *propname,
>> +                                void *childobj, size_t size, const char *type,
>> +                                Error **errp);
> 
> Why did you use void* instead of Object*?

That's the same what object_initialize() is doing (see above). Otherwise
all the callers have to cast their pointers with OBJECT() first.

 Thomas

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Eduardo Habkost]

On Thu, Jul 12, 2018 at 06:55:20PM +0200, Thomas Huth wrote:
> On 12.07.2018 18:52, Eduardo Habkost wrote:
> > On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
> >> A lot of code is using the object_initialize() function followed by a call to
> >> object_property_add_child() to add the newly initialized object as a child of
> >> the current object. Both functions increase the reference counter of the new
> >> object, but many spots that call these two functions then forget to drop one
> >> of the superfluous references. So the newly created object is often not cleaned
> >> up correctly when the parent is destroyed. In the worst case, this can cause
> >> crashes, e.g. because device objects are not correctly removed from their
> >> parent_bus.
> >> Since this is a common pattern between many code spots, let's introdcue a
> >> new function that takes care of calling all three required initialization
> >> functions, first object_initialize(), then object_property_add_child() and
> >> finally object_unref().
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  include/qom/object.h | 19 +++++++++++++++++++
> >>  qom/object.c         | 14 ++++++++++++++
> >>  2 files changed, 33 insertions(+)
> >>
> >> diff --git a/include/qom/object.h b/include/qom/object.h
> >> index f3d2308..c1b254c 100644
> >> --- a/include/qom/object.h
> >> +++ b/include/qom/object.h
> >> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
> >>  void object_initialize(void *obj, size_t size, const char *typename);
> >>  
> >>  /**
> >> + * object_initialize_as_child:
> >> + * @parentobj: The parent object to add a property to
> >> + * @propname: The name of the property
> >> + * @childobj: A pointer to the memory to be used for the object.
> >> + * @size: The maximum size available at @obj for the object.
> >> + * @type: The name of the type of the object to instantiate.
> >> + * @errp: If an error occurs, a pointer to an area to store the area
> >> + *
> >> + * This function will initialize an object. The memory for the object should
> >> + * have already been allocated. The object will then be added as child property
> >> + * to a parent with object_property_add_child() function. The returned object
> >> + * has a reference count of 1, and will be finalized when the last reference is
> >> + * dropped.
> >> + */
> >> +void object_initialize_as_child(Object *parentobj, const char *propname,
> >> +                                void *childobj, size_t size, const char *type,
> >> +                                Error **errp);
> > 
> > Why did you use void* instead of Object*?
> 
> That's the same what object_initialize() is doing (see above). Otherwise
> all the callers have to cast their pointers with OBJECT() first.

Why wouldn't the same argument apply to every single function
that takes Object* as argument?  Why the OBJECT macro exists?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Peter Maydell]

On 12 July 2018 at 17:55, Thomas Huth <thuth@redhat.com> wrote:
> On 12.07.2018 18:52, Eduardo Habkost wrote:
>> On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
>>> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>>>  void object_initialize(void *obj, size_t size, const char *typename);
>>>
>>>  /**
>>> + * object_initialize_as_child:
>>> + * @parentobj: The parent object to add a property to
>>> + * @propname: The name of the property
>>> + * @childobj: A pointer to the memory to be used for the object.
>>> + * @size: The maximum size available at @obj for the object.
>>> + * @type: The name of the type of the object to instantiate.
>>> + * @errp: If an error occurs, a pointer to an area to store the area
>>> + *
>>> + * This function will initialize an object. The memory for the object should
>>> + * have already been allocated. The object will then be added as child property
>>> + * to a parent with object_property_add_child() function. The returned object
>>> + * has a reference count of 1, and will be finalized when the last reference is
>>> + * dropped.
>>> + */
>>> +void object_initialize_as_child(Object *parentobj, const char *propname,
>>> +                                void *childobj, size_t size, const char *type,
>>> +                                Error **errp);
>>
>> Why did you use void* instead of Object*?
>
> That's the same what object_initialize() is doing (see above). Otherwise
> all the callers have to cast their pointers with OBJECT() first.

Casting with OBJECT() would not work, because it does a typecheck.
At the point where something is calling object_initialize() or
object_initialize_with_child(), the memory at 'childobj' has not
been initialized, so a typecheck is impossible.

It's only once the object has actually been initialized that
the memory is a valid Object* that you can put through OBJECT().

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Eduardo Habkost]

On Thu, Jul 12, 2018 at 05:30:59PM +0200, Thomas Huth wrote:
> A lot of code is using the object_initialize() function followed by a call to
> object_property_add_child() to add the newly initialized object as a child of
> the current object. Both functions increase the reference counter of the new
> object, but many spots that call these two functions then forget to drop one
> of the superfluous references. So the newly created object is often not cleaned
> up correctly when the parent is destroyed. In the worst case, this can cause
> crashes, e.g. because device objects are not correctly removed from their
> parent_bus.
> Since this is a common pattern between many code spots, let's introdcue a
> new function that takes care of calling all three required initialization
> functions, first object_initialize(), then object_property_add_child() and
> finally object_unref().
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/qom/object.h | 19 +++++++++++++++++++
>  qom/object.c         | 14 ++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3d2308..c1b254c 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -749,6 +749,25 @@ int object_set_propv(Object *obj,
>  void object_initialize(void *obj, size_t size, const char *typename);
>  
>  /**
> + * object_initialize_as_child:
> + * @parentobj: The parent object to add a property to
> + * @propname: The name of the property
> + * @childobj: A pointer to the memory to be used for the object.
> + * @size: The maximum size available at @obj for the object.
> + * @type: The name of the type of the object to instantiate.
> + * @errp: If an error occurs, a pointer to an area to store the area

"to store the error"

> + *
> + * This function will initialize an object. The memory for the object should
> + * have already been allocated. The object will then be added as child property
> + * to a parent with object_property_add_child() function. The returned object
> + * has a reference count of 1, and will be finalized when the last reference is
> + * dropped.

I think we need to document clearly who owns the reference and is
responsible for dropping it.  In this case, we need to make it
clear that the child property will own the reference, and nobody
should drop it except object_finalize_child_property().  This is
a subtle but important difference from object_initialize().


> + */
> +void object_initialize_as_child(Object *parentobj, const char *propname,
> +                                void *childobj, size_t size, const char *type,
> +                                Error **errp);
> +
> +/**
>   * object_dynamic_cast:
>   * @obj: The object to cast.
>   * @typename: The @typename to cast to.
> diff --git a/qom/object.c b/qom/object.c
> index 4609e34..de2ded0 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -392,6 +392,20 @@ void object_initialize(void *data, size_t size, const char *typename)
>      object_initialize_with_type(data, size, type);
>  }
>  
> +void object_initialize_as_child(Object *parentobj, const char *propname,
> +                                void *childobj, size_t size, const char *type,
> +                                Error **errp)
> +{
> +    object_initialize(childobj, size, type);
> +    object_property_add_child(parentobj, propname, OBJECT(childobj), errp);
> +    /*
> +     * Since object_property_add_child added a reference to the child object,
> +     * we can drop the initial reference from object_initialize now.
> +     */
> +    object_unref(OBJECT(childobj));

I suggest "drop the reference added by object_initialize(), the
child property will own the only reference to the object".

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Paolo Bonzini]

On 12/07/2018 18:55, Thomas Huth wrote:
>>> +void object_initialize_as_child(Object *parentobj, const char *propname,
>>> +                                void *childobj, size_t size, const char *type,
>>> +                                Error **errp);
>> Why did you use void* instead of Object*?
> That's the same what object_initialize() is doing (see above). Otherwise
> all the callers have to cast their pointers with OBJECT() first.

Actually I had forgotten about object_new_with_props, which is very
similar to the new function above.  I think we should follow the model
and define the new function as

void object_initialize_with_props(Object *parent, const char *id,
                           void *childobj,
                           size_t size, const char *type,
                           Error **errp,
                           ...) QEMU_SENTINEL;
void object_initialize_with_propv(Object *parent, const char *id,
                           void *childobj,
                           size_t size, const char *type,
                           Error **errp,
                           va_list vargs);

I actually prefer Thomas's name (or maybe even
object_initialize_child/object_initialize_childv), but if we adopt it we
should also rename object_new_with_props and object_new_with_propv, for
consistency.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Paolo Bonzini]

On 12/07/2018 19:09, Peter Maydell wrote:
>> That's the same what object_initialize() is doing (see above). Otherwise
>> all the callers have to cast their pointers with OBJECT() first.
> Casting with OBJECT() would not work, because it does a typecheck.
> At the point where something is calling object_initialize() or
> object_initialize_with_child(), the memory at 'childobj' has not
> been initialized, so a typecheck is impossible.
> 
> It's only once the object has actually been initialized that
> the memory is a valid Object* that you can put through OBJECT().

OBJECT is special and just does a cast, because every pointer to a QOM
object would pass.  However, your reasoning still applies: you should
not put through OBJECT() an object that hasn't been initialized yet,
just like you should not put it through DEVICE() or any of the macros
that _do_ perform the typecheck.

Paolo

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Thomas Huth]

On 13.07.2018 08:26, Paolo Bonzini wrote:
> On 12/07/2018 18:55, Thomas Huth wrote:
>>>> +void object_initialize_as_child(Object *parentobj, const char *propname,
>>>> +                                void *childobj, size_t size, const char *type,
>>>> +                                Error **errp);
>>> Why did you use void* instead of Object*?
>> That's the same what object_initialize() is doing (see above). Otherwise
>> all the callers have to cast their pointers with OBJECT() first.
> 
> Actually I had forgotten about object_new_with_props, which is very
> similar to the new function above.  I think we should follow the model
> and define the new function as
> 
> void object_initialize_with_props(Object *parent, const char *id,
>                            void *childobj,
>                            size_t size, const char *type,
>                            Error **errp,
>                            ...) QEMU_SENTINEL;
> void object_initialize_with_propv(Object *parent, const char *id,
>                            void *childobj,
>                            size_t size, const char *type,
>                            Error **errp,
>                            va_list vargs);
> 
> I actually prefer Thomas's name (or maybe even
> object_initialize_child/object_initialize_childv), but if we adopt it we
> should also rename object_new_with_props and object_new_with_propv, for
> consistency.

Since we are in hard freeze already, I'd rather prefer to keep it
simple, without the variable args for now (we don't need them yet).
Especially since we can easily add them later when we need them,
changing the prototype from

void object_initialize_child(Object *parentobj, const char *propname,
                             void *childobj, size_t size,
                             const char *type, Error **errp);

to

void object_initialize_child(Object *parentobj, const char *propname,
                             void *childobj, size_t size,
                             const char *type, Error **errp, ...);

should not affect any of the existing caller sites later.

And yes, I think these functions should have "child" in their names, so
I'll continue with object_initialize_child now. Renaming
object_new_with_props to object_new_child also sounds like a good idea
to me, but that's something for the time when the hard freeze is over.

 Thomas

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Paolo Bonzini]

On 13/07/2018 09:57, Thomas Huth wrote:
> Since we are in hard freeze already, I'd rather prefer to keep it
> simple, without the variable args for now (we don't need them yet).
> Especially since we can easily add them later when we need them,
> changing the prototype from
> 
> void object_initialize_child(Object *parentobj, const char *propname,
>                              void *childobj, size_t size,
>                              const char *type, Error **errp);
> 
> to
> 
> void object_initialize_child(Object *parentobj, const char *propname,
>                              void *childobj, size_t size,
>                              const char *type, Error **errp, ...);
> 
> should not affect any of the existing caller sites later.
> 
> And yes, I think these functions should have "child" in their names, so
> I'll continue with object_initialize_child now. Renaming
> object_new_with_props to object_new_child also sounds like a good idea
> to me, but that's something for the time when the hard freeze is over.

It's true that we are in hard freeze, but now that I've noticed the
parallel with object_new_with_props, it seems to me that making the
functions subtly different (in naming or functionality) is just being
sloppy.  A search-and-replace patch is acceptable for hard freeze if
there is a good reason for it, and the rest is just copying code from
object_new_with_props into the no-malloc version.  The variable
arguments are handled simply by

    if (object_set_propv(obj, &local_err, vargs) < 0) {
        goto error;
    }

and another thing to do is

    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
        user_creatable_complete(obj, &local_err);
        if (local_err) {
            object_unparent(obj);
            goto error;
        }
    }

which is included in object_new_with_propv.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 1/5] qom/object: Add a new function object_initialize_as_child()

[Thomas Huth]

On 13.07.2018 12:12, Paolo Bonzini wrote:
> On 13/07/2018 09:57, Thomas Huth wrote:
>> Since we are in hard freeze already, I'd rather prefer to keep it
>> simple, without the variable args for now (we don't need them yet).
>> Especially since we can easily add them later when we need them,
>> changing the prototype from
>>
>> void object_initialize_child(Object *parentobj, const char *propname,
>>                              void *childobj, size_t size,
>>                              const char *type, Error **errp);
>>
>> to
>>
>> void object_initialize_child(Object *parentobj, const char *propname,
>>                              void *childobj, size_t size,
>>                              const char *type, Error **errp, ...);
>>
>> should not affect any of the existing caller sites later.
>>
>> And yes, I think these functions should have "child" in their names, so
>> I'll continue with object_initialize_child now. Renaming
>> object_new_with_props to object_new_child also sounds like a good idea
>> to me, but that's something for the time when the hard freeze is over.
> 
> It's true that we are in hard freeze, but now that I've noticed the
> parallel with object_new_with_props, it seems to me that making the
> functions subtly different (in naming or functionality) is just being
> sloppy.  A search-and-replace patch is acceptable for hard freeze if
> there is a good reason for it, and the rest is just copying code from
> object_new_with_props into the no-malloc version.  The variable
> arguments are handled simply by
> 
>     if (object_set_propv(obj, &local_err, vargs) < 0) {
>         goto error;
>     }
> 
> and another thing to do is
> 
>     if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
>         user_creatable_complete(obj, &local_err);
>         if (local_err) {
>             object_unparent(obj);
>             goto error;
>         }
>     }
Maybe. But I doubt that I've got enough spare time to implement and test
this before the next rc. So if anybody wants to see the fixes in the
next rc already, please pick up the v2 series. Otherwise, I'll have a
closer look next week.

 Thomas