[RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

[
  This is an RFC patch. As we hit a few bugs were del_timer() is called
  instead of del_timer_sync() before the timer is freed, and there could
  be bugs where even del_timer_sync() is used, but the timer gets rearmed,
  I decided to introduce a "del_timer_free()" function that can be used
  instead. This will at least educate developers on what to call before they
  free a structure that holds a timer.

  In this RFC, I modified hci_qca.c as a use case, even though that change
  needs some work, because the workqueue could still rearm it (I'm looking
  to see if I can trigger the warning).

  If this approach is acceptable, then I will remove the hci_qca.c portion
  from this patch, and create a series of patches to use the
  del_timer_free() in all the locations in the kernel that remove the timer
  before freeing.
]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

We are hitting a common bug were a timer is being triggered after it is
freed. This causes a corruption in the timer link list and crashes the
kernel. Unfortunately it is not easy to know what timer it was that was
freed. Looking at the code, it appears that there are several cases that
del_timer() is used when del_timer_sync() should have been.

Add a del_timer_free() that not only does a del_timer_sync() but will mark
the timer as freed in case it gets rearmed, it will trigger a WARN_ON. The
del_timer_free() is more likely to be used by developers that are about to
free a timer, then using del_timer_sync() as the latter is not as obvious
to being needed for freeing. Having the word "free" in the name of the
function will hopefully help developers know that that function needs to
be called before freeing.

The added bonus is the marking of the timer as being freed such that it
will trigger a warning if it gets rearmed. At least that way if the system
crashes on a freed timer, at least we may see which timer it was that was
freed.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 drivers/bluetooth/hci_qca.c |  4 ++--
 include/linux/timer.h       |  8 ++++---
 kernel/time/timer.c         | 42 +++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f6e91fb432a3..8b3e57fd0f9f 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -696,8 +696,8 @@ static int qca_close(struct hci_uart *hu)
 	skb_queue_purge(&qca->tx_wait_q);
 	skb_queue_purge(&qca->txq);
 	skb_queue_purge(&qca->rx_memdump_q);
-	del_timer(&qca->tx_idle_timer);
-	del_timer(&qca->wake_retrans_timer);
+	del_timer_free(&qca->tx_idle_timer);
+	del_timer_free(&qca->wake_retrans_timer);
 	destroy_workqueue(qca->workqueue);
 	qca->hu = NULL;
 
diff --git a/include/linux/timer.h b/include/linux/timer.h
index fda13c9d1256..cc76ab0659f3 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -67,11 +67,12 @@ struct timer_list {
 #define TIMER_DEFERRABLE	0x00080000
 #define TIMER_PINNED		0x00100000
 #define TIMER_IRQSAFE		0x00200000
+#define TIMER_FREED		0x00400000
 #define TIMER_INIT_FLAGS	(TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE)
-#define TIMER_ARRAYSHIFT	22
-#define TIMER_ARRAYMASK		0xFFC00000
+#define TIMER_ARRAYSHIFT	23
+#define TIMER_ARRAYMASK		0xFF800000
 
-#define TIMER_TRACE_FLAGMASK	(TIMER_MIGRATING | TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE)
+#define TIMER_TRACE_FLAGMASK	(TIMER_MIGRATING | TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE | TIMER_FREED)
 
 #define __TIMER_INITIALIZER(_function, _flags) {		\
 		.entry = { .next = TIMER_ENTRY_STATIC },	\
@@ -170,6 +171,7 @@ static inline int timer_pending(const struct timer_list * timer)
 
 extern void add_timer_on(struct timer_list *timer, int cpu);
 extern int del_timer(struct timer_list * timer);
+extern int del_timer_free(struct timer_list *timer);
 extern int mod_timer(struct timer_list *timer, unsigned long expires);
 extern int mod_timer_pending(struct timer_list *timer, unsigned long expires);
 extern int timer_reduce(struct timer_list *timer, unsigned long expires);
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 85f1021ad459..0477e8237a0a 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -966,6 +966,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option
 
 	BUG_ON(!timer->function);
 
+	WARN_ON(timer->flags & TIMER_FREED);
+
 	/*
 	 * This is a common optimization triggered by the networking code - if
 	 * the timer is re-modified to have the same timeout or ends up in the
@@ -1392,6 +1394,46 @@ int del_timer_sync(struct timer_list *timer)
 EXPORT_SYMBOL(del_timer_sync);
 #endif
 
+/**
+ * del_timer_free - Release the timer for freeing
+ * @timer: the timer to be deactivated for freeing
+ *
+ * This is the same as del_timer_sync() but should be called
+ * instead if the timer is to be freed afterward. If the
+ * timer gets rearmed before freeing, it will trigger a WARN_ON().
+ *
+ * The function returns whether it has deactivated a pending timer or not.
+ */
+int del_timer_free(struct timer_list *timer)
+{
+	struct timer_base *base;
+	unsigned long flags;
+	int ret;
+
+	debug_assert_init(timer);
+
+	for (;;) {
+		ret = -1;
+		base = lock_timer_base(timer, &flags);
+
+		if (base->running_timer != timer)
+			ret = detach_if_pending(timer, base, true);
+
+		if (ret >= 0) {
+			timer->flags |= TIMER_FREED;
+			raw_spin_unlock_irqrestore(&base->lock, flags);
+			break;
+		}
+
+		raw_spin_unlock_irqrestore(&base->lock, flags);
+
+		del_timer_wait_running(timer);
+		cpu_relax();
+	}
+
+	return ret;
+}
+
 static void call_timer_fn(struct timer_list *timer,
 			  void (*fn)(struct timer_list *),
 			  unsigned long baseclk)
-- 
2.35.1

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Guenter Roeck]

Hi Steven,

On Thu, Apr 07, 2022 at 04:17:45PM -0400, Steven Rostedt wrote:
> [
>   This is an RFC patch. As we hit a few bugs were del_timer() is called
>   instead of del_timer_sync() before the timer is freed, and there could
>   be bugs where even del_timer_sync() is used, but the timer gets rearmed,
>   I decided to introduce a "del_timer_free()" function that can be used
>   instead. This will at least educate developers on what to call before they
>   free a structure that holds a timer.
> 
>   In this RFC, I modified hci_qca.c as a use case, even though that change
>   needs some work, because the workqueue could still rearm it (I'm looking
>   to see if I can trigger the warning).
> 
>   If this approach is acceptable, then I will remove the hci_qca.c portion
>   from this patch, and create a series of patches to use the
>   del_timer_free() in all the locations in the kernel that remove the timer
>   before freeing.
> ]
> 
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> We are hitting a common bug were a timer is being triggered after it is
> freed. This causes a corruption in the timer link list and crashes the
> kernel. Unfortunately it is not easy to know what timer it was that was
> freed. Looking at the code, it appears that there are several cases that
> del_timer() is used when del_timer_sync() should have been.
> 
> Add a del_timer_free() that not only does a del_timer_sync() but will mark

This limits the use case to situations where del_timer_sync() can actually
be called. There is, however, code where this is not possible.
Specifically, it doesn't work if the code triggered with the timer uses a
lock, and del_timer() is also called under that same lock. An example for
that is the code in sound/synth/emux/emux.c. How do you suggest to handle
that situation ?

Thanks,
Guenter

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Thu, 7 Apr 2022 14:58:02 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > Add a del_timer_free() that not only does a del_timer_sync() but will mark  
> 
> This limits the use case to situations where del_timer_sync() can actually
> be called. There is, however, code where this is not possible.
> Specifically, it doesn't work if the code triggered with the timer uses a
> lock, and del_timer() is also called under that same lock. An example for
> that is the code in sound/synth/emux/emux.c. How do you suggest to handle
> that situation ?

Easy. Tell me how that situation is not a bug?

That code you point out at emux.c looks extremely buggy as well. In other
words, if you can't call del_timer_free() for the reason you mention above,
the code is very likely to have race conditions. I cannot think of a
situation that it is safe to do this.

In fact, I think that just replacing that with del_timer_free() may be good
enough. At least to show why it blows up later. I'm confused in what they
are doing by taking that lock. I can see:

	CPU1				CPU2
	----				----
				    snd_emux_timer_callback()
    spin_lock(voice_lock)
    if (timer_active)
	del_timer()
    spin_unlock(voice_lock)

				    [..]
				    	do_again++
				    [..]
				    if (do_again) {
					mod_timer()
					timer_active = 1;
				    }

    [..]
    free(emu);


BOOM!!

Hmm, perhaps I should change the code in __mod_timer() to:

	if (WARN_ON(timer->flags & TIMER_FREED))
		return;

To not rearm it.

-- Steve

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Guenter Roeck]

On 4/7/22 15:51, Steven Rostedt wrote:
> On Thu, 7 Apr 2022 14:58:02 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>>> Add a del_timer_free() that not only does a del_timer_sync() but will mark
>>
>> This limits the use case to situations where del_timer_sync() can actually
>> be called. There is, however, code where this is not possible.
>> Specifically, it doesn't work if the code triggered with the timer uses a
>> lock, and del_timer() is also called under that same lock. An example for
>> that is the code in sound/synth/emux/emux.c. How do you suggest to handle
>> that situation ?
> 
> Easy. Tell me how that situation is not a bug?
> 

Sure, fixing the problem is of course the right thing to do. But replacing
del_timer() with your suggested version of del_timer_free() doesn't work
with this code because it would deadlock. Sure, that would not fix the
underlying problem anyway, but that isn't the point I was trying to make:
I think it would be beneficial to be able to replace del_timer() with a
version that can not result in deadlocks but would still identify problems
such as the one in the code in emux.c.

Can we have del_timer_free() and del_timer_sync_free() ? Or am I missing
something and that doesn't really make sense ?

Thanks,
Guenter

> That code you point out at emux.c looks extremely buggy as well. In other
> words, if you can't call del_timer_free() for the reason you mention above,
> the code is very likely to have race conditions. I cannot think of a
> situation that it is safe to do this.
> 
> In fact, I think that just replacing that with del_timer_free() may be good
> enough. At least to show why it blows up later. I'm confused in what they
> are doing by taking that lock. I can see:
> 
> 	CPU1				CPU2
> 	----				----
> 				    snd_emux_timer_callback()
>      spin_lock(voice_lock)
>      if (timer_active)
> 	del_timer()
>      spin_unlock(voice_lock)
> 
> 				    [..]
> 				    	do_again++
> 				    [..]
> 				    if (do_again) {
> 					mod_timer()
> 					timer_active = 1;
> 				    }
> 
>      [..]
>      free(emu);
>  >
> BOOM!!
> 
> Hmm, perhaps I should change the code in __mod_timer() to:
> 
> 	if (WARN_ON(timer->flags & TIMER_FREED))
> 		return;
> 
> To not rearm it.
> 
> -- Steve

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Thu, 7 Apr 2022 17:58:09 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> >>> Add a del_timer_free() that not only does a del_timer_sync() but will mark  
> >>
> >> This limits the use case to situations where del_timer_sync() can actually
> >> be called. There is, however, code where this is not possible.
> >> Specifically, it doesn't work if the code triggered with the timer uses a
> >> lock, and del_timer() is also called under that same lock. An example for
> >> that is the code in sound/synth/emux/emux.c. How do you suggest to handle
> >> that situation ?  
> > 
> > Easy. Tell me how that situation is not a bug?
> >   
> 
> Sure, fixing the problem is of course the right thing to do. But replacing
> del_timer() with your suggested version of del_timer_free() doesn't work

I meant replacing the entire block with del_timer_free().

diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c
index 5ed8e36d2e04..f631e090e074 100644
--- a/sound/synth/emux/emux.c
+++ b/sound/synth/emux/emux.c
@@ -131,10 +131,7 @@ int snd_emux_free(struct snd_emux *emu)
 	if (! emu)
 		return -EINVAL;
 
-	spin_lock_irqsave(&emu->voice_lock, flags);
-	if (emu->timer_active)
-		del_timer(&emu->tlist);
-	spin_unlock_irqrestore(&emu->voice_lock, flags);
+	del_timer_free(&emu->tlist);
 
 	snd_emux_proc_free(emu);
 	snd_emux_delete_virmidi(emu);

It doesn't hurt to delete it if it wasn't queued. I'm not sure what the
dance with spinlocks are all about.

The above may actually be enough. I don't see where the timer could be
enqueued again after that.

That code goes back to original git history, so it was probably trying to
do it's own del_timer_sync() albeit poorly.

> with this code because it would deadlock. Sure, that would not fix the
> underlying problem anyway, but that isn't the point I was trying to make:
> I think it would be beneficial to be able to replace del_timer() with a
> version that can not result in deadlocks but would still identify problems
> such as the one in the code in emux.c.
> 
> Can we have del_timer_free() and del_timer_sync_free() ? Or am I missing
> something and that doesn't really make sense ?

No, that doesn't make sense.

-- Steve

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Thomas Gleixner]

On Thu, Apr 07 2022 at 16:17, Steven Rostedt wrote:
> diff --git a/include/linux/timer.h b/include/linux/timer.h
> index fda13c9d1256..cc76ab0659f3 100644
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h
> @@ -67,11 +67,12 @@ struct timer_list {
>  #define TIMER_DEFERRABLE	0x00080000
>  #define TIMER_PINNED		0x00100000
>  #define TIMER_IRQSAFE		0x00200000
> +#define TIMER_FREED		0x00400000
>  #define TIMER_INIT_FLAGS	(TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE)
> -#define TIMER_ARRAYSHIFT	22
> -#define TIMER_ARRAYMASK		0xFFC00000
> +#define TIMER_ARRAYSHIFT	23
> +#define TIMER_ARRAYMASK		0xFF800000

#define LVL_BITS	6
#define LVL_SIZE	(1UL << LVL_BITS)

#if HZ > 100
# define LVL_DEPTH	9
# else
# define LVL_DEPTH	8
#endif

#define WHEEL_SIZE	(LVL_SIZE * LVL_DEPTH)

WHEEL_SIZE(HZ > 100) = (1 << 6) * 9 = 576
max_index(HZ > 100) = 575 = 0x23f

0x23f << ARRAY_SHIFT = 0x11f800000

Q: Does 0x11f800000 fit into 32bit?
A: No, and we are not making flags 64bit just because.

If you want a bit you have to steal it from the CPUMASK field, but you
could simply set timer->function to NULL, so you can spare extra checks,
no?

> @@ -966,6 +966,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option
>  
>  	BUG_ON(!timer->function);
>  
> +	WARN_ON(timer->flags & TIMER_FREED);

So this would become:

-  	BUG_ON(!timer->function);
+	if (WARN_ON(!timer->function))
+		return -EBROKEN;

You really want to return here and not proceed.

That needs a corresponding change in add_timer_on() plus:

-		fn = timer->function;
+		fn = READ_ONCE(timer->function);

in expire_timers().

> +int del_timer_free(struct timer_list *timer)
> +{
> +	struct timer_base *base;
> +	unsigned long flags;
> +	int ret;
> +
> +	debug_assert_init(timer);
> +
> +	for (;;) {
> +		ret = -1;
> +		base = lock_timer_base(timer, &flags);
> +
> +		if (base->running_timer != timer)
> +			ret = detach_if_pending(timer, base, true);
> +
> +		if (ret >= 0) {
> +			timer->flags |= TIMER_FREED;
> +			raw_spin_unlock_irqrestore(&base->lock, flags);
> +			break;
> +		}
> +
> +		raw_spin_unlock_irqrestore(&base->lock, flags);
> +
> +		del_timer_wait_running(timer);
> +		cpu_relax();
> +	}
> +
> +	return ret;
> +}

So this reimplements del_timer_sync() just slightly different for no
reason. Something like the uncompiled below.

Thanks,

        tglx
---
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -183,12 +183,17 @@ extern int timer_reduce(struct timer_lis
 extern void add_timer(struct timer_list *timer);
 
 extern int try_to_del_timer_sync(struct timer_list *timer);
+extern int __del_timer_sync(struct timer_list *timer, bool free);
 
-#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
-  extern int del_timer_sync(struct timer_list *timer);
-#else
-# define del_timer_sync(t)		del_timer(t)
-#endif
+static inline int del_timer_sync(struct timer_list *timer)
+{
+	return __del_timer_sync(timer, false);
+}
+
+static inline int del_timer_sync_free(struct timer_list *timer)
+{
+	return __del_timer_sync(timer, true);
+}
 
 #define del_singleshot_timer_sync(t) del_timer_sync(t)
 
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -964,7 +964,8 @@ static inline int
 	unsigned int idx = UINT_MAX;
 	int ret = 0;
 
-	BUG_ON(!timer->function);
+	if (WARN_ON(!timer->function))
+		return -EINVAL;
 
 	/*
 	 * This is a common optimization triggered by the networking code - if
@@ -1140,7 +1141,8 @@ EXPORT_SYMBOL(timer_reduce);
  */
 void add_timer(struct timer_list *timer)
 {
-	BUG_ON(timer_pending(timer));
+	if (WARN_ON(timer_pending(timer)))
+		return;
 	__mod_timer(timer, timer->expires, MOD_TIMER_NOTPENDING);
 }
 EXPORT_SYMBOL(add_timer);
@@ -1157,7 +1159,8 @@ void add_timer_on(struct timer_list *tim
 	struct timer_base *new_base, *base;
 	unsigned long flags;
 
-	BUG_ON(timer_pending(timer) || !timer->function);
+	if (WARN_ON(timer_pending(timer) || !timer->function))
+		return;
 
 	new_base = get_timer_cpu_base(timer->flags, cpu);
 
@@ -1213,14 +1216,7 @@ int del_timer(struct timer_list *timer)
 }
 EXPORT_SYMBOL(del_timer);
 
-/**
- * try_to_del_timer_sync - Try to deactivate a timer
- * @timer: timer to delete
- *
- * This function tries to deactivate a timer. Upon successful (ret >= 0)
- * exit the timer is not queued and the handler is not running on any CPU.
- */
-int try_to_del_timer_sync(struct timer_list *timer)
+static int __try_to_del_timer_sync(struct timer_list *timer, boot free)
 {
 	struct timer_base *base;
 	unsigned long flags;
@@ -1232,11 +1228,25 @@ int try_to_del_timer_sync(struct timer_l
 
 	if (base->running_timer != timer)
 		ret = detach_if_pending(timer, base, true);
+	if (free)
+		timer->function = NULL;
 
 	raw_spin_unlock_irqrestore(&base->lock, flags);
 
 	return ret;
 }
+
+/**
+ * try_to_del_timer_sync - Try to deactivate a timer
+ * @timer: timer to delete
+ *
+ * This function tries to deactivate a timer. Upon successful (ret >= 0)
+ * exit the timer is not queued and the handler is not running on any CPU.
+ */
+int try_to_del_timer_sync(struct timer_list *timer)
+{
+	return __try_to_del_timer_sync(timer, false);
+}
 EXPORT_SYMBOL(try_to_del_timer_sync);
 
 #ifdef CONFIG_PREEMPT_RT
@@ -1312,7 +1322,6 @@ static inline void timer_sync_wait_runni
 static inline void del_timer_wait_running(struct timer_list *timer) { }
 #endif
 
-#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
 /**
  * del_timer_sync - deactivate a timer and wait for the handler to finish.
  * @timer: the timer to be deactivated
@@ -1349,7 +1358,7 @@ static inline void del_timer_wait_runnin
  *
  * The function returns whether it has deactivated a pending timer or not.
  */
-int del_timer_sync(struct timer_list *timer)
+int __del_timer_sync(struct timer_list *timer, bool free)
 {
 	int ret;
 
@@ -1379,7 +1388,7 @@ int del_timer_sync(struct timer_list *ti
 		lockdep_assert_preemption_enabled();
 
 	do {
-		ret = try_to_del_timer_sync(timer);
+		ret = __try_to_del_timer_sync(timer, free);
 
 		if (unlikely(ret < 0)) {
 			del_timer_wait_running(timer);
@@ -1389,8 +1398,7 @@ int del_timer_sync(struct timer_list *ti
 
 	return ret;
 }
-EXPORT_SYMBOL(del_timer_sync);
-#endif
+EXPORT_SYMBOL(__del_timer_sync);
 
 static void call_timer_fn(struct timer_list *timer,
 			  void (*fn)(struct timer_list *),

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Fri, 08 Apr 2022 12:37:40 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, Apr 07 2022 at 16:17, Steven Rostedt wrote:
> > diff --git a/include/linux/timer.h b/include/linux/timer.h
> > index fda13c9d1256..cc76ab0659f3 100644
> > --- a/include/linux/timer.h
> > +++ b/include/linux/timer.h
> > @@ -67,11 +67,12 @@ struct timer_list {
> >  #define TIMER_DEFERRABLE	0x00080000
> >  #define TIMER_PINNED		0x00100000
> >  #define TIMER_IRQSAFE		0x00200000
> > +#define TIMER_FREED		0x00400000
> >  #define TIMER_INIT_FLAGS	(TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE)
> > -#define TIMER_ARRAYSHIFT	22
> > -#define TIMER_ARRAYMASK		0xFFC00000
> > +#define TIMER_ARRAYSHIFT	23
> > +#define TIMER_ARRAYMASK		0xFF800000  
> 
> #define LVL_BITS	6
> #define LVL_SIZE	(1UL << LVL_BITS)
> 
> #if HZ > 100
> # define LVL_DEPTH	9
> # else
> # define LVL_DEPTH	8
> #endif
> 
> #define WHEEL_SIZE	(LVL_SIZE * LVL_DEPTH)
> 
> WHEEL_SIZE(HZ > 100) = (1 << 6) * 9 = 576
> max_index(HZ > 100) = 575 = 0x23f
> 
> 0x23f << ARRAY_SHIFT = 0x11f800000
> 
> Q: Does 0x11f800000 fit into 32bit?
> A: No, and we are not making flags 64bit just because.

And this is why it's a RFC patch. ;-)

I figured shrinking that mask was going to break something, but I
wanted to know if this approach was sound before digging further into
making it correct.

> 
> If you want a bit you have to steal it from the CPUMASK field, but you
> could simply set timer->function to NULL, so you can spare extra checks,
> no?


That could work. Thanks.

> 
> > @@ -966,6 +966,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option
> >  
> >  	BUG_ON(!timer->function);
> >  
> > +	WARN_ON(timer->flags & TIMER_FREED);  
> 
> So this would become:
> 
> -  	BUG_ON(!timer->function);
> +	if (WARN_ON(!timer->function))
> +		return -EBROKEN;
> 
> You really want to return here and not proceed.

I mention exactly this later in the thread with my discussions with
Guenter. But it was for the flag check, and not the NULL function.

> 
> That needs a corresponding change in add_timer_on() plus:
> 
> -		fn = timer->function;
> +		fn = READ_ONCE(timer->function);
> 
> in expire_timers().

OK.

> 
> > +int del_timer_free(struct timer_list *timer)
> > +{
> > +	struct timer_base *base;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	debug_assert_init(timer);
> > +
> > +	for (;;) {
> > +		ret = -1;
> > +		base = lock_timer_base(timer, &flags);
> > +
> > +		if (base->running_timer != timer)
> > +			ret = detach_if_pending(timer, base, true);
> > +
> > +		if (ret >= 0) {
> > +			timer->flags |= TIMER_FREED;
> > +			raw_spin_unlock_irqrestore(&base->lock, flags);
> > +			break;
> > +		}
> > +
> > +		raw_spin_unlock_irqrestore(&base->lock, flags);
> > +
> > +		del_timer_wait_running(timer);
> > +		cpu_relax();
> > +	}
> > +
> > +	return ret;
> > +}  
> 
> So this reimplements del_timer_sync() just slightly different for no
> reason. Something like the uncompiled below.

Yeah, I was going to try a helper function for both, but wasn't sure I
would mess up something subtle, so for the RFC patch, I figured I just
implement it completely.

> 
> Thanks,
> 
>         tglx
> ---
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h

I'll take a look at the below later today.

Thanks for the review.

-- Steve

> @@ -183,12 +183,17 @@ extern int timer_reduce(struct timer_lis
>  extern void add_timer(struct timer_list *timer);
>  
>  extern int try_to_del_timer_sync(struct timer_list *timer);
> +extern int __del_timer_sync(struct timer_list *timer, bool free);
>  
> -#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
> -  extern int del_timer_sync(struct timer_list *timer);
> -#else
> -# define del_timer_sync(t)		del_timer(t)
> -#endif
> +static inline int del_timer_sync(struct timer_list *timer)
> +{
> +	return __del_timer_sync(timer, false);
> +}
> +
> +static inline int del_timer_sync_free(struct timer_list *timer)
> +{
> +	return __del_timer_sync(timer, true);
> +}
>  
>  #define del_singleshot_timer_sync(t) del_timer_sync(t)
>  
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -964,7 +964,8 @@ static inline int
>  	unsigned int idx = UINT_MAX;
>  	int ret = 0;
>  
> -	BUG_ON(!timer->function);
> +	if (WARN_ON(!timer->function))
> +		return -EINVAL;
>  
>  	/*
>  	 * This is a common optimization triggered by the networking code - if
> @@ -1140,7 +1141,8 @@ EXPORT_SYMBOL(timer_reduce);
>   */
>  void add_timer(struct timer_list *timer)
>  {
> -	BUG_ON(timer_pending(timer));
> +	if (WARN_ON(timer_pending(timer)))
> +		return;
>  	__mod_timer(timer, timer->expires, MOD_TIMER_NOTPENDING);
>  }
>  EXPORT_SYMBOL(add_timer);
> @@ -1157,7 +1159,8 @@ void add_timer_on(struct timer_list *tim
>  	struct timer_base *new_base, *base;
>  	unsigned long flags;
>  
> -	BUG_ON(timer_pending(timer) || !timer->function);
> +	if (WARN_ON(timer_pending(timer) || !timer->function))
> +		return;
>  
>  	new_base = get_timer_cpu_base(timer->flags, cpu);
>  
> @@ -1213,14 +1216,7 @@ int del_timer(struct timer_list *timer)
>  }
>  EXPORT_SYMBOL(del_timer);
>  
> -/**
> - * try_to_del_timer_sync - Try to deactivate a timer
> - * @timer: timer to delete
> - *
> - * This function tries to deactivate a timer. Upon successful (ret >= 0)
> - * exit the timer is not queued and the handler is not running on any CPU.
> - */
> -int try_to_del_timer_sync(struct timer_list *timer)
> +static int __try_to_del_timer_sync(struct timer_list *timer, boot free)
>  {
>  	struct timer_base *base;
>  	unsigned long flags;
> @@ -1232,11 +1228,25 @@ int try_to_del_timer_sync(struct timer_l
>  
>  	if (base->running_timer != timer)
>  		ret = detach_if_pending(timer, base, true);
> +	if (free)
> +		timer->function = NULL;
>  
>  	raw_spin_unlock_irqrestore(&base->lock, flags);
>  
>  	return ret;
>  }
> +
> +/**
> + * try_to_del_timer_sync - Try to deactivate a timer
> + * @timer: timer to delete
> + *
> + * This function tries to deactivate a timer. Upon successful (ret >= 0)
> + * exit the timer is not queued and the handler is not running on any CPU.
> + */
> +int try_to_del_timer_sync(struct timer_list *timer)
> +{
> +	return __try_to_del_timer_sync(timer, false);
> +}
>  EXPORT_SYMBOL(try_to_del_timer_sync);
>  
>  #ifdef CONFIG_PREEMPT_RT
> @@ -1312,7 +1322,6 @@ static inline void timer_sync_wait_runni
>  static inline void del_timer_wait_running(struct timer_list *timer) { }
>  #endif
>  
> -#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
>  /**
>   * del_timer_sync - deactivate a timer and wait for the handler to finish.
>   * @timer: the timer to be deactivated
> @@ -1349,7 +1358,7 @@ static inline void del_timer_wait_runnin
>   *
>   * The function returns whether it has deactivated a pending timer or not.
>   */
> -int del_timer_sync(struct timer_list *timer)
> +int __del_timer_sync(struct timer_list *timer, bool free)
>  {
>  	int ret;
>  
> @@ -1379,7 +1388,7 @@ int del_timer_sync(struct timer_list *ti
>  		lockdep_assert_preemption_enabled();
>  
>  	do {
> -		ret = try_to_del_timer_sync(timer);
> +		ret = __try_to_del_timer_sync(timer, free);
>  
>  		if (unlikely(ret < 0)) {
>  			del_timer_wait_running(timer);
> @@ -1389,8 +1398,7 @@ int del_timer_sync(struct timer_list *ti
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL(del_timer_sync);
> -#endif
> +EXPORT_SYMBOL(__del_timer_sync);
>  
>  static void call_timer_fn(struct timer_list *timer,
>  			  void (*fn)(struct timer_list *),

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Fri, 08 Apr 2022 12:37:40 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> +++ b/include/linux/timer.h
> @@ -183,12 +183,17 @@ extern int timer_reduce(struct timer_lis
>  extern void add_timer(struct timer_list *timer);
>  
>  extern int try_to_del_timer_sync(struct timer_list *timer);
> +extern int __del_timer_sync(struct timer_list *timer, bool free);

Do we really want to expose this to all the kernel?

That is, we do not need to make the static inlines in the header, but
instead do the split in the timer.c file. Not to mention, why have the
"free" parameter be created by the callers? It duplicates the work of
updating the second parameter around the kernel, instead of just in one
place.

One concern I have is that I wanted to keep the "free" version right next
to the del_timer() prototype. That way it becomes more visible to users and
they will be more likely to see it. I'm wondering if some of the issue
with not using del_timer_sync() is because it is not next to the other
prototypes, and people may have missed it.

>  
> -#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)

Is the above not important for del_timer_sync() anymore?

Can we just have a del_timer_sync() prototype and not have this macro logic
anymore?

Anyway, I'll take this code and make my updates and send out a v2.

Thanks,

-- Steve

> -  extern int del_timer_sync(struct timer_list *timer);
> -#else
> -# define del_timer_sync(t)		del_timer(t)
> -#endif
> +static inline int del_timer_sync(struct timer_list *timer)
> +{
> +	return __del_timer_sync(timer, false);
> +}
> +
> +static inline int del_timer_sync_free(struct timer_list *timer)
> +{
> +	return __del_timer_sync(timer, true);
> +}
>  
>  #define del_singleshot_timer_sync(t) del_timer_sync(t)

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Linus Torvalds]

On Fri, Apr 8, 2022 at 12:37 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> So this would become:
>
> -       BUG_ON(!timer->function);
> +       if (WARN_ON(!timer->function))
> +               return -EBROKEN;

Yes. But please make it a WARN_ON_ONCE(), just on basic principles. I
can't imagine this happening a lot, but at the same time I don't think
there's any reason _not_ to just always use WARN_ON_ONCE() for these
kinds of "serious bug, but should never happen" situations.

Because we don't want some "user can trigger this and spam the logs"
situation either.

That said, I would actually prefer a name-change: instead of making
this about "del_timer_free()", can we please just make this about it
being "final". Or maybe "del_timer_cancel()" or something like that?

Because the actual _freeing_ will obviously happen later, and the
function does nothing of the sort. In fact, there may be situations
where you don't free it at all, but just want to be in the situation
where you want to make sure there are no pending timers until after
you explicitly re-arm it, even if the timer would otherwise be
self-arming.

(That use-case would actually mean removing the WARN_ON_ONCE(), but I
think that would be a "future use" issue, I'm *not* suggesting it not
be done initially).

I also suspect that 99% of all del_timer_sync() users actually want
that kind of explicit "del_timer_final()" behavior. Some may not
_need_ it (because their re-arming already checks for "have I shut
down?"), but I have this suspicion that we could convert a lot - maybe
all - of the current del_timer_sync() users to this and try to see if
we could just make it the rule.

And then we could actually maybe start removing the explicit "shut
down timer" code. See for example "work->canceling" logic for
kthreads, which now does double duty (it disables re-arming the timer,
_and_ it does some "double cancel work avoidance")

              Linus

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Fri, 8 Apr 2022 07:33:53 -1000
Linus Torvalds <torvalds@linux-foundation.org> wrote:


> That said, I would actually prefer a name-change: instead of making
> this about "del_timer_free()", can we please just make this about it
> being "final". Or maybe "del_timer_cancel()" or something like that?
> 
> Because the actual _freeing_ will obviously happen later, and the
> function does nothing of the sort. In fact, there may be situations

I was originally thinking of calling it "del_timer_prepare_free()"

> where you don't free it at all, but just want to be in the situation
> where you want to make sure there are no pending timers until after
> you explicitly re-arm it, even if the timer would otherwise be
> self-arming.

We have that already, it's called "del_timer_sync()". And that's not used
when it should be, and does not catch the case of the timer being rearmed.

The idea of "del_timer_free()" or perhaps "del_timer_sync_terminate()?" is
that once called you will NEVER arm it again. And if you do, it's a bug.

> 
> (That use-case would actually mean removing the WARN_ON_ONCE(), but I
> think that would be a "future use" issue, I'm *not* suggesting it not
> be done initially).

The point of this call is to be used when and only when the timer is about
to be freed. Not to just say "wait till it's done". Because we are hitting
a lot of bugs where the system crashes in the timer code while walking
through the link list of timers where one of the pending timers has been
freed, and we have no idea what timer that was.

Once this API is added, I would go around and add it to all locations that
del_timer(_sync) just before freeing it and call this instead. We already
found a few cases that there's a race after the del_timer_sync() that could
actually rearm the timer before it gets freed. This function would trigger
the WARN_ON(_ONCE).

> 
> I also suspect that 99% of all del_timer_sync() users actually want
> that kind of explicit "del_timer_final()" behavior. Some may not
> _need_ it (because their re-arming already checks for "have I shut
> down?"), but I have this suspicion that we could convert a lot - maybe
> all - of the current del_timer_sync() users to this and try to see if
> we could just make it the rule.

We did find places that use del_timer_sync() that would later legitimately
rearm it. But I agree. Most would be del_timer_final() (or whatever).

> 
> And then we could actually maybe start removing the explicit "shut
> down timer" code. See for example "work->canceling" logic for
> kthreads, which now does double duty (it disables re-arming the timer,
> _and_ it does some "double cancel work avoidance")

Yeah, that's the case we have with the hci_qca.c code that we have right
now. It has a timer where it can be rearmed in the work queue, and the
timer can wake the workqueue. This requires one of those dances to be able
to stop both and prevent one from enabling the other.

Hmm, I guess if we do remove the WARN_ON_ONCE(), and just not enable it
after del_timer_terminate/final() is called then it would remove this race.
You could terminate the timers and then destroy the work queue, and if
there's a pending work happening, you do not have to worry about it
rearming the timers, because they would be shut off, and the mod_timer()
would just return without issue.

Maybe add an API to allow both? One that will cause the warn on, as in (if
anything calls this again, warn about it), and the other that has it not
warn, but just be ignored.

We could differentiate between the two by making a stub function for the
ignore one.

On the warn case:

	timer->fn = NULL;

for the non warn case:

 	timer->fn = timer_null();

and have:

	if (WARN_ON_ONCE(!timer->fn) || unlikely(timer->fn == timer_null))
		return;

del_timer_final();	// warn
del_timer_final_sync(); // no warn

  or

del_timer_sync_final(); // warn
del_timer_sync_final_nowarn(); // no warn

??

[ Let the bike-shedding begin! ]


-- Steve


-- Steve

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Fri, 8 Apr 2022 16:10:25 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> del_timer_sync_final(); // warn
> del_timer_sync_final_nowarn(); // no warn

Or just:

int del_timer_sync_final(struct timer_list *timer, bool warn)

-- Steve

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Thomas Gleixner]

On Fri, Apr 08 2022 at 07:33, Linus Torvalds wrote:
> On Fri, Apr 8, 2022 at 12:37 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> So this would become:
>>
>> -       BUG_ON(!timer->function);
>> +       if (WARN_ON(!timer->function))
>> +               return -EBROKEN;
>
> Yes. But please make it a WARN_ON_ONCE(), just on basic principles. I
> can't imagine this happening a lot, but at the same time I don't think
> there's any reason _not_ to just always use WARN_ON_ONCE() for these
> kinds of "serious bug, but should never happen" situations.
>
> Because we don't want some "user can trigger this and spam the logs"
> situation either.

Fair enough.

> That said, I would actually prefer a name-change: instead of making
> this about "del_timer_free()", can we please just make this about it
> being "final". Or maybe "del_timer_cancel()" or something like that?

I was anyway thinking to use the timer_* namespace if we want to go for
this.

timer_shutdown() perhaps?

> Because the actual _freeing_ will obviously happen later, and the
> function does nothing of the sort. In fact, there may be situations
> where you don't free it at all, but just want to be in the situation
> where you want to make sure there are no pending timers until after
> you explicitly re-arm it, even if the timer would otherwise be
> self-arming.
> 
> (That use-case would actually mean removing the WARN_ON_ONCE(), but I
> think that would be a "future use" issue, I'm *not* suggesting it not
> be done initially).

Well, you'd have to reinitialize it first before the explicit rearm
because the shutdown cleared the function pointer or if we use a flag
then the flag would prevent arming it again.

> I also suspect that 99% of all del_timer_sync() users actually want
> that kind of explicit "del_timer_final()" behavior. Some may not
> _need_ it (because their re-arming already checks for "have I shut
> down?"), but I have this suspicion that we could convert a lot - maybe
> all - of the current del_timer_sync() users to this and try to see if
> we could just make it the rule.

Hmm. That would mean, that we still check the function pointer for NULL
without warning and just return. That would indeed be a good argument
for not having the warning at all.

But, there is a twist. While most callsites ignore the return value of
mod_timer() there are 39 (about 2%) which actually care. So that needs
some thought. Though code which cares about these details is mostly
networking core code and not the random driver thing. Though there are a
few suspects in drivers too including bluetooth, but the latter is what
started this whole discussion. See below.

> And then we could actually maybe start removing the explicit "shut
> down timer" code. See for example "work->canceling" logic for
> kthreads, which now does double duty (it disables re-arming the timer,
> _and_ it does some "double cancel work avoidance")

This serializes the cancel against _any_ queuing of that work including
the queueing from an already running timer callback which is blocked on
the worker lock. The shutdown thing wont help there I think.

The problem where it could help is shutdown code for timers plus other
entities with circular dependencies. The problem which started this was
some bluetooth thing which has two timers and a workqueue. The timers
can queue work and the workqueue can arm timers....

So well written drivers have a priv->shutdown flag which makes timer
callbacks and workqueue functions aware that a shutdown is in progress
so they can take appropriate action. That's not necessarily trivial and
I've decoded my share of subtle problems in that realm over the years.

The bluetooth thing in question does not fall into that category, it
even just used del_timer() before destroying the work queue.

What a shutdown function would prevent here is UAF, but I'm not entirely
sure whether it will simplify coordinated shutdown and remove the
requirement of a priv->shutdown flag all over the place. It might make
some of the driver muck just get stuck in the shutdown, but that's
definitely an improvement over a potential UAF which happens every blue
moons.

Thanks,

        tglx

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Fri, 08 Apr 2022 22:29:58 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> What a shutdown function would prevent here is UAF, but I'm not entirely
> sure whether it will simplify coordinated shutdown and remove the
> requirement of a priv->shutdown flag all over the place. It might make
> some of the driver muck just get stuck in the shutdown, but that's
> definitely an improvement over a potential UAF which happens every blue
> moons.

Note, it is the cause of a large percentage of crash reports reported by
ChromeOS.

And we do not even know if it was this bluetooth issue that caused them.
There's evidence they are mostly caused by the wifi code. I only used the
bluetooth issue because it was the first one we found that looked obviously
wrong.

-- Steve

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Thomas Gleixner]

On Fri, Apr 08 2022 at 16:58, Steven Rostedt wrote:

> On Fri, 08 Apr 2022 22:29:58 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> What a shutdown function would prevent here is UAF, but I'm not entirely
>> sure whether it will simplify coordinated shutdown and remove the
>> requirement of a priv->shutdown flag all over the place. It might make
>> some of the driver muck just get stuck in the shutdown, but that's
>> definitely an improvement over a potential UAF which happens every blue
>> moons.
>
> Note, it is the cause of a large percentage of crash reports reported by
> ChromeOS.
>
> And we do not even know if it was this bluetooth issue that caused them.
> There's evidence they are mostly caused by the wifi code. I only used the
> bluetooth issue because it was the first one we found that looked obviously
> wrong.

I'm sure that there are hundres more...

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Fri, 08 Apr 2022 23:46:34 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> I'm sure that there are hundres more...

Hence why we want to add something that will catch it or make it less
likely to happen.

-- Steve

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Linus Torvalds]

On Fri, Apr 8, 2022 at 10:10 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> We have that already, it's called "del_timer_sync()". And that's not used
> when it should be, and does not catch the case of the timer being rearmed.

THAT'S MY POINT.

It doesn't do the "no rearming".

So my point is that people who actually use "del_timer_sync()"
generally *want* that "no re-arming", and right now they go to extra
work to do so.

Because calling del_timer_sync() while the timer can get re-armed is
almost always a bug _anyway_.

So :

 - don't use the name "free" - because that's not what people always are about

 - aim to *replace* the current del_timer_sync() with the new functionality

A new name is good. But "free" is wrong. The function doesn't free
anything, and it's not necessarily even the case that all users would
want to free it.

I do like the "timer_shutdown()" naming that Thomas suggested.

Imagine having something that just guarantees that the timer isn't
running, and you have to explicitly restart it.

No, del_timer_sync() doesn't do that. It only guarantees that any
previously started timer has been finished.

And no, currently we don't actually have a "timer_restart()". You have
to re-init it completely with timer_setup(), and then do a
"mod_timer()". But that's because we haven't had that
"timer_shutdown()" functionality in the past.

Now, most timers don't necessarily re-arm themselves at all, so those
people don't care, and del_timer_sync() works for them as-is.

 But even if you don't have a self-rearming timer, maybe you want to
make sure nobody else restarts it either.

As mentioned, right now people actually do things like this manually
using some flag of their own.

               Linus

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Fri, 08 Apr 2022 22:29:58 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> Well, you'd have to reinitialize it first before the explicit rearm
> because the shutdown cleared the function pointer or if we use a flag
> then the flag would prevent arming it again.

We could always use the LSB bit of the function as a "shutdown" flag, where:

timer_shutdown() {
	[..]
	timer->fn = (void *)((unsigned long)timer->fn | 1);
	[..]
}

timer_rearm() {
	[..]
	timer->fn = (void *)((unsigned long)timer->fn & ~1UL);
	[..]
}

mod_timer() {

	if ((unsigned long)timer->fn & 1)
		return -EBUSY?;

	[..]
}

-- Steve

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Linus Torvalds]

On Fri, Apr 8, 2022 at 2:22 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> We could always use the LSB bit of the function as a "shutdown" flag, where:

While we do that for data pointers, doing it for function pointers is dodgy.

Not all architectures necessarily align code pointers. In fact, I
think that "-Os" on x86 makes all code alignment go away, and so
function pointers have no alignment at all.

                 Linus

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Fri, 8 Apr 2022 14:30:21 -1000
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Apr 8, 2022 at 2:22 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > We could always use the LSB bit of the function as a "shutdown" flag, where:  
> 
> While we do that for data pointers, doing it for function pointers is dodgy.
> 
> Not all architectures necessarily align code pointers. In fact, I
> think that "-Os" on x86 makes all code alignment go away, and so
> function pointers have no alignment at all.

Hmm, well, I'm not sure it would work for all architectures, but what
about the MSB?  Setting it to zero on "shutdown"?

-- Steve

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Linus Torvalds]

On Fri, Apr 8, 2022 at 2:49 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Hmm, well, I'm not sure it would work for all architectures, but what
> about the MSB?  Setting it to zero on "shutdown"?

Let's just clear the whole thing for now. We don't actually _have_ any
timer_restart() cases yet.

I was more thinking that we might have situations where "I don't want
to race with timers, but I also don't want to take an interrupt-safe
lock" makes a lot of sense.

Most people most definitely are just about "module unload" and similar
issues, where it goes along with doing "task_work_cancel()" and
friends.

I do wonder if we want some way to shut down new timers that doesn't
actually wait for old ones to finish.

We've had issues with some code not being able to use del_timer_sync()
simply because they hold locks that could deadlock with any "wait for
running timer" situation.

Those places couldn't use a synchronous cancel operation either, for
the same reason.

I'm not sure a "make sure no future timers can start" operation is
sensible on its own, though. I can't think of a situation where that
wouldn't also need that "wait for existing ones to finish".

                       Linus

Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Steven Rostedt]

On Fri, 8 Apr 2022 15:00:43 -1000
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Apr 8, 2022 at 2:49 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Hmm, well, I'm not sure it would work for all architectures, but what
> > about the MSB?  Setting it to zero on "shutdown"?  
> 
> Let's just clear the whole thing for now. We don't actually _have_ any
> timer_restart() cases yet.

OK, so this has gone toward the handling all sorts of situations tangent.

Thus, I want to get back to the current situation at hand. We have a
bunch of places that use del_timer(), and possibly del_timer_sync() but
can then have it rearm, and then the timer gets freed and BOOM we get a
crash in the timer code. Worse yet, we have no idea what timer it was
that did the UAF.

So, we could just add that "timer_shutdown()" function that clears the
function and mod_timer() would no longer rearm it. It would also need
to do the synchronization as well. Which means it can't be called with
locks that might be taken in the timer itself.

We can look into more elaborate APIs if we want to help fix other
issues later, but for now, it would be nice to go audit the kernel for
all locations that do a del_timer(_sync) followed by freeing the timer,
and replace it with a timer_shutdown() call.

For the del_timer() cases, we will have to make sure it's not done that
way due to locking. But they will still need to be dealt with because
they are still prone to UAF.

-- Steve

[tip: timers/core] timers: Provide timer_shutdown[_sync]()

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     f571faf6e443b6011ccb585d57866177af1f643c
Gitweb:        https://git.kernel.org/tip/f571faf6e443b6011ccb585d57866177af1f643c
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 23 Nov 2022 21:18:53 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 24 Nov 2022 15:09:12 +01:00

timers: Provide timer_shutdown[_sync]()

Tearing down timers which have circular dependencies to other
functionality, e.g. workqueues, where the timer can schedule work and work
can arm timers, is not trivial.

In those cases it is desired to shutdown the timer in a way which prevents
rearming of the timer. The mechanism to do so is to set timer->function to
NULL and use this as an indicator for the timer arming functions to ignore
the (re)arm request.

Expose new interfaces for this: timer_shutdown_sync() and timer_shutdown().

timer_shutdown_sync() has the same functionality as timer_delete_sync()
plus the NULL-ification of the timer function.

timer_shutdown() has the same functionality as timer_delete() plus the
NULL-ification of the timer function.

In both cases the rearming of the timer is prevented by silently discarding
rearm attempts due to timer->function being NULL.

Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home
Link: https://lore.kernel.org/all/20221110064101.429013735@goodmis.org
Link: https://lore.kernel.org/r/20221123201625.314230270@linutronix.de

---
 include/linux/timer.h |  2 +-
 kernel/time/timer.c   | 66 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+)

diff --git a/include/linux/timer.h b/include/linux/timer.h
index e338e17..9162f27 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -184,6 +184,8 @@ extern void add_timer(struct timer_list *timer);
 extern int try_to_del_timer_sync(struct timer_list *timer);
 extern int timer_delete_sync(struct timer_list *timer);
 extern int timer_delete(struct timer_list *timer);
+extern int timer_shutdown_sync(struct timer_list *timer);
+extern int timer_shutdown(struct timer_list *timer);
 
 /**
  * del_timer_sync - Delete a pending timer and wait for a running callback
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 167e43c..63a8ce7 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1363,6 +1363,27 @@ int timer_delete(struct timer_list *timer)
 EXPORT_SYMBOL(timer_delete);
 
 /**
+ * timer_shutdown - Deactivate a timer and prevent rearming
+ * @timer:	The timer to be deactivated
+ *
+ * The function does not wait for an eventually running timer callback on a
+ * different CPU but it prevents rearming of the timer. Any attempt to arm
+ * @timer after this function returns will be silently ignored.
+ *
+ * This function is useful for teardown code and should only be used when
+ * timer_shutdown_sync() cannot be invoked due to locking or context constraints.
+ *
+ * Return:
+ * * %0 - The timer was not pending
+ * * %1 - The timer was pending
+ */
+int timer_shutdown(struct timer_list *timer)
+{
+	return __timer_delete(timer, true);
+}
+EXPORT_SYMBOL_GPL(timer_shutdown);
+
+/**
  * __try_to_del_timer_sync - Internal function: Try to deactivate a timer
  * @timer:	Timer to deactivate
  * @shutdown:	If true, this indicates that the timer is about to be
@@ -1595,6 +1616,9 @@ static int __timer_delete_sync(struct timer_list *timer, bool shutdown)
  * lock. If there is the possibility of a concurrent rearm then the return
  * value of the function is meaningless.
  *
+ * If such a guarantee is needed, e.g. for teardown situations then use
+ * timer_shutdown_sync() instead.
+ *
  * Return:
  * * %0	- The timer was not pending
  * * %1	- The timer was pending and deactivated
@@ -1605,6 +1629,48 @@ int timer_delete_sync(struct timer_list *timer)
 }
 EXPORT_SYMBOL(timer_delete_sync);
 
+/**
+ * timer_shutdown_sync - Shutdown a timer and prevent rearming
+ * @timer: The timer to be shutdown
+ *
+ * When the function returns it is guaranteed that:
+ *   - @timer is not queued
+ *   - The callback function of @timer is not running
+ *   - @timer cannot be enqueued again. Any attempt to rearm
+ *     @timer is silently ignored.
+ *
+ * See timer_delete_sync() for synchronization rules.
+ *
+ * This function is useful for final teardown of an infrastructure where
+ * the timer is subject to a circular dependency problem.
+ *
+ * A common pattern for this is a timer and a workqueue where the timer can
+ * schedule work and work can arm the timer. On shutdown the workqueue must
+ * be destroyed and the timer must be prevented from rearming. Unless the
+ * code has conditionals like 'if (mything->in_shutdown)' to prevent that
+ * there is no way to get this correct with timer_delete_sync().
+ *
+ * timer_shutdown_sync() is solving the problem. The correct ordering of
+ * calls in this case is:
+ *
+ *	timer_shutdown_sync(&mything->timer);
+ *	workqueue_destroy(&mything->workqueue);
+ *
+ * After this 'mything' can be safely freed.
+ *
+ * This obviously implies that the timer is not required to be functional
+ * for the rest of the shutdown operation.
+ *
+ * Return:
+ * * %0 - The timer was not pending
+ * * %1 - The timer was pending
+ */
+int timer_shutdown_sync(struct timer_list *timer)
+{
+	return __timer_delete_sync(timer, true);
+}
+EXPORT_SYMBOL_GPL(timer_shutdown_sync);
+
 static void call_timer_fn(struct timer_list *timer,
 			  void (*fn)(struct timer_list *),
 			  unsigned long baseclk)

[tip: timers/core] timers: Add shutdown mechanism to the internal functions

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     0cc04e80458a822300b93f82ed861a513edde194
Gitweb:        https://git.kernel.org/tip/0cc04e80458a822300b93f82ed861a513edde194
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 23 Nov 2022 21:18:52 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 24 Nov 2022 15:09:12 +01:00

timers: Add shutdown mechanism to the internal functions

Tearing down timers which have circular dependencies to other
functionality, e.g. workqueues, where the timer can schedule work and work
can arm timers, is not trivial.

In those cases it is desired to shutdown the timer in a way which prevents
rearming of the timer. The mechanism to do so is to set timer->function to
NULL and use this as an indicator for the timer arming functions to ignore
the (re)arm request.

Add a shutdown argument to the relevant internal functions which makes the
actual deactivation code set timer->function to NULL which in turn prevents
rearming of the timer.

Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home
Link: https://lore.kernel.org/all/20221110064101.429013735@goodmis.org
Link: https://lore.kernel.org/r/20221123201625.253883224@linutronix.de

---
 kernel/time/timer.c | 62 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 8 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index e635bb5..167e43c 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1300,12 +1300,19 @@ EXPORT_SYMBOL_GPL(add_timer_on);
 /**
  * __timer_delete - Internal function: Deactivate a timer
  * @timer:	The timer to be deactivated
+ * @shutdown:	If true, this indicates that the timer is about to be
+ *		shutdown permanently.
+ *
+ * If @shutdown is true then @timer->function is set to NULL under the
+ * timer base lock which prevents further rearming of the time. In that
+ * case any attempt to rearm @timer after this function returns will be
+ * silently ignored.
  *
  * Return:
  * * %0 - The timer was not pending
  * * %1 - The timer was pending and deactivated
  */
-static int __timer_delete(struct timer_list *timer)
+static int __timer_delete(struct timer_list *timer, bool shutdown)
 {
 	struct timer_base *base;
 	unsigned long flags;
@@ -1313,9 +1320,22 @@ static int __timer_delete(struct timer_list *timer)
 
 	debug_assert_init(timer);
 
-	if (timer_pending(timer)) {
+	/*
+	 * If @shutdown is set then the lock has to be taken whether the
+	 * timer is pending or not to protect against a concurrent rearm
+	 * which might hit between the lockless pending check and the lock
+	 * aquisition. By taking the lock it is ensured that such a newly
+	 * enqueued timer is dequeued and cannot end up with
+	 * timer->function == NULL in the expiry code.
+	 *
+	 * If timer->function is currently executed, then this makes sure
+	 * that the callback cannot requeue the timer.
+	 */
+	if (timer_pending(timer) || shutdown) {
 		base = lock_timer_base(timer, &flags);
 		ret = detach_if_pending(timer, base, true);
+		if (shutdown)
+			timer->function = NULL;
 		raw_spin_unlock_irqrestore(&base->lock, flags);
 	}
 
@@ -1338,20 +1358,31 @@ static int __timer_delete(struct timer_list *timer)
  */
 int timer_delete(struct timer_list *timer)
 {
-	return __timer_delete(timer);
+	return __timer_delete(timer, false);
 }
 EXPORT_SYMBOL(timer_delete);
 
 /**
  * __try_to_del_timer_sync - Internal function: Try to deactivate a timer
  * @timer:	Timer to deactivate
+ * @shutdown:	If true, this indicates that the timer is about to be
+ *		shutdown permanently.
+ *
+ * If @shutdown is true then @timer->function is set to NULL under the
+ * timer base lock which prevents further rearming of the timer. Any
+ * attempt to rearm @timer after this function returns will be silently
+ * ignored.
+ *
+ * This function cannot guarantee that the timer cannot be rearmed
+ * right after dropping the base lock if @shutdown is false. That
+ * needs to be prevented by the calling code if necessary.
  *
  * Return:
  * * %0  - The timer was not pending
  * * %1  - The timer was pending and deactivated
  * * %-1 - The timer callback function is running on a different CPU
  */
-static int __try_to_del_timer_sync(struct timer_list *timer)
+static int __try_to_del_timer_sync(struct timer_list *timer, bool shutdown)
 {
 	struct timer_base *base;
 	unsigned long flags;
@@ -1363,6 +1394,8 @@ static int __try_to_del_timer_sync(struct timer_list *timer)
 
 	if (base->running_timer != timer)
 		ret = detach_if_pending(timer, base, true);
+	if (shutdown)
+		timer->function = NULL;
 
 	raw_spin_unlock_irqrestore(&base->lock, flags);
 
@@ -1387,7 +1420,7 @@ static int __try_to_del_timer_sync(struct timer_list *timer)
  */
 int try_to_del_timer_sync(struct timer_list *timer)
 {
-	return __try_to_del_timer_sync(timer);
+	return __try_to_del_timer_sync(timer, false);
 }
 EXPORT_SYMBOL(try_to_del_timer_sync);
 
@@ -1468,12 +1501,25 @@ static inline void del_timer_wait_running(struct timer_list *timer) { }
  * __timer_delete_sync - Internal function: Deactivate a timer and wait
  *			 for the handler to finish.
  * @timer:	The timer to be deactivated
+ * @shutdown:	If true, @timer->function will be set to NULL under the
+ *		timer base lock which prevents rearming of @timer
+ *
+ * If @shutdown is not set the timer can be rearmed later. If the timer can
+ * be rearmed concurrently, i.e. after dropping the base lock then the
+ * return value is meaningless.
+ *
+ * If @shutdown is set then @timer->function is set to NULL under timer
+ * base lock which prevents rearming of the timer. Any attempt to rearm
+ * a shutdown timer is silently ignored.
+ *
+ * If the timer should be reused after shutdown it has to be initialized
+ * again.
  *
  * Return:
  * * %0	- The timer was not pending
  * * %1	- The timer was pending and deactivated
  */
-static int __timer_delete_sync(struct timer_list *timer)
+static int __timer_delete_sync(struct timer_list *timer, bool shutdown)
 {
 	int ret;
 
@@ -1503,7 +1549,7 @@ static int __timer_delete_sync(struct timer_list *timer)
 		lockdep_assert_preemption_enabled();
 
 	do {
-		ret = __try_to_del_timer_sync(timer);
+		ret = __try_to_del_timer_sync(timer, shutdown);
 
 		if (unlikely(ret < 0)) {
 			del_timer_wait_running(timer);
@@ -1555,7 +1601,7 @@ static int __timer_delete_sync(struct timer_list *timer)
  */
 int timer_delete_sync(struct timer_list *timer)
 {
-	return __timer_delete_sync(timer);
+	return __timer_delete_sync(timer, false);
 }
 EXPORT_SYMBOL(timer_delete_sync);
 

[tip: timers/core] timers: Split [try_to_]del_timer[_sync]() to prepare for shutdown mode

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     8553b5f2774a66b1f293b7d783934210afb8f23c
Gitweb:        https://git.kernel.org/tip/8553b5f2774a66b1f293b7d783934210afb8f23c
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 23 Nov 2022 21:18:50 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 24 Nov 2022 15:09:12 +01:00

timers: Split [try_to_]del_timer[_sync]() to prepare for shutdown mode

Tearing down timers which have circular dependencies to other
functionality, e.g. workqueues, where the timer can schedule work and work
can arm timers, is not trivial.

In those cases it is desired to shutdown the timer in a way which prevents
rearming of the timer. The mechanism to do so is to set timer->function to
NULL and use this as an indicator for the timer arming functions to ignore
the (re)arm request.

Split the inner workings of try_do_del_timer_sync(), del_timer_sync() and
del_timer() into helper functions to prepare for implementing the shutdown
functionality.

No functional change.

Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home
Link: https://lore.kernel.org/all/20221110064101.429013735@goodmis.org
Link: https://lore.kernel.org/r/20221123201625.195147423@linutronix.de

---
 kernel/time/timer.c | 143 +++++++++++++++++++++++++++----------------
 1 file changed, 92 insertions(+), 51 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index e4fcf56..e635bb5 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1298,20 +1298,14 @@ out_unlock:
 EXPORT_SYMBOL_GPL(add_timer_on);
 
 /**
- * timer_delete - Deactivate a timer
+ * __timer_delete - Internal function: Deactivate a timer
  * @timer:	The timer to be deactivated
  *
- * The function only deactivates a pending timer, but contrary to
- * timer_delete_sync() it does not take into account whether the timer's
- * callback function is concurrently executed on a different CPU or not.
- * It neither prevents rearming of the timer. If @timer can be rearmed
- * concurrently then the return value of this function is meaningless.
- *
  * Return:
  * * %0 - The timer was not pending
  * * %1 - The timer was pending and deactivated
  */
-int timer_delete(struct timer_list *timer)
+static int __timer_delete(struct timer_list *timer)
 {
 	struct timer_base *base;
 	unsigned long flags;
@@ -1327,25 +1321,37 @@ int timer_delete(struct timer_list *timer)
 
 	return ret;
 }
-EXPORT_SYMBOL(timer_delete);
 
 /**
- * try_to_del_timer_sync - Try to deactivate a timer
- * @timer:	Timer to deactivate
+ * timer_delete - Deactivate a timer
+ * @timer:	The timer to be deactivated
  *
- * This function tries to deactivate a timer. On success the timer is not
- * queued and the timer callback function is not running on any CPU.
+ * The function only deactivates a pending timer, but contrary to
+ * timer_delete_sync() it does not take into account whether the timer's
+ * callback function is concurrently executed on a different CPU or not.
+ * It neither prevents rearming of the timer.  If @timer can be rearmed
+ * concurrently then the return value of this function is meaningless.
  *
- * This function does not guarantee that the timer cannot be rearmed right
- * after dropping the base lock. That needs to be prevented by the calling
- * code if necessary.
+ * Return:
+ * * %0 - The timer was not pending
+ * * %1 - The timer was pending and deactivated
+ */
+int timer_delete(struct timer_list *timer)
+{
+	return __timer_delete(timer);
+}
+EXPORT_SYMBOL(timer_delete);
+
+/**
+ * __try_to_del_timer_sync - Internal function: Try to deactivate a timer
+ * @timer:	Timer to deactivate
  *
  * Return:
  * * %0  - The timer was not pending
  * * %1  - The timer was pending and deactivated
  * * %-1 - The timer callback function is running on a different CPU
  */
-int try_to_del_timer_sync(struct timer_list *timer)
+static int __try_to_del_timer_sync(struct timer_list *timer)
 {
 	struct timer_base *base;
 	unsigned long flags;
@@ -1362,6 +1368,27 @@ int try_to_del_timer_sync(struct timer_list *timer)
 
 	return ret;
 }
+
+/**
+ * try_to_del_timer_sync - Try to deactivate a timer
+ * @timer:	Timer to deactivate
+ *
+ * This function tries to deactivate a timer. On success the timer is not
+ * queued and the timer callback function is not running on any CPU.
+ *
+ * This function does not guarantee that the timer cannot be rearmed right
+ * after dropping the base lock. That needs to be prevented by the calling
+ * code if necessary.
+ *
+ * Return:
+ * * %0  - The timer was not pending
+ * * %1  - The timer was pending and deactivated
+ * * %-1 - The timer callback function is running on a different CPU
+ */
+int try_to_del_timer_sync(struct timer_list *timer)
+{
+	return __try_to_del_timer_sync(timer);
+}
 EXPORT_SYMBOL(try_to_del_timer_sync);
 
 #ifdef CONFIG_PREEMPT_RT
@@ -1438,45 +1465,15 @@ static inline void del_timer_wait_running(struct timer_list *timer) { }
 #endif
 
 /**
- * timer_delete_sync - Deactivate a timer and wait for the handler to finish.
+ * __timer_delete_sync - Internal function: Deactivate a timer and wait
+ *			 for the handler to finish.
  * @timer:	The timer to be deactivated
  *
- * Synchronization rules: Callers must prevent restarting of the timer,
- * otherwise this function is meaningless. It must not be called from
- * interrupt contexts unless the timer is an irqsafe one. The caller must
- * not hold locks which would prevent completion of the timer's callback
- * function. The timer's handler must not call add_timer_on(). Upon exit
- * the timer is not queued and the handler is not running on any CPU.
- *
- * For !irqsafe timers, the caller must not hold locks that are held in
- * interrupt context. Even if the lock has nothing to do with the timer in
- * question.  Here's why::
- *
- *    CPU0                             CPU1
- *    ----                             ----
- *                                     <SOFTIRQ>
- *                                       call_timer_fn();
- *                                       base->running_timer = mytimer;
- *    spin_lock_irq(somelock);
- *                                     <IRQ>
- *                                        spin_lock(somelock);
- *    timer_delete_sync(mytimer);
- *    while (base->running_timer == mytimer);
- *
- * Now timer_delete_sync() will never return and never release somelock.
- * The interrupt on the other CPU is waiting to grab somelock but it has
- * interrupted the softirq that CPU0 is waiting to finish.
- *
- * This function cannot guarantee that the timer is not rearmed again by
- * some concurrent or preempting code, right after it dropped the base
- * lock. If there is the possibility of a concurrent rearm then the return
- * value of the function is meaningless.
- *
  * Return:
  * * %0	- The timer was not pending
  * * %1	- The timer was pending and deactivated
  */
-int timer_delete_sync(struct timer_list *timer)
+static int __timer_delete_sync(struct timer_list *timer)
 {
 	int ret;
 
@@ -1506,7 +1503,7 @@ int timer_delete_sync(struct timer_list *timer)
 		lockdep_assert_preemption_enabled();
 
 	do {
-		ret = try_to_del_timer_sync(timer);
+		ret = __try_to_del_timer_sync(timer);
 
 		if (unlikely(ret < 0)) {
 			del_timer_wait_running(timer);
@@ -1516,6 +1513,50 @@ int timer_delete_sync(struct timer_list *timer)
 
 	return ret;
 }
+
+/**
+ * timer_delete_sync - Deactivate a timer and wait for the handler to finish.
+ * @timer:	The timer to be deactivated
+ *
+ * Synchronization rules: Callers must prevent restarting of the timer,
+ * otherwise this function is meaningless. It must not be called from
+ * interrupt contexts unless the timer is an irqsafe one. The caller must
+ * not hold locks which would prevent completion of the timer's callback
+ * function. The timer's handler must not call add_timer_on(). Upon exit
+ * the timer is not queued and the handler is not running on any CPU.
+ *
+ * For !irqsafe timers, the caller must not hold locks that are held in
+ * interrupt context. Even if the lock has nothing to do with the timer in
+ * question.  Here's why::
+ *
+ *    CPU0                             CPU1
+ *    ----                             ----
+ *                                     <SOFTIRQ>
+ *                                       call_timer_fn();
+ *                                       base->running_timer = mytimer;
+ *    spin_lock_irq(somelock);
+ *                                     <IRQ>
+ *                                        spin_lock(somelock);
+ *    timer_delete_sync(mytimer);
+ *    while (base->running_timer == mytimer);
+ *
+ * Now timer_delete_sync() will never return and never release somelock.
+ * The interrupt on the other CPU is waiting to grab somelock but it has
+ * interrupted the softirq that CPU0 is waiting to finish.
+ *
+ * This function cannot guarantee that the timer is not rearmed again by
+ * some concurrent or preempting code, right after it dropped the base
+ * lock. If there is the possibility of a concurrent rearm then the return
+ * value of the function is meaningless.
+ *
+ * Return:
+ * * %0	- The timer was not pending
+ * * %1	- The timer was pending and deactivated
+ */
+int timer_delete_sync(struct timer_list *timer)
+{
+	return __timer_delete_sync(timer);
+}
 EXPORT_SYMBOL(timer_delete_sync);
 
 static void call_timer_fn(struct timer_list *timer,

[tip: timers/core] timers: Silently ignore timers with a NULL function

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     d02e382cef06cc73561dd32dfdc171c00dcc416d
Gitweb:        https://git.kernel.org/tip/d02e382cef06cc73561dd32dfdc171c00dcc416d
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Thu, 24 Nov 2022 09:22:36 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 24 Nov 2022 15:09:11 +01:00

timers: Silently ignore timers with a NULL function

Tearing down timers which have circular dependencies to other
functionality, e.g. workqueues, where the timer can schedule work and work
can arm timers, is not trivial.

In those cases it is desired to shutdown the timer in a way which prevents
rearming of the timer. The mechanism to do so is to set timer->function to
NULL and use this as an indicator for the timer arming functions to ignore
the (re)arm request.

In preparation for that replace the warnings in the relevant code paths
with checks for timer->function == NULL. If the pointer is NULL, then
discard the rearm request silently.

Add debug_assert_init() instead of the WARN_ON_ONCE(!timer->function)
checks so that debug objects can warn about non-initialized timers.

The warning of debug objects does not warn if timer->function == NULL.  It
warns when timer was not initialized using timer_setup[_on_stack]() or via
DEFINE_TIMER(). If developers fail to enable debug objects and then waste
lots of time to figure out why their non-initialized timer is not firing,
they deserve it. Same for initializing a timer with a NULL function.

Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home
Link: https://lore.kernel.org/all/20221110064101.429013735@goodmis.org
Link: https://lore.kernel.org/r/87wn7kdann.ffs@tglx

---
 kernel/time/timer.c | 57 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 2b5e8c2..e4fcf56 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1017,7 +1017,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option
 	unsigned int idx = UINT_MAX;
 	int ret = 0;
 
-	BUG_ON(!timer->function);
+	debug_assert_init(timer);
 
 	/*
 	 * This is a common optimization triggered by the networking code - if
@@ -1044,6 +1044,14 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option
 		 * dequeue/enqueue dance.
 		 */
 		base = lock_timer_base(timer, &flags);
+		/*
+		 * Has @timer been shutdown? This needs to be evaluated
+		 * while holding base lock to prevent a race against the
+		 * shutdown code.
+		 */
+		if (!timer->function)
+			goto out_unlock;
+
 		forward_timer_base(base);
 
 		if (timer_pending(timer) && (options & MOD_TIMER_REDUCE) &&
@@ -1070,6 +1078,14 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option
 		}
 	} else {
 		base = lock_timer_base(timer, &flags);
+		/*
+		 * Has @timer been shutdown? This needs to be evaluated
+		 * while holding base lock to prevent a race against the
+		 * shutdown code.
+		 */
+		if (!timer->function)
+			goto out_unlock;
+
 		forward_timer_base(base);
 	}
 
@@ -1128,8 +1144,12 @@ out_unlock:
  * mod_timer_pending() is the same for pending timers as mod_timer(), but
  * will not activate inactive timers.
  *
+ * If @timer->function == NULL then the start operation is silently
+ * discarded.
+ *
  * Return:
- * * %0 - The timer was inactive and not modified
+ * * %0 - The timer was inactive and not modified or was in
+ *	  shutdown state and the operation was discarded
  * * %1 - The timer was active and requeued to expire at @expires
  */
 int mod_timer_pending(struct timer_list *timer, unsigned long expires)
@@ -1155,8 +1175,12 @@ EXPORT_SYMBOL(mod_timer_pending);
  * same timer, then mod_timer() is the only safe way to modify the timeout,
  * since add_timer() cannot modify an already running timer.
  *
+ * If @timer->function == NULL then the start operation is silently
+ * discarded. In this case the return value is 0 and meaningless.
+ *
  * Return:
- * * %0 - The timer was inactive and started
+ * * %0 - The timer was inactive and started or was in shutdown
+ *	  state and the operation was discarded
  * * %1 - The timer was active and requeued to expire at @expires or
  *	  the timer was active and not modified because @expires did
  *	  not change the effective expiry time
@@ -1176,8 +1200,12 @@ EXPORT_SYMBOL(mod_timer);
  * modify an enqueued timer if that would reduce the expiration time. If
  * @timer is not enqueued it starts the timer.
  *
+ * If @timer->function == NULL then the start operation is silently
+ * discarded.
+ *
  * Return:
- * * %0 - The timer was inactive and started
+ * * %0 - The timer was inactive and started or was in shutdown
+ *	  state and the operation was discarded
  * * %1 - The timer was active and requeued to expire at @expires or
  *	  the timer was active and not modified because @expires
  *	  did not change the effective expiry time such that the
@@ -1200,6 +1228,9 @@ EXPORT_SYMBOL(timer_reduce);
  * The @timer->expires and @timer->function fields must be set prior
  * to calling this function.
  *
+ * If @timer->function == NULL then the start operation is silently
+ * discarded.
+ *
  * If @timer->expires is already in the past @timer will be queued to
  * expire at the next timer tick.
  *
@@ -1228,7 +1259,9 @@ void add_timer_on(struct timer_list *timer, int cpu)
 	struct timer_base *new_base, *base;
 	unsigned long flags;
 
-	if (WARN_ON_ONCE(timer_pending(timer) || !timer->function))
+	debug_assert_init(timer);
+
+	if (WARN_ON_ONCE(timer_pending(timer)))
 		return;
 
 	new_base = get_timer_cpu_base(timer->flags, cpu);
@@ -1239,6 +1272,13 @@ void add_timer_on(struct timer_list *timer, int cpu)
 	 * wrong base locked.  See lock_timer_base().
 	 */
 	base = lock_timer_base(timer, &flags);
+	/*
+	 * Has @timer been shutdown? This needs to be evaluated while
+	 * holding base lock to prevent a race against the shutdown code.
+	 */
+	if (!timer->function)
+		goto out_unlock;
+
 	if (base != new_base) {
 		timer->flags |= TIMER_MIGRATING;
 
@@ -1252,6 +1292,7 @@ void add_timer_on(struct timer_list *timer, int cpu)
 
 	debug_timer_activate(timer);
 	internal_add_timer(base, timer);
+out_unlock:
 	raw_spin_unlock_irqrestore(&base->lock, flags);
 }
 EXPORT_SYMBOL_GPL(add_timer_on);
@@ -1541,6 +1582,12 @@ static void expire_timers(struct timer_base *base, struct hlist_head *head)
 
 		fn = timer->function;
 
+		if (WARN_ON_ONCE(!fn)) {
+			/* Should never happen. Emphasis on should! */
+			base->running_timer = NULL;
+			continue;
+		}
+
 		if (timer->flags & TIMER_IRQSAFE) {
 			raw_spin_unlock(&base->lock);
 			call_timer_fn(timer, fn, baseclk);

[tip: timers/core] timers: Use del_timer_sync() even on UP

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     168f6b6ffbeec0b9333f3582e4cf637300858db5
Gitweb:        https://git.kernel.org/tip/168f6b6ffbeec0b9333f3582e4cf637300858db5
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Wed, 23 Nov 2022 21:18:42 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 24 Nov 2022 15:09:11 +01:00

timers: Use del_timer_sync() even on UP

del_timer_sync() is assumed to be pointless on uniprocessor systems and can
be mapped to del_timer() because in theory del_timer() can never be invoked
while the timer callback function is executed.

This is not entirely true because del_timer() can be invoked from interrupt
context and therefore hit in the middle of a running timer callback.

Contrary to that del_timer_sync() is not allowed to be invoked from
interrupt context unless the affected timer is marked with TIMER_IRQSAFE.
del_timer_sync() has proper checks in place to detect such a situation.

Give up on the UP optimization and make del_timer_sync() unconditionally
available.

Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home
Link: https://lore.kernel.org/all/20221110064101.429013735@goodmis.org
Link: https://lore.kernel.org/r/20221123201624.888306160@linutronix.de

---
 include/linux/timer.h | 7 +------
 kernel/time/timer.c   | 2 --
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/include/linux/timer.h b/include/linux/timer.h
index ea35d00..31b8b60 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -183,12 +183,7 @@ extern int timer_reduce(struct timer_list *timer, unsigned long expires);
 extern void add_timer(struct timer_list *timer);
 
 extern int try_to_del_timer_sync(struct timer_list *timer);
-
-#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
-  extern int del_timer_sync(struct timer_list *timer);
-#else
-# define del_timer_sync(t)		del_timer(t)
-#endif
+extern int del_timer_sync(struct timer_list *timer);
 
 extern void init_timers(void);
 struct hrtimer;
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index ea0150c..550f0df 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1396,7 +1396,6 @@ static inline void timer_sync_wait_running(struct timer_base *base) { }
 static inline void del_timer_wait_running(struct timer_list *timer) { }
 #endif
 
-#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
 /**
  * del_timer_sync - Deactivate a timer and wait for the handler to finish.
  * @timer:	The timer to be deactivated
@@ -1477,7 +1476,6 @@ int del_timer_sync(struct timer_list *timer)
 	return ret;
 }
 EXPORT_SYMBOL(del_timer_sync);
-#endif
 
 static void call_timer_fn(struct timer_list *timer,
 			  void (*fn)(struct timer_list *),