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

[Roman Kiryanov]

On Thu, Jun 27, 2024 at 6:27 AM Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> On Thu, Jun 27, 2024 at 2:32 AM Roman Kiryanov <r...@google.com> wrote:
> > +        if (qatomic_read(&ts->cb_running)) {
> > +            qemu_event_wait(&timer_list->timers_done_ev);
> > +        }
>
> qemu_event_wait() already has the right atomic magic, and
> ts->cb_running is both redundant (in general), and I think racy (as
> implemented in this patch).

I added cb_running to avoid waiting for timers_done_ev if we know our
cb is done.

> But especially, you haven't justified in the commit message _why_ you
> need this.

I mentioned the problem of cleanup racing with the timer's callback function
in the current shape of QEMU.

> Since this problem is not timer-specific,

It would be nice to fix all the places then.

> using
> aio_bh_schedule_oneshot() or aio_wait_bh_oneshot() to synchronize
> everything with the AioContext thread seems like a superior solution
> to me.

Could you please elaborate? The problem we want to solve is this:

void myThreadFunc() {
    CallbackState callbackState;
    QEMUTimer timer;

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

Currently, myTimerCallbackFunc could fire after myThreadFunc exits
(if timer_del runs between qemu_mutex_unlock and cb(opaque) in
timerlist_run_timers) and callbackState gets destroyed.