hw/net/virtio-net: fix memory leak in timer_del()

Commit Message

Zheng Huang March 27, 2025, 10:44 a.m. UTC

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.

Also 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.

Signed-off-by: Zheng Huang <[e-mail]>

---
 hw/net/virtio-net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Prasad Pandit March 27, 2025, 12:49 p.m. UTC | #1

On Thu, 27 Mar 2025 at 16:15, Zheng Huang <[e-mail]> wrote:
> +++ b/hw/net/virtio-net.c
> @@ -422,7 +422,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>              }
>          } else {
>              if (q->tx_timer) {
> -                timer_del(q->tx_timer);
> +                timer_free(q->tx_timer);

* free()'ing a timer object while setting status in '_set_status()'
function does not seem right, probably timer_del() is intended here.

> @@ -2844,7 +2844,7 @@ static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
>
>      if (q->tx_waiting) {
>          /* We already have queued packets, immediately flush */
> -        timer_del(q->tx_timer);
> +        timer_free(q->tx_timer);
>          virtio_net_tx_timer(q);
>      } else {
>          /* re-arm timer to flush it (and more) on next tick */
> @@ -3982,7 +3982,7 @@ static void virtio_net_reset(VirtIODevice *vdev)
>      n->nobcast = 0;
>      /* multiqueue is disabled by default */
>      n->curr_queue_pairs = 1;
> -    timer_del(n->announce_timer.tm);
> +    timer_free(n->announce_timer.tm);
>      n->announce_timer.round = 0;
>      n->status &= ~VIRTIO_NET_S_ANNOUNCE;

* This timer_free() usage does not seem right, likely timer_del() is
intended here. And a memory leak is happening due to another reason.

Thank you.
---
  - Prasad

Patch

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index de87cfadff..ca403821e0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -422,7 +422,7 @@  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
             }
         } else {
             if (q->tx_timer) {
-                timer_del(q->tx_timer);
+                timer_free(q->tx_timer);
             } else {
                 qemu_bh_cancel(q->tx_bh);
             }
@@ -2844,7 +2844,7 @@  static void virtio_net_handle_tx_timer(VirtIODevice *vdev, VirtQueue *vq)
 
     if (q->tx_waiting) {
         /* We already have queued packets, immediately flush */
-        timer_del(q->tx_timer);
+        timer_free(q->tx_timer);
         virtio_net_tx_timer(q);
     } else {
         /* re-arm timer to flush it (and more) on next tick */
@@ -3982,7 +3982,7 @@  static void virtio_net_reset(VirtIODevice *vdev)
     n->nobcast = 0;
     /* multiqueue is disabled by default */
     n->curr_queue_pairs = 1;
-    timer_del(n->announce_timer.tm);
+    timer_free(n->announce_timer.tm);
     n->announce_timer.round = 0;
     n->status &= ~VIRTIO_NET_S_ANNOUNCE;