[PATCH v2 0/2] delay timer_new from init to realize to fix memleaks.

[PATCH v2 0/2] delay timer_new from init to realize to fix memleaks.

[pannengyuan]

From: Pan Nengyuan <pannengyuan@huawei.com>

v1:
   - Delay timer_new() from init() to realize() to fix memleaks.
v2:
   - Similarly to other cleanups, move timer_new into realize in target/s390x/cpu.c (Suggested by Philippe Mathieu-Daudé).
   - Send these two patches as a series instead of sending each as a single patch but with wrong subject in v1.

Pan Nengyuan (2):
  s390x: fix memleaks in cpu_finalize
  hw: move timer_new from init() into realize() to avoid memleaks

 hw/arm/pxa2xx.c        | 17 +++++++++++------
 hw/arm/spitz.c         |  8 +++++++-
 hw/arm/strongarm.c     | 18 ++++++++++++------
 hw/misc/mos6522.c      | 14 ++++++++++++--
 hw/timer/cadence_ttc.c | 16 +++++++++++-----
 target/s390x/cpu.c     | 22 ++++++++++++++++++----
 6 files changed, 71 insertions(+), 24 deletions(-)

-- 
2.21.0.windows.1

[PATCH v2 1/2] s390x: fix memleaks in cpu_finalize

[pannengyuan]

From: Pan Nengyuan <pannengyuan@huawei.com>

This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:

Direct leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
    #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
    #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
    #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
    #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
    #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
    #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
    #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
    #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
    #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
    #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
    #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
    #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Cornelia Huck <cohuck@redhat.com>
---
Changes v2 to v1:
- Similarly to other cleanups, move timer_new into realize, then do
timer_del in unrealize.
---
 target/s390x/cpu.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index cf84d307c6..f18dbc6fe4 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
 #if !defined(CONFIG_USER_ONLY)
     S390CPU *cpu = S390_CPU(dev);
+    cpu->env.tod_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
+    cpu->env.cpu_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
 #endif
+
     Error *err = NULL;
 
     /* the model has to be realized before qemu_init_vcpu() due to kvm */
@@ -227,6 +232,16 @@ out:
     error_propagate(errp, err);
 }
 
+static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp)
+{
+#if !defined(CONFIG_USER_ONLY)
+    S390CPU *cpu = S390_CPU(dev);
+
+    timer_del(cpu->env.tod_timer);
+    timer_del(cpu->env.cpu_timer);
+#endif
+}
+
 static GuestPanicInformation *s390_cpu_get_crash_info(CPUState *cs)
 {
     GuestPanicInformation *panic_info;
@@ -279,10 +294,6 @@ static void s390_cpu_initfn(Object *obj)
                         s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
     s390_cpu_model_register_props(obj);
 #if !defined(CONFIG_USER_ONLY)
-    cpu->env.tod_timer =
-        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
-    cpu->env.cpu_timer =
-        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
     s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
 #endif
 }
@@ -294,6 +305,8 @@ static void s390_cpu_finalize(Object *obj)
 
     qemu_unregister_reset(s390_cpu_machine_reset_cb, cpu);
     g_free(cpu->irqstate);
+    timer_free(cpu->env.tod_timer);
+    timer_free(cpu->env.cpu_timer);
 #endif
 }
 
@@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
 
     device_class_set_parent_realize(dc, s390_cpu_realizefn,
                                     &scc->parent_realize);
+    dc->unrealize = s390_cpu_unrealizefn;
     device_class_set_props(dc, s390x_cpu_properties);
     dc->user_creatable = true;
 
-- 
2.21.0.windows.1

[PATCH v2 2/2] hw: move timer_new from init() into realize() to avoid memleaks

[pannengyuan]

From: Pan Nengyuan <pannengyuan@huawei.com>

There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
Meanwhile, do the null check in mos6522_reset() to avoid null deref if we move timer_new into realize().

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Changes v2 to v1:
- Send this patch in a series instead of a single patch but with wrong subject in v1.
---
 hw/arm/pxa2xx.c        | 17 +++++++++++------
 hw/arm/spitz.c         |  8 +++++++-
 hw/arm/strongarm.c     | 18 ++++++++++++------
 hw/misc/mos6522.c      | 14 ++++++++++++--
 hw/timer/cadence_ttc.c | 16 +++++++++++-----
 5 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index b33f8f1351..56a36202d7 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1134,18 +1134,22 @@ static void pxa2xx_rtc_init(Object *obj)
     s->last_rtcpicr = 0;
     s->last_hz = s->last_sw = s->last_pi = qemu_clock_get_ms(rtc_clock);
 
+    sysbus_init_irq(dev, &s->rtc_irq);
+
+    memory_region_init_io(&s->iomem, obj, &pxa2xx_rtc_ops, s,
+                          "pxa2xx-rtc", 0x10000);
+    sysbus_init_mmio(dev, &s->iomem);
+}
+
+static void pxa2xx_rtc_realize(DeviceState *dev, Error **errp)
+{
+    PXA2xxRTCState *s = PXA2XX_RTC(dev);
     s->rtc_hz    = timer_new_ms(rtc_clock, pxa2xx_rtc_hz_tick,    s);
     s->rtc_rdal1 = timer_new_ms(rtc_clock, pxa2xx_rtc_rdal1_tick, s);
     s->rtc_rdal2 = timer_new_ms(rtc_clock, pxa2xx_rtc_rdal2_tick, s);
     s->rtc_swal1 = timer_new_ms(rtc_clock, pxa2xx_rtc_swal1_tick, s);
     s->rtc_swal2 = timer_new_ms(rtc_clock, pxa2xx_rtc_swal2_tick, s);
     s->rtc_pi    = timer_new_ms(rtc_clock, pxa2xx_rtc_pi_tick,    s);
-
-    sysbus_init_irq(dev, &s->rtc_irq);
-
-    memory_region_init_io(&s->iomem, obj, &pxa2xx_rtc_ops, s,
-                          "pxa2xx-rtc", 0x10000);
-    sysbus_init_mmio(dev, &s->iomem);
 }
 
 static int pxa2xx_rtc_pre_save(void *opaque)
@@ -1203,6 +1207,7 @@ static void pxa2xx_rtc_sysbus_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "PXA2xx RTC Controller";
     dc->vmsd = &vmstate_pxa2xx_rtc_regs;
+    dc->realize = pxa2xx_rtc_realize;
 }
 
 static const TypeInfo pxa2xx_rtc_sysbus_info = {
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index e001088103..cbfa6934cf 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -524,11 +524,16 @@ static void spitz_keyboard_init(Object *obj)
 
     spitz_keyboard_pre_map(s);
 
-    s->kbdtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, spitz_keyboard_tick, s);
     qdev_init_gpio_in(dev, spitz_keyboard_strobe, SPITZ_KEY_STROBE_NUM);
     qdev_init_gpio_out(dev, s->sense, SPITZ_KEY_SENSE_NUM);
 }
 
+static void spitz_keyboard_realize(DeviceState *dev, Error **errp)
+{
+    SpitzKeyboardState *s = SPITZ_KEYBOARD(dev);
+    s->kbdtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, spitz_keyboard_tick, s);
+}
+
 /* LCD backlight controller */
 
 #define LCDTG_RESCTL	0x00
@@ -1115,6 +1120,7 @@ static void spitz_keyboard_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_spitz_kbd;
+    dc->realize = spitz_keyboard_realize;
 }
 
 static const TypeInfo spitz_keyboard_info = {
diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index cd8a99aaf2..3010d765bb 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -399,9 +399,6 @@ static void strongarm_rtc_init(Object *obj)
     s->last_rcnr = (uint32_t) mktimegm(&tm);
     s->last_hz = qemu_clock_get_ms(rtc_clock);
 
-    s->rtc_alarm = timer_new_ms(rtc_clock, strongarm_rtc_alarm_tick, s);
-    s->rtc_hz = timer_new_ms(rtc_clock, strongarm_rtc_hz_tick, s);
-
     sysbus_init_irq(dev, &s->rtc_irq);
     sysbus_init_irq(dev, &s->rtc_hz_irq);
 
@@ -410,6 +407,13 @@ static void strongarm_rtc_init(Object *obj)
     sysbus_init_mmio(dev, &s->iomem);
 }
 
+static void strongarm_rtc_realize(DeviceState *dev, Error **errp)
+{
+    StrongARMRTCState *s = STRONGARM_RTC(dev);
+    s->rtc_alarm = timer_new_ms(rtc_clock, strongarm_rtc_alarm_tick, s);
+    s->rtc_hz = timer_new_ms(rtc_clock, strongarm_rtc_hz_tick, s);
+}
+
 static int strongarm_rtc_pre_save(void *opaque)
 {
     StrongARMRTCState *s = opaque;
@@ -451,6 +455,7 @@ static void strongarm_rtc_sysbus_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "StrongARM RTC Controller";
     dc->vmsd = &vmstate_strongarm_rtc_regs;
+    dc->realize = strongarm_rtc_realize;
 }
 
 static const TypeInfo strongarm_rtc_sysbus_info = {
@@ -1240,15 +1245,16 @@ static void strongarm_uart_init(Object *obj)
                           "uart", 0x10000);
     sysbus_init_mmio(dev, &s->iomem);
     sysbus_init_irq(dev, &s->irq);
-
-    s->rx_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, strongarm_uart_rx_to, s);
-    s->tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, strongarm_uart_tx, s);
 }
 
 static void strongarm_uart_realize(DeviceState *dev, Error **errp)
 {
     StrongARMUARTState *s = STRONGARM_UART(dev);
 
+    s->rx_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                       strongarm_uart_rx_to,
+                                       s);
+    s->tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, strongarm_uart_tx, s);
     qemu_chr_fe_set_handlers(&s->chr,
                              strongarm_uart_can_receive,
                              strongarm_uart_receive,
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 19e154b870..980eda7599 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -465,11 +465,15 @@ static void mos6522_reset(DeviceState *dev)
     s->timers[0].frequency = s->frequency;
     s->timers[0].latch = 0xffff;
     set_counter(s, &s->timers[0], 0xffff);
-    timer_del(s->timers[0].timer);
+    if (s->timers[0].timer) {
+        timer_del(s->timers[0].timer);
+    }
 
     s->timers[1].frequency = s->frequency;
     s->timers[1].latch = 0xffff;
-    timer_del(s->timers[1].timer);
+    if (s->timers[1].timer) {
+        timer_del(s->timers[1].timer);
+    }
 }
 
 static void mos6522_init(Object *obj)
@@ -485,6 +489,11 @@ static void mos6522_init(Object *obj)
     for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
         s->timers[i].index = i;
     }
+}
+
+static void mos6522_realize(DeviceState *dev, Error **errp)
+{
+    MOS6522State *s = MOS6522(dev);
 
     s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
     s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
@@ -502,6 +511,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
 
     dc->reset = mos6522_reset;
     dc->vmsd = &vmstate_mos6522;
+    dc->realize = mos6522_realize;
     device_class_set_props(dc, mos6522_properties);
     mdc->parent_reset = dc->reset;
     mdc->set_sr_int = mos6522_set_sr_int;
diff --git a/hw/timer/cadence_ttc.c b/hw/timer/cadence_ttc.c
index 5e3128c1e3..b0ba6b2bba 100644
--- a/hw/timer/cadence_ttc.c
+++ b/hw/timer/cadence_ttc.c
@@ -412,16 +412,21 @@ static void cadence_timer_init(uint32_t freq, CadenceTimerState *s)
 static void cadence_ttc_init(Object *obj)
 {
     CadenceTTCState *s = CADENCE_TTC(obj);
+
+    memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
+                          "timer", 0x1000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
+}
+
+static void cadence_ttc_realize(DeviceState *dev, Error **errp)
+{
+    CadenceTTCState *s = CADENCE_TTC(dev);
     int i;
 
     for (i = 0; i < 3; ++i) {
         cadence_timer_init(133000000, &s->timer[i]);
-        sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->timer[i].irq);
+        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->timer[i].irq);
     }
-
-    memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
-                          "timer", 0x1000);
-    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
 }
 
 static int cadence_timer_pre_save(void *opaque)
@@ -479,6 +484,7 @@ static void cadence_ttc_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_cadence_ttc;
+    dc->realize = cadence_ttc_realize;
 }
 
 static const TypeInfo cadence_ttc_info = {
-- 
2.21.0.windows.1

Re: [PATCH v2 1/2] s390x: fix memleaks in cpu_finalize

[Philippe Mathieu-Daudé]

On 2/17/20 4:21 AM, pannengyuan@huawei.com wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
> 
> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>      #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>      #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>      #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>      #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>      #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>      #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>      #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>      #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>      #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>      #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>      #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>      #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>      #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Cornelia Huck <cohuck@redhat.com>
> ---
> Changes v2 to v1:
> - Similarly to other cleanups, move timer_new into realize, then do
> timer_del in unrealize.
> ---
>   target/s390x/cpu.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index cf84d307c6..f18dbc6fe4 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>       S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>   #if !defined(CONFIG_USER_ONLY)
>       S390CPU *cpu = S390_CPU(dev);
> +    cpu->env.tod_timer =
> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
> +    cpu->env.cpu_timer =
> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>   #endif
> +
>       Error *err = NULL;
>   
>       /* the model has to be realized before qemu_init_vcpu() due to kvm */
> @@ -227,6 +232,16 @@ out:
>       error_propagate(errp, err);
>   }
>   
> +static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    S390CPU *cpu = S390_CPU(dev);
> +
> +    timer_del(cpu->env.tod_timer);
> +    timer_del(cpu->env.cpu_timer);
> +#endif
> +}
> +
>   static GuestPanicInformation *s390_cpu_get_crash_info(CPUState *cs)
>   {
>       GuestPanicInformation *panic_info;
> @@ -279,10 +294,6 @@ static void s390_cpu_initfn(Object *obj)
>                           s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
>       s390_cpu_model_register_props(obj);
>   #if !defined(CONFIG_USER_ONLY)
> -    cpu->env.tod_timer =
> -        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
> -    cpu->env.cpu_timer =
> -        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>       s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>   #endif
>   }
> @@ -294,6 +305,8 @@ static void s390_cpu_finalize(Object *obj)
>   
>       qemu_unregister_reset(s390_cpu_machine_reset_cb, cpu);
>       g_free(cpu->irqstate);
> +    timer_free(cpu->env.tod_timer);
> +    timer_free(cpu->env.cpu_timer);
>   #endif
>   }
>   
> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>   
>       device_class_set_parent_realize(dc, s390_cpu_realizefn,
>                                       &scc->parent_realize);
> +    dc->unrealize = s390_cpu_unrealizefn;
>       device_class_set_props(dc, s390x_cpu_properties);
>       dc->user_creatable = true;
>   
> 

Thanks for v2.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 1/2] s390x: fix memleaks in cpu_finalize

[Cornelia Huck]

On Mon, 17 Feb 2020 11:21:26 +0800
<pannengyuan@huawei.com> wrote:

> From: Pan Nengyuan <pannengyuan@huawei.com>
> 
> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
> 
> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Cornelia Huck <cohuck@redhat.com>
> ---
> Changes v2 to v1:
> - Similarly to other cleanups, move timer_new into realize, then do
> timer_del in unrealize.
> ---
>  target/s390x/cpu.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index cf84d307c6..f18dbc6fe4 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>  #if !defined(CONFIG_USER_ONLY)
>      S390CPU *cpu = S390_CPU(dev);
> +    cpu->env.tod_timer =
> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
> +    cpu->env.cpu_timer =
> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);

I may be missing something, but what cleans up the timers if we fail
further down in this function? I don't think the unrealize callback is
invoked by the core in case of error?

Also, as a matter of personal preference, I think it would be better to
initialize the timers in the !CONFIG_USER_ONLY section further below,
rather than in the variable declaration section.

>  #endif
> +
>      Error *err = NULL;
>  
>      /* the model has to be realized before qemu_init_vcpu() due to kvm */

Re: [PATCH v2 1/2] s390x: fix memleaks in cpu_finalize

[Peter Maydell]

On Thu, 20 Feb 2020 at 16:01, Cornelia Huck <cohuck@redhat.com> wrote:
> I may be missing something, but what cleans up the timers if we fail
> further down in this function? I don't think the unrealize callback is
> invoked by the core in case of error?

FWIW I sent a mail to one of these threads a few days ago
where I claimed we did call unrealize if realize fails,
but looking again at the device_set_realized() code I think
I was misreading it. If the device's own 'realize' method
fails, we go to the 'fail' label, which just reports back the
error and doesn't call unrealize. It's only if the device's
'realize' method succeeds and we get a failure in some later
thing like calling 'realize' on the child bus objects that we
will call the device unrealize as part of cleanup.

Apologies for any confusion caused.

thanks
-- PMM

Re: [PATCH v2 2/2] hw: move timer_new from init() into realize() to avoid memleaks

[Peter Maydell]

On Mon, 17 Feb 2020 at 03:22, <pannengyuan@huawei.com> wrote:
>
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
> Meanwhile, do the null check in mos6522_reset() to avoid null deref if we move timer_new into realize().
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 19e154b870..980eda7599 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -465,11 +465,15 @@ static void mos6522_reset(DeviceState *dev)
>      s->timers[0].frequency = s->frequency;
>      s->timers[0].latch = 0xffff;
>      set_counter(s, &s->timers[0], 0xffff);
> -    timer_del(s->timers[0].timer);
> +    if (s->timers[0].timer) {
> +        timer_del(s->timers[0].timer);
> +    }
>
>      s->timers[1].frequency = s->frequency;
>      s->timers[1].latch = 0xffff;
> -    timer_del(s->timers[1].timer);
> +    if (s->timers[1].timer) {
> +        timer_del(s->timers[1].timer);
> +    }
>  }

What code path calls a device 'reset' method on a device
that has not yet been realized ? I wasn't expecting that
to be valid...

thanks
-- PMM

Re: [PATCH v2 2/2] hw: move timer_new from init() into realize() to avoid memleaks

[Philippe Mathieu-Daudé]

On 2/20/20 6:56 PM, Peter Maydell wrote:
> On Mon, 17 Feb 2020 at 03:22, <pannengyuan@huawei.com> wrote:
>>
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>> Meanwhile, do the null check in mos6522_reset() to avoid null deref if we move timer_new into realize().
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> 
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index 19e154b870..980eda7599 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -465,11 +465,15 @@ static void mos6522_reset(DeviceState *dev)
>>       s->timers[0].frequency = s->frequency;
>>       s->timers[0].latch = 0xffff;
>>       set_counter(s, &s->timers[0], 0xffff);
>> -    timer_del(s->timers[0].timer);
>> +    if (s->timers[0].timer) {
>> +        timer_del(s->timers[0].timer);
>> +    }
>>
>>       s->timers[1].frequency = s->frequency;
>>       s->timers[1].latch = 0xffff;
>> -    timer_del(s->timers[1].timer);
>> +    if (s->timers[1].timer) {
>> +        timer_del(s->timers[1].timer);
>> +    }
>>   }
> 
> What code path calls a device 'reset' method on a device
> that has not yet been realized ? I wasn't expecting that
> to be valid...

This is not valid. What I understood while reviewing this patch is on 
reset the timer is removed from the timers list. But this patch miss 
setting timer = NULL in case the device is reset multiple times, here 
can happen a NULL deref.

> 
> thanks
> -- PMM
> 

Re: [PATCH v2 2/2] hw: move timer_new from init() into realize() to avoid memleaks

[Peter Maydell]

On Thu, 20 Feb 2020 at 18:52, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 2/20/20 6:56 PM, Peter Maydell wrote:
> > On Mon, 17 Feb 2020 at 03:22, <pannengyuan@huawei.com> wrote:
> >>
> >> From: Pan Nengyuan <pannengyuan@huawei.com>
> >>
> >> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
> >> Meanwhile, do the null check in mos6522_reset() to avoid null deref if we move timer_new into realize().
> >>
> >> Reported-by: Euler Robot <euler.robot@huawei.com>
> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >
> >
> >> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> >> index 19e154b870..980eda7599 100644
> >> --- a/hw/misc/mos6522.c
> >> +++ b/hw/misc/mos6522.c
> >> @@ -465,11 +465,15 @@ static void mos6522_reset(DeviceState *dev)
> >>       s->timers[0].frequency = s->frequency;
> >>       s->timers[0].latch = 0xffff;
> >>       set_counter(s, &s->timers[0], 0xffff);
> >> -    timer_del(s->timers[0].timer);
> >> +    if (s->timers[0].timer) {
> >> +        timer_del(s->timers[0].timer);
> >> +    }
> >>
> >>       s->timers[1].frequency = s->frequency;
> >>       s->timers[1].latch = 0xffff;
> >> -    timer_del(s->timers[1].timer);
> >> +    if (s->timers[1].timer) {
> >> +        timer_del(s->timers[1].timer);
> >> +    }
> >>   }
> >
> > What code path calls a device 'reset' method on a device
> > that has not yet been realized ? I wasn't expecting that
> > to be valid...
>
> This is not valid. What I understood while reviewing this patch is on
> reset the timer is removed from the timers list. But this patch miss
> setting timer = NULL in case the device is reset multiple times, here
> can happen a NULL deref.

I should have checked the APIs here.

timer_new() allocates memory and initialises a timer.
timer_del() removes a timer from any list it is on, but
does not deallocate memory. It's the function you call
to stop a timer (and arguably timer_stop() would be a
better name for it).
If you created the timer with timer_init(), then the
code to clean it up is:
 (1) call timer_del() to make sure it's not on any
list of active timers
 (2) call timer_free()

So:
 * the mos6522_reset code is fine as it is
 * if we wanted cleanup code that undoes the timer_new
   then that would be a timer_del() + timer_free().
   This would go in unrealize if the timer_new is put
   in realize, but...
 * ...like the other devices touched in this patch,
   mos6522 isn't user-creatable, so if realize succeeds
   it won't ever be destroyed; so we don't need to
   do that. (This is a little harder to check than
   with most of these devices, since mos6522 is an
   abstract base class for some other devices, but
   I think it's correct.)

Side notes:
 * for new code, rather than using timer_new() or one
   of its sibling functions, prefer timer_init(),
   timer_init_ns(), etc. These take a pointer to a
   pre-existing QEMUTimer, typically one you have
   directly embedded in the device state struct. So
   they don't need to be freed on unrealize (though
   you do still want to make sure the timer is not
   on an active list with timer_del() before the memory
   in the device state struct goes away).
 * maybe timer_free() should call timer_del(),
   rather than obliging the caller to?

thanks
-- PMM

Re: [PATCH v2 2/2] hw: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan]

On 2/21/2020 1:56 AM, Peter Maydell wrote:
> On Mon, 17 Feb 2020 at 03:22, <pannengyuan@huawei.com> wrote:
>>
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>> Meanwhile, do the null check in mos6522_reset() to avoid null deref if we move timer_new into realize().
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> 
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index 19e154b870..980eda7599 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -465,11 +465,15 @@ static void mos6522_reset(DeviceState *dev)
>>      s->timers[0].frequency = s->frequency;
>>      s->timers[0].latch = 0xffff;
>>      set_counter(s, &s->timers[0], 0xffff);
>> -    timer_del(s->timers[0].timer);
>> +    if (s->timers[0].timer) {
>> +        timer_del(s->timers[0].timer);
>> +    }
>>
>>      s->timers[1].frequency = s->frequency;
>>      s->timers[1].latch = 0xffff;
>> -    timer_del(s->timers[1].timer);
>> +    if (s->timers[1].timer) {
>> +        timer_del(s->timers[1].timer);
>> +    }
>>  }
> 
> What code path calls a device 'reset' method on a device
> that has not yet been realized ? I wasn't expecting that
> to be valid...

I got the follow null-deref case on m68k If I move timer_new into realize():

    #0 0x55cbb0d3e9f9 in timer_del /mnt/sdb/qemu-new/qemu/util/qemu-timer.c:429
    #1 0x55cbb04f3abe in mos6522_reset /mnt/sdb/qemu-new/qemu/hw/misc/mos6522.c:468
    #2 0x55cbb02b5fd5 in mos6522_q800_via2_reset /mnt/sdb/qemu-new/qemu/hw/misc/mac_via.c:1098
    #3 0x55cbb047b926 in device_transitional_reset /mnt/sdb/qemu-new/qemu/hw/core/qdev.c:1136
    #4 0x55cbb0491a71 in resettable_phase_hold /mnt/sdb/qemu-new/qemu/hw/core/resettable.c:182
    #5 0x55cbb048700e in bus_reset_child_foreach /mnt/sdb/qemu-new/qemu/hw/core/bus.c:94
    #6 0x55cbb0490f66 in resettable_child_foreach /mnt/sdb/qemu-new/qemu/hw/core/resettable.c:96
    #7 0x55cbb0491896 in resettable_phase_hold /mnt/sdb/qemu-new/qemu/hw/core/resettable.c:173
    #8 0x55cbb0490c06 in resettable_assert_reset /mnt/sdb/qemu-new/qemu/hw/core/resettable.c:60
    #9 0x55cbb0490aec in resettable_reset /mnt/sdb/qemu-new/qemu/hw/core/resettable.c:45
    #10 0x55cbb0492668 in resettable_cold_reset_fn /mnt/sdb/qemu-new/qemu/hw/core/resettable.c:269
    #11 0x55cbb0494a04 in qemu_devices_reset /mnt/sdb/qemu-new/qemu/hw/core/reset.c:69
    #12 0x55cbb03ab91d in qemu_system_reset /mnt/sdb/qemu-new/qemu/vl.c:1412
    #13 0x55cbb03bfe04 in main /mnt/sdb/qemu-new/qemu/vl.c:4403

mos6522_init was called in mac_via_realize as follow, but mos6522_realize was not called at all.
So maybe we shouldn't move it into realize or add realize step in this code path?

    #0  0x0000555555789e40 in mos6522_init (obj=0x555557537b00) at /mnt/sdb/qemu-new/qemu/hw/misc/mos6522.c:476
    #1  0x000055555581b6c3 in object_init_with_type (obj=0x555557537b00, ti=0x55555617c2b0) at /mnt/sdb/qemu-new/qemu/qom/object.c:372
    #2  0x000055555581cc80 in object_initialize_with_type (data=data@entry=0x555557537b00, size=1504, type=0x55555617c2b0) at /mnt/sdb/qemu-new/qemu/qom/object.c:516
    #3  0x000055555581cd1f in object_initialize (data=data@entry=0x555557537b00, size=<optimized out>, typename=<optimized out>) at /mnt/sdb/qemu-new/qemu/qom/object.c:529
    #4  0x000055555581e387 in object_initialize_childv
    (parentobj=0x555557537510, propname=0x555555a3c673 "via1", childobj=0x555557537b00, size=<optimized out>, type=<optimized out>, errp=0x55555613b338 <error_abort>, vargs=0x7fffffffdb30)
    at /mnt/sdb/qemu-new/qemu/qom/object.c:552
    #5  0x000055555581e4d3 in object_initialize_child
    (parentobj=<optimized out>, propname=<optimized out>, childobj=childobj@entry=0x555557537b00, size=<optimized out>, type=<optimized out>, errp=<optimized out>) at /mnt/sdb/qemu-new/qemu/qom/object.c:539
    #6  0x000055555577ba88 in sysbus_init_child_obj (parent=<optimized out>, childname=<optimized out>, child=0x555557537b00, childsize=<optimized out>, childtype=<optimized out>)
    at /mnt/sdb/qemu-new/qemu/hw/core/sysbus.c:352
    #7  0x000055555570d301 in mac_via_realize (dev=0x555557537510, errp=0x7fffffffdce0) at /mnt/sdb/qemu-new/qemu/hw/misc/mac_via.c:876
    #8  0x0000555555774444 in device_set_realized (obj=0x555557537510, value=<optimized out>, errp=0x7fffffffddd0) at /mnt/sdb/qemu-new/qemu/hw/core/qdev.c:891
    #9  0x000055555581b266 in property_set_bool (obj=0x555557537510, v=<optimized out>, name=<optimized out>, opaque=0x555556165f50, errp=0x7fffffffddd0) at /mnt/sdb/qemu-new/qemu/qom/object.c:2238
    #10 0x000055555581feee in object_property_set_qobject (obj=0x555557537510, value=<optimized out>, name=0x555555a5fa67 "realized", errp=0x7fffffffddd0) at /mnt/sdb/qemu-new/qemu/qom/qom-qobject.c:26
    #11 0x000055555581d60f in object_property_set_bool (obj=0x555557537510, value=<optimized out>, name=0x555555a5fa67 "realized", errp=0x7fffffffddd0) at /mnt/sdb/qemu-new/qemu/qom/object.c:1390
    #12 0x0000555555773381 in qdev_init_nofail (dev=dev@entry=0x555557537510) at /mnt/sdb/qemu-new/qemu/hw/core/qdev.c:418
    #13 0x0000555555711fcd in q800_init (machine=<optimized out>) at /mnt/sdb/qemu-new/qemu/hw/m68k/q800.c:230
    #14 0x0000555555686dfb in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at /mnt/sdb/qemu-new/qemu/vl.c:4308

And I have another quesion, how to distinguish whether the realize() will be called or not.

Thanks.

> 
> thanks
> -- PMM
> .
>