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

From: Steven Rostedt
Date: Sun Nov 13 2022 - 19:15:27 EST


On Mon, 14 Nov 2022 00:18:21 +0100
Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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