| Message ID | 20200217032127.46508-3-pannengyuan@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | delay timer_new from init to realize to fix memleaks. | expand |
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
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 >
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
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 > . >
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 = {