| Message ID | 551d9eaf-dce0-4cf0-95b1-d2347bfaa1a6@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/net/e1000: fix memory leak in timer_del() | expand |
Hello, On Thu, 27 Mar 2025 at 16:59, Zheng Huang <hz1624917200@gmail.com> wrote: > This patch addresses a memory leak bug in the usages of `timer_del()`. > The issue arisesfrom the incorrect use of the ambiguous timer API > `timer_del()`, which does not free the timer object. The leak sanitizer > report this issue during fuzzing. The correct API `timer_free()` freed > the timer object instead. > > In addition, I'd like to ask for a way to fix all 100+ wrong usages. In my > opinion, the best way to fix this is to hide to `timer_del()` API and > eliminate all usages of it. > > @@ -379,9 +379,9 @@ static void e1000_reset_hold(Object *obj, ResetType type) > E1000BaseClass *edc = E1000_GET_CLASS(d); > uint8_t *macaddr = d->conf.macaddr.a; > > - timer_del(d->autoneg_timer); > - timer_del(d->mit_timer); > - timer_del(d->flush_queue_timer); > + timer_free(d->autoneg_timer); > + timer_free(d->mit_timer); > + timer_free(d->flush_queue_timer); > d->mit_timer_on = 0; > d->mit_irq_level = 0; > d->mit_ide = 0; > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > index 2413858790..61fdc8a3e9 100644 > --- a/hw/net/e1000e_core.c > +++ b/hw/net/e1000e_core.c > @@ -127,7 +127,7 @@ static inline void > e1000e_intrmgr_stop_timer(E1000IntrDelayTimer *timer) > { > if (timer->running) { > - timer_del(timer->timer); > + timer_free(timer->timer); > timer->running = false; > } > } > @@ -360,13 +360,13 @@ e1000e_intrmgr_fire_all_timers(E1000ECore *core) > int i; > > if (core->itr.running) { > - timer_del(core->itr.timer); > + timer_free(core->itr.timer); > e1000e_intrmgr_on_throttling_timer(&core->itr); > } > > for (i = 0; i < E1000E_MSIX_VEC_NUM; i++) { > if (core->eitr[i].running) { > - timer_del(core->eitr[i].timer); > + timer_free(core->eitr[i].timer); > e1000e_intrmgr_on_msix_throttling_timer(&core->eitr[i]); > } > } > @@ -3452,7 +3452,7 @@ static void e1000e_reset(E1000ECore *core, bool sw) > { > int i; > > - timer_del(core->autoneg_timer); > + timer_free(core->autoneg_timer); > > e1000e_intrmgr_reset(core); * I doubt if this is correct; Because timer_del() API explicitly says -> /* stop a timer, but do not dealloc it */ * Secondly: autoneg_timer/mit_timer/flush_queue_timer objects are freed in 'pci_e1000_uninit()/e1000e_pci_uninit()' functions via timer_free() calls. So the memory leak could be because the respective *_pci__uninit() function is not called? Thank you. --- - Prasad
Hello Prasad, 在 2025/3/27 20:40, Prasad Pandit wrote: >> @@ -360,13 +360,13 @@ e1000e_intrmgr_fire_all_timers(E1000ECore *core) >> int i; >> >> if (core->itr.running) { >> - timer_del(core->itr.timer); >> + timer_free(core->itr.timer); >> e1000e_intrmgr_on_throttling_timer(&core->itr); >> } >> >> for (i = 0; i < E1000E_MSIX_VEC_NUM; i++) { >> if (core->eitr[i].running) { >> - timer_del(core->eitr[i].timer); >> + timer_free(core->eitr[i].timer); >> e1000e_intrmgr_on_msix_throttling_timer(&core->eitr[i]); >> } >> } >> @@ -3452,7 +3452,7 @@ static void e1000e_reset(E1000ECore *core, bool sw) >> { >> int i; >> >> - timer_del(core->autoneg_timer); >> + timer_free(core->autoneg_timer); >> >> e1000e_intrmgr_reset(core); > > * I doubt if this is correct; Because timer_del() API explicitly says > -> /* stop a timer, but do not dealloc it */ > > * Secondly: autoneg_timer/mit_timer/flush_queue_timer objects are > freed in 'pci_e1000_uninit()/e1000e_pci_uninit()' functions via > timer_free() calls. So the memory leak could be because the respective > *_pci__uninit() function is not called? Yes, you are right. I mistakenly assumed that there's no way to reenable a `_del()`ed timer. Thank you sincerely for pointing out this. I'll check the usage carefully in other occurences. Best regard, Zheng
diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 3d0b227703..5dddf9e0a7 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -379,9 +379,9 @@ static void e1000_reset_hold(Object *obj, ResetType type) E1000BaseClass *edc = E1000_GET_CLASS(d); uint8_t *macaddr = d->conf.macaddr.a; - timer_del(d->autoneg_timer); - timer_del(d->mit_timer); - timer_del(d->flush_queue_timer); + timer_free(d->autoneg_timer); + timer_free(d->mit_timer); + timer_free(d->flush_queue_timer); d->mit_timer_on = 0; d->mit_irq_level = 0; d->mit_ide = 0; diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index 2413858790..61fdc8a3e9 100644 --- a/hw/net/e1000e_core.c +++ b/hw/net/e1000e_core.c @@ -127,7 +127,7 @@ static inline void e1000e_intrmgr_stop_timer(E1000IntrDelayTimer *timer) { if (timer->running) { - timer_del(timer->timer); + timer_free(timer->timer); timer->running = false; } } @@ -360,13 +360,13 @@ e1000e_intrmgr_fire_all_timers(E1000ECore *core) int i; if (core->itr.running) { - timer_del(core->itr.timer); + timer_free(core->itr.timer); e1000e_intrmgr_on_throttling_timer(&core->itr); } for (i = 0; i < E1000E_MSIX_VEC_NUM; i++) { if (core->eitr[i].running) { - timer_del(core->eitr[i].timer); + timer_free(core->eitr[i].timer); e1000e_intrmgr_on_msix_throttling_timer(&core->eitr[i]); } } @@ -3452,7 +3452,7 @@ static void e1000e_reset(E1000ECore *core, bool sw) { int i; - timer_del(core->autoneg_timer); + timer_free(core->autoneg_timer); e1000e_intrmgr_reset(core);
Hi, This patch addresses a memory leak bug in the usages of `timer_del()`. The issue arisesfrom the incorrect use of the ambiguous timer API `timer_del()`, which does not free the timer object. The leak sanitizer report this issue during fuzzing. The correct API `timer_free()` freed the timer object instead. In addition, I'd like to ask for a way to fix all 100+ wrong usages. In my opinion, the best way to fix this is to hide to `timer_del()` API and eliminate all usages of it. ps: Sorry for the mistake in subject of the previous mail. Signed-off-by: Zheng Huang <hz1624917200@outlook.com> --- hw/net/e1000.c | 6 +++--- hw/net/e1000e_core.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-)