All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Steven Rostedt <rostedt@goodmis.org>, linux-kernel@vger.kernel.org
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Anna-Maria Gleixner <anna-maria@linutronix.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Julia Lawall <Julia.Lawall@inria.fr>
Subject: Re: [PATCH v6 5/6] timers: Add timer_shutdown() to be called before freeing timers
Date: Sun, 13 Nov 2022 23:20:31 +0100	[thread overview]
Message-ID: <87a64uts28.ffs@tglx> (raw)
In-Reply-To: <20221110064147.529154710@goodmis.org>

On Thu, Nov 10 2022 at 01:41, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

$Subject: !!@*&^*&^@!

> Before a timer is to be freed, it must be shutdown. But there are some
> locations were timer_shutdown_sync() can not be called due to the context
> the object that holds the timer is in when it is freed.

locations? This is not about locations, it's about contexts. And please
provide a proper example for such a context.

> For cases where the logic should keep the timer from being re-armed but
> still needs to be shutdown with a sync, a new API of timer_shutdown() is
> available.

"Needs to shutdown with a sync"? "is available"? Try again with
comprehensible explanations.

> This is the same as del_timer() except that after it is called, the
> timer can not be re-armed. If it is, a WARN_ON_ONCE() will be
> triggered.
>
> The implementation of timer_shutdown() follows the timer_shutdown_sync()
> method of using the same code as del_timer() but will pass in a boolean
> that the timer is about to be freed, in which case the timer->function is
> set to NULL, just like timer_shutdown_sync().

That's complete useless information for a changelog. We can see that
from the patch itself, no?

Changelogs are about context and the problem the patch tries to solve,
not about implementation details.

> +/**
> + * del_timer - deactivate a timer.
> + * @timer: the timer to be deactivated

See previous comments about uppercase.

> + * del_timer() deactivates a timer - this works on both active and inactive
> + * timers.

How so? What "works"? What's the work done on an inactive timer? Also
this lacks documentation that this function is fundamentally racy
against a concurrent rearm.

> + * The function returns whether it has deactivated a pending timer or not.
> + * (ie. del_timer() of an inactive timer returns 0, del_timer() of an
> + * active timer returns 1.)

See previous comment about return value documentation.

> + */
> +static inline int del_timer(struct timer_list *timer)
> +{
> +	return __del_timer(timer, false);
> +}
> +
> +/**
> + * timer_shutdown - deactivate a timer and shut it down
> + * @timer: the timer to be deactivated
> + *
> + * timer_shutdown() deactivates a timer - this works on both active
> + * and inactive timers, and will prevent it from being rearmed.

This needs some further explanation especially vs. the function pointer
being set to NULL. Which means that in case that the timer is not freed
and reused later on it needs to be initialized again. Which is btw
lacking from timer_shutdown_sync() too.

> + * The function returns whether it has deactivated a pending timer or not.
> + * (ie. timer_shutdown() of an inactive timer returns 0,
> + *   timer_shutdown() of an active timer returns 1.)
> + */
> +static inline int timer_shutdown(struct timer_list *timer)
> +{
> +	return __del_timer(timer, true);
> +}
> +
>  /*
>   * The jiffies value which is added to now, when there is no timer
>   * in the timer wheel:
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 111a3550b3f2..7c224766065e 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1240,18 +1240,7 @@ void add_timer_on(struct timer_list *timer, int cpu)
>  }
>  EXPORT_SYMBOL_GPL(add_timer_on);
>  
> -/**
> - * del_timer - deactivate a timer.
> - * @timer: the timer to be deactivated
> - *
> - * del_timer() deactivates a timer - this works on both active and inactive
> - * timers.
> - *
> - * The function returns whether it has deactivated a pending timer or not.
> - * (ie. del_timer() of an inactive timer returns 0, del_timer() of an
> - * active timer returns 1.)
> - */

Instead of blurbing about invoking __del_timer() with free=true in the
changelog you could have kept the kernel doc here and/or added some
useful comment to the code below.

But...

> -int del_timer(struct timer_list *timer)
> +int __del_timer(struct timer_list *timer, bool free)
>  {
>  	struct timer_base *base;
>  	unsigned long flags;
> @@ -1262,12 +1251,18 @@ int del_timer(struct timer_list *timer)
>  	if (timer_pending(timer)) {
>  		base = lock_timer_base(timer, &flags);
>  		ret = detach_if_pending(timer, base, true);
> +		if (free)
> +			timer->function = NULL;
> +		raw_spin_unlock_irqrestore(&base->lock, flags);
> +	} else if (free) {
> +		base = lock_timer_base(timer, &flags);
> +		timer->function = NULL;
>  		raw_spin_unlock_irqrestore(&base->lock, flags);
>  	}

... this function is a concurrency disaster:

CPU0                           		CPU1

timer_shutdown(timer)
  __del_timer(timer, free=true)
    // timer is not pending
    ....
    } else if (free)                    mod_timer()
                                          lock_timer(timer);
      lock_timer(timer)                   enqueue_timer(timer);
                                          unlock_timer(timer);
      timer->function = NULL;
      unlock_timer(timer);
                                        //timer expires
                                        lock_timer(timer);
                                        fn = timer->function;
                                        unlock_timer(timer);
                                        fn(timer); <--- NULL pointer dereference

So you "solve" the existing problem by introducing one which is even
more horrible to debug, right?

Let me go back to the timer_shutdown_sync() variant and figure out
whether that one is at least not borked in the same way.

Thanks,

        tglx

  reply	other threads:[~2022-11-13 22:20 UTC|newest]

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

Reply instructions:

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

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

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

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

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

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

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