Re: [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called before freeing timers

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 14 Nov 2022 00:18:21 +0100
Thomas Gleixner <tglx@linutronix.de> wrote:

> > @@ -1285,11 +1281,25 @@ int try_to_del_timer_sync(struct timer_list *timer)
> >  
> >  	if (base->running_timer != timer)
> >  		ret = detach_if_pending(timer, base, true);
> > +	if (free)
> > +		timer->function = NULL;  
> 
> Same problem as in the timer_shutdown() case just more subtle:
> 
> CPU0                           		CPU1
> 
>                                         lock_timer(timer);
>                                         base->running_timer = timer;
> 					fn = timer->function;
> 					unlock_timer(timer);
> 					fn(timer) {
> 
> __try_to_del_timer_sync(timer, free=true)
>     lock_timer(timer);
>     if (base->running_timer != timer)
>        // Not taken
>     if (free)                             mod_timer(timer);
>                                             if (WARN_ON_ONCE(!timer->function))
>                                                return; // not taken
>        timer->function = NULL;
>     unlock_timer(timer);
> 					    lock_timer(timer);
>                                             enqueue_timer(timer);
> 					    unlock_timer(timer);
>                                         }
> 
> 					//timer expires
> 					lock_timer(timer);
> 					fn = timer->function;
> 					unlock_timer(timer);
> 					fn(timer); <--- NULL pointer dereference
> 
> You surely have spent a massive amount of analysis on this!
> 
> Can you please explain how you came up with the brilliant idea of asking
> Linus to pull this post -rc4 without a review from the timer maintainers
> or anyone else who understands concurrency?

I trusted the source of this code:

  https://lore.kernel.org/all/87pmlrkgi3.ffs@tglx/


-- Steve