[v4] Add timer_join to avoid racing in timer cleanup

Commit Message

Roman Kiryanov July 1, 2024, 9:15 p.m. UTC

Currently there is no mechanism guaranteeing
that it is safe to delete the object pointed
by opaque in timer_init.

This race condition happens if a timer is
created on a separate thread and timer_del
is called between qemu_mutex_unlock and
cb(opaque) in timerlist_run_timers.

In this case the user thread will continue
executing (once timer_del returns) which could
cause destruction of the object pointed by
opaque while the callback is (or will be)
using it. See the example below:

void myThreadFunc() {
    void *opaque = malloc(42);
    QEMUTimer timer;

    timer_init(&timer, myClockType, myScale,
               &myTimerCallbackFunc, opaque);
    ...
    timer_del(&timer);
    free(opaque);
}

This change adds a function, timer_join,
that makes sure that the timer callback
(all timer callbacks, to be precise; this
is an implementation detail and could be
adjusted later) is done and it is safe to
destroy the object pointed by opaque.

Once this function is available, the example
above will look like this:

    timer_del(&timer);
    timer_join(&timer);
    free(opaque);

Signed-off-by: Roman Kiryanov <[e-mail]>
---
v2: rebased to the right branch and removed
    Google specific tags from the commit message.

v3: if a timer's callback happens to be running
    (cb_running) wait until all timers are done.
    qatomic_read/qemu_event_reset could be racing
    and timer_del might wait one extra loop of
    timers to be done.

v4: add timer_join implmented via aio_wait_bh_oneshot
    as suggested by [e-mail].

 include/qemu/timer.h | 11 +++++++++++
 util/qemu-timer.c    | 14 ++++++++++++++
 2 files changed, 25 insertions(+)

Patch

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 5ce83c7911..5f7d673830 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -635,6 +635,17 @@  void timer_deinit(QEMUTimer *ts);
  */
 void timer_del(QEMUTimer *ts);
 
+/**
+ * timer_join:
+ * @ts: the timer
+ *
+ * Wait for the timer callback to finish (if it is runniing).
+ *
+ * This function is thread-safe but the timer and its timer list must not be
+ * freed while this function is running.
+ */
+void timer_join(QEMUTimer *ts);
+
 /**
  * timer_free:
  * @ts: the timer
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 213114be68..22759be580 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -30,6 +30,8 @@ 
 #include "sysemu/replay.h"
 #include "sysemu/cpus.h"
 
+#include "block/aio-wait.h"
+
 #ifdef CONFIG_POSIX
 #include <pthread.h>
 #endif
@@ -438,6 +440,18 @@  void timer_del(QEMUTimer *ts)
     }
 }
 
+static void timer_join_noop(void *unused) {}
+
+/* Make sure the timer callback is done */
+void timer_join(QEMUTimer *ts)
+{
+    /* A side effect of aio_wait_bh_oneshot is all
+     * timer callbacks are done once it returns.
+     */
+    aio_wait_bh_oneshot(qemu_get_aio_context(),
+                        &timer_join_noop, NULL);
+}
+
 /* modify the current timer so that it will be fired when current_time
    >= expire_time. The corresponding callback will be called. */
 void timer_mod_ns(QEMUTimer *ts, int64_t expire_time)