linux-kernel - Re: [patch 10/15] timers: Silently ignore timers with a NULL function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221121163522.5eedbfe9@gandalf.local.home>
Date:   Mon, 21 Nov 2022 16:35:22 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...uxfoundation.org>,
        Anna-Maria Behnsen <anna-maria@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Julia Lawall <Julia.Lawall@...ia.fr>,
        Arnd Bergmann <arnd@...db.de>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Marc Zyngier <maz@...nel.org>,
        Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        linux-bluetooth@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [patch 10/15] timers: Silently ignore timers with a NULL
 function

On Tue, 15 Nov 2022 21:28:49 +0100 (CET)
Thomas Gleixner <tglx@...utronix.de> 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);