Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 7 Apr 2022 18:51:44 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>, jstultz@google.com,
        Stephen Boyd <sboyd@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        Eric Dumazet <edumazet@google.com>
Subject: Re: [RFC][PATCH] timers: Add del_time_free() to be called before
 freeing timers
Message-ID: <20220407185144.5ac036b7@gandalf.local.home>
In-Reply-To: <20220407215802.GB186606@roeck-us.net>
References: <20220407161745.7d6754b3@gandalf.local.home>
        <20220407215802.GB186606@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 7 Apr 2022 14:58:02 -0700
Guenter Roeck <linux@roeck-us.net> 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