[PATCH] timer: Fix a race condition between timer's callback and destroying code

Roman Kiryanov posted 1 patch 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240626181012.3321084-1-rkir@google.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
include/qemu/timer.h |  4 ++++
util/qemu-timer.c    | 22 ++++++++++++++++++++++
2 files changed, 26 insertions(+)
[PATCH] timer: Fix a race condition between timer's callback and destroying code
Posted by Roman Kiryanov 5 months ago
`timerlist_run_timers` provides no mechanism to
make sure the data pointed by `opaque` is valid
when calling timer's callback: there could be
another thread running which is destroying
timer's opaque data.

With this change `timer_del` becomes blocking if
timer's callback is running and it will be safe
to destroy timer's data once `timer_del` returns.

Google-Bug-Id: 348598513
Change-Id: I3cd6350587fb67c73bbee7b934d2c0c1acb27229
Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 include/qemu/timer.h |  4 ++++
 util/qemu-timer.c    | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 9a366e551f..de82ed78ae 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -3,6 +3,7 @@
 
 #include "qemu/bitops.h"
 #include "qemu/notify.h"
+#include "qemu/thread.h"
 #include "qemu/host-utils.h"
 
 #define NANOSECONDS_PER_SECOND 1000000000LL
@@ -86,9 +87,12 @@ struct QEMUTimer {
     QEMUTimerList *timer_list;
     QEMUTimerCB *cb;
     void *opaque;
+    QemuMutex opaque_lock;
+    QemuCond cb_done;
     QEMUTimer *next;
     int attributes;
     int scale;
+    bool cb_running;
 };
 
 extern QEMUTimerListGroup main_loop_tlg;
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 6a0de33dd2..9eb6fca5fa 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -369,7 +369,10 @@ void timer_init_full(QEMUTimer *ts,
     ts->opaque = opaque;
     ts->scale = scale;
     ts->attributes = attributes;
+    qemu_mutex_init(&ts->opaque_lock);
+    qemu_cond_init(&ts->cb_done);
     ts->expire_time = -1;
+    ts->cb_running = false;
 }
 
 void timer_deinit(QEMUTimer *ts)
@@ -394,6 +397,12 @@ static void timer_del_locked(QEMUTimerList *timer_list, QEMUTimer *ts)
         }
         pt = &t->next;
     }
+
+    qemu_mutex_lock(&ts->opaque_lock);
+    while (ts->cb_running) {
+        qemu_cond_wait(&ts->cb_done, &ts->opaque_lock);
+    }
+    qemu_mutex_unlock(&ts->opaque_lock);
 }
 
 static bool timer_mod_ns_locked(QEMUTimerList *timer_list,
@@ -571,9 +580,22 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
         cb = ts->cb;
         opaque = ts->opaque;
 
+        /* Mark the callback as running to prevent
+         * destroying `opaque` in another thread.
+         */
+        qemu_mutex_lock(&ts->opaque_lock);
+        ts->cb_running = true;
+        qemu_mutex_unlock(&ts->opaque_lock);
+
         /* run the callback (the timer list can be modified) */
         qemu_mutex_unlock(&timer_list->active_timers_lock);
         cb(opaque);
+
+        qemu_mutex_lock(&ts->opaque_lock);
+        ts->cb_running = false;
+        qemu_cond_broadcast(&ts->cb_done);
+        qemu_mutex_unlock(&ts->opaque_lock);
+
         qemu_mutex_lock(&timer_list->active_timers_lock);
 
         progress = true;
-- 
2.45.2.741.gdbec12cfda-goog