Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <63d4c8d8-89c6-33ed-8178-be9ea86e53b9@roeck-us.net>
Date:   Thu, 7 Apr 2022 17:58:09 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>
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>
References: <20220407161745.7d6754b3@gandalf.local.home>
 <20220407215802.GB186606@roeck-us.net>
 <20220407185144.5ac036b7@gandalf.local.home>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [RFC][PATCH] timers: Add del_time_free() to be called before
 freeing timers
In-Reply-To: <20220407185144.5ac036b7@gandalf.local.home>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/7/22 15:51, Steven Rostedt wrote:
> 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?
> 

Sure, fixing the problem is of course the right thing to do. But replacing
del_timer() with your suggested version of del_timer_free() doesn't work
with this code because it would deadlock. Sure, that would not fix the
underlying problem anyway, but that isn't the point I was trying to make:
I think it would be beneficial to be able to replace del_timer() with a
version that can not result in deadlocks but would still identify problems
such as the one in the code in emux.c.

Can we have del_timer_free() and del_timer_sync_free() ? Or am I missing
something and that doesn't really make sense ?

Thanks,
Guenter

> 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