hw/net/e1000.c | 6 +++--- hw/net/e1000e_core.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-)
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(-)
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);
--
2.34.1Hello,
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
© 2016 - 2025 Red Hat, Inc.