All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Steven Rostedt <rostedt@goodmis.org>, linux-kernel@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Anna-Maria Gleixner <anna-maria@linutronix.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Julia Lawall <Julia.Lawall@inria.fr>
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	[thread overview]
Message-ID: <875yfitpdu.ffs@tglx> (raw)
In-Reply-To: <20221110064147.343514404@goodmis.org>

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









  parent reply	other threads:[~2022-11-13 23:18 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-10  6:41 [PATCH v6 0/6] timers: Use timer_shutdown*() before freeing timers Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 1/6] ARM: spear: Do not use timer namespace for timer_shutdown() function Steven Rostedt
2022-11-10  6:41   ` Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 2/6] clocksource/drivers/arm_arch_timer: " Steven Rostedt
2022-11-10  6:41   ` Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 3/6] clocksource/drivers/sp804: " Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called before freeing timers Steven Rostedt
2022-11-13 21:52   ` Thomas Gleixner
2022-11-14  0:11     ` Steven Rostedt
2022-11-14  1:04       ` Thomas Gleixner
2022-11-14 14:08         ` Steven Rostedt
2022-11-14 18:53           ` Thomas Gleixner
2022-11-14 19:14             ` Steven Rostedt
2022-11-13 23:18   ` Thomas Gleixner [this message]
2022-11-14  0:15     ` Steven Rostedt
2022-11-14  0:33       ` Thomas Gleixner
2022-11-14 13:36         ` Steven Rostedt
2022-11-14 19:13           ` Thomas Gleixner
2022-11-14 19:28             ` Steven Rostedt
2022-11-14 19:54               ` Thomas Gleixner
2022-11-14 15:42         ` Thomas Gleixner
2022-11-14 16:04           ` Steven Rostedt
2022-11-14 17:16           ` Linus Torvalds
2022-11-14 17:50             ` Steven Rostedt
2022-11-14 17:54               ` Linus Torvalds
2022-11-14 19:45             ` Thomas Gleixner
2022-11-24 14:15           ` [tip: timers/core] Bluetooth: hci_qca: Fix the teardown problem for real tip-bot2 for Thomas Gleixner
2022-11-10  6:41 ` [PATCH v6 5/6] timers: Add timer_shutdown() to be called before freeing timers Steven Rostedt
2022-11-13 22:20   ` Thomas Gleixner
2022-11-10  6:41 ` [PATCH v6 6/6] timers: Update the documentation to reflect on the new timer_shutdown() API Steven Rostedt
2022-11-24 14:16   ` [tip: timers/core] " tip-bot2 for Steven Rostedt (Google)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875yfitpdu.ffs@tglx \
    --to=tglx@linutronix.de \
    --cc=Julia.Lawall@inria.fr \
    --cc=akpm@linux-foundation.org \
    --cc=anna-maria@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=rostedt@goodmis.org \
    --cc=sboyd@kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.