Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

From: Steven Rostedt
Date: Thu Apr 07 2022 - 18:51:54 EST


On Thu, 7 Apr 2022 14:58:02 -0700
Guenter Roeck <linux@xxxxxxxxxxxx> wrote:

> > Add a del_timer_free() that not only does a del_timer_sync() but will mark
>
> This limits the use case to situations where del_timer_sync() can actually
> be called. There is, however, code where this is not possible.
> Specifically, it doesn't work if the code triggered with the timer uses a
> lock, and del_timer() is also called under that same lock. An example for
> that is the code in sound/synth/emux/emux.c. How do you suggest to handle
> that situation ?

Easy. Tell me how that situation is not a bug?

That code you point out at emux.c looks extremely buggy as well. In other
words, if you can't call del_timer_free() for the reason you mention above,
the code is very likely to have race conditions. I cannot think of a
situation that it is safe to do this.

In fact, I think that just replacing that with del_timer_free() may be good
enough. At least to show why it blows up later. I'm confused in what they
are doing by taking that lock. I can see:

CPU1 CPU2
---- ----
snd_emux_timer_callback()
spin_lock(voice_lock)
if (timer_active)
del_timer()
spin_unlock(voice_lock)

[..]
do_again++
[..]
if (do_again) {
mod_timer()
timer_active = 1;
}

[..]
free(emu);


BOOM!!

Hmm, perhaps I should change the code in __mod_timer() to:

if (WARN_ON(timer->flags & TIMER_FREED))
return;

To not rearm it.

-- Steve