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

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Nov 10 2022 at 01:41, Steven Rostedt wrote:
> +static inline int timer_shutdown_sync(struct timer_list *timer)
> +{
> +	return __del_timer_sync(timer, true);
> +}

> +static int __try_to_del_timer_sync(struct timer_list *timer, bool free)
>  {
>  	struct timer_base *base;
>  	unsigned long flags;
> @@ -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?

If we really want to make this work, then this needs at least a sanity
check of timer->function in the mod/add*_timer() path _after_ locking
the timer.

Though I'm not convinced that this would really be cutting it simply
because the circular dependencies of timer scheduling work and work
arming timer is as demonstrated above not as trivial as you might think.

In the worst case the concurrent code path might still end up in a UAF
as far as I can tell.

But what's worse is that you try to create the illusion that
timer_shutdown_sync() is actually preventing people from shooting
themself into their feet.

As implemented right now it's just a bandaid which makes it less likely,
but does neither prevent any of the hard to debug shutdown issues nor
the resulting holes in peoples feets.

Thanks,

        tglx