Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called before freeing timers | Date | Mon, 14 Nov 2022 00:18:21 +0100 |
| |
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
| |