[10/15] timers: Silently ignore timers with a NULL function

Commit Message

Thomas Gleixner Nov. 15, 2022, 8:28 p.m. UTC

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 it 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 pathes
with checks for timer->function == NULL and 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.

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.

Co-developed-by: Steven Rostedt <[e-mail]>
Signed-off-by: Steven Rostedt <[e-mail]>
Signed-off-by: Thomas Gleixner <[e-mail]>
Link: https://lore.kernel.org/all/[e-mail]
Link: https://lore.kernel.org/all/[e-mail]
---
 kernel/time/timer.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

Comments

Steven Rostedt Nov. 21, 2022, 9:35 p.m. UTC | #1

On Tue, 15 Nov 2022 21:28:49 +0100 (CET)
Thomas Gleixner <[e-mail]> wrote:

> @@ -1128,6 +1144,9 @@ static inline int
>   * 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
>   * * %1 - The timer was active and requeued to expire at @expires
> @@ -1154,6 +1173,9 @@ 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, the return value is 0 and meaningless.
> + *
>   * Return:
>   * * %0 - The timer was inactive and started

For those that only read the "Return" portion of kernel-doc, perhaps add
here:
             "or the timer is in the shutdown state and was not started".


>   * * %1 - The timer was active and requeued to expire at @expires or
> @@ -1175,6 +1197,9 @@ 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
>   * * %1 - The timer was active and requeued to expire at @expires or
> @@ -1201,6 +1226,9 @@ EXPORT_SYMBOL(timer_reduce);
>   *
>   * If @timer->expires is already in the past @timer will be queued to
>   * expire at the next timer tick.
> + *
> + * If @timer->function == NULL then the start operation is silently
> + * discarded.
>   */
>  void add_timer(struct timer_list *timer)
>  {
> @@ -1217,13 +1245,18 @@ EXPORT_SYMBOL(add_timer);
>   *
>   * This can only operate on an inactive timer. Attempts to invoke this on
>   * an active timer are rejected with a warning.
> + *
> + * If @timer->function == NULL then the start operation is silently
> + * discarded.
>   */
>  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);
> @@ -1234,6 +1267,13 @@ void add_timer_on(struct timer_list *tim
>  	 * 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;
>  
> @@ -1247,6 +1287,7 @@ void add_timer_on(struct timer_list *tim
>  
>  	debug_timer_activate(timer);
>  	internal_add_timer(base, timer);
> +out_unlock:
>  	raw_spin_unlock_irqrestore(&base->lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(add_timer_on);
> @@ -1532,6 +1573,12 @@ static void expire_timers(struct timer_b
>  
>  		fn = timer->function;
>  
> +		if (WARN_ON_ONCE(!fn)) {
> +			/* Should never happen. Emphasis on should! */
> +			base->running_timer = NULL;
> +			return;

Why return and not continue?

Wont this drop the other timers in the queue?

-- Steve


> +		}
> +
>  		if (timer->flags & TIMER_IRQSAFE) {
>  			raw_spin_unlock(&base->lock);
>  			call_timer_fn(timer, fn, baseclk);

Thomas Gleixner Nov. 21, 2022, 9:46 p.m. UTC | #2

On Mon, Nov 21 2022 at 16:35, Steven Rostedt wrote:
> On Tue, 15 Nov 2022 21:28:49 +0100 (CET)
> Thomas Gleixner <[e-mail]> wrote:
>> @@ -1532,6 +1573,12 @@ static void expire_timers(struct timer_b
>>  
>>  		fn = timer->function;
>>  
>> +		if (WARN_ON_ONCE(!fn)) {
>> +			/* Should never happen. Emphasis on should! */
>> +			base->running_timer = NULL;
>> +			return;
>
> Why return and not continue?
>
> Wont this drop the other timers in the queue?

Duh. Yes. Thanks for catching that!

Patch

--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1017,7 +1017,7 @@  static inline int
 	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 @@  static inline int
 		 * 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 @@  static inline int
 		}
 	} 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,6 +1144,9 @@  static inline int
  * 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
  * * %1 - The timer was active and requeued to expire at @expires
@@ -1154,6 +1173,9 @@  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, the return value is 0 and meaningless.
+ *
  * Return:
  * * %0 - The timer was inactive and started
  * * %1 - The timer was active and requeued to expire at @expires or
@@ -1175,6 +1197,9 @@  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
  * * %1 - The timer was active and requeued to expire at @expires or
@@ -1201,6 +1226,9 @@  EXPORT_SYMBOL(timer_reduce);
  *
  * If @timer->expires is already in the past @timer will be queued to
  * expire at the next timer tick.
+ *
+ * If @timer->function == NULL then the start operation is silently
+ * discarded.
  */
 void add_timer(struct timer_list *timer)
 {
@@ -1217,13 +1245,18 @@  EXPORT_SYMBOL(add_timer);
  *
  * This can only operate on an inactive timer. Attempts to invoke this on
  * an active timer are rejected with a warning.
+ *
+ * If @timer->function == NULL then the start operation is silently
+ * discarded.
  */
 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);
@@ -1234,6 +1267,13 @@  void add_timer_on(struct timer_list *tim
 	 * 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;
 
@@ -1247,6 +1287,7 @@  void add_timer_on(struct timer_list *tim
 
 	debug_timer_activate(timer);
 	internal_add_timer(base, timer);
+out_unlock:
 	raw_spin_unlock_irqrestore(&base->lock, flags);
 }
 EXPORT_SYMBOL_GPL(add_timer_on);
@@ -1532,6 +1573,12 @@  static void expire_timers(struct timer_b
 
 		fn = timer->function;
 
+		if (WARN_ON_ONCE(!fn)) {
+			/* Should never happen. Emphasis on should! */
+			base->running_timer = NULL;
+			return;
+		}
+
 		if (timer->flags & TIMER_IRQSAFE) {
 			raw_spin_unlock(&base->lock);
 			call_timer_fn(timer, fn, baseclk);