[SRU][F][PATCH 4/9] timers: Split [try_to_]del_timer[_sync]() to prepare for shutdown mode
Magali Lemes
magali.lemes at canonical.com
Thu Nov 14 19:25:28 UTC 2024
On 13/11/2024 13:18, Massimiliano Pellizzer wrote:
> From: Thomas Gleixner <tglx at linutronix.de>
>
> 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 at goodmis.org>
> Signed-off-by: Steven Rostedt <rostedt at goodmis.org>
> Signed-off-by: Thomas Gleixner <tglx at linutronix.de>
> Tested-by: Guenter Roeck <linux at roeck-us.net>
> Reviewed-by: Jacob Keller <jacob.e.keller at intel.com>
> Reviewed-by: Anna-Maria Behnsen <anna-maria at 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
>
> (backported from commit 8553b5f2774a66b1f293b7d783934210afb8f23c)
> [mpellizzer: backported solving trivial merge conflicts and by using
> hardirq_count() instead of in_hardirq() since the latter is not defined
> and they resolve to the same instruction]
> CVE-2024-35887
> Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com>
> ---
> kernel/time/timer.c | 145 ++++++++++++++++++++++++++++----------------
> 1 file changed, 93 insertions(+), 52 deletions(-)
>
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index f4a28deb179a..2aca1a7edf6c 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1206,20 +1206,14 @@ void add_timer_on(struct timer_list *timer, int cpu)
> 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;
> @@ -1235,25 +1229,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;
> @@ -1270,6 +1276,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
> @@ -1346,45 +1373,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;
>
> @@ -1404,7 +1401,7 @@ int timer_delete_sync(struct timer_list *timer)
> * don't use it in hardirq context, because it
> * could lead to deadlock.
> */
> - WARN_ON(in_irq() && !(timer->flags & TIMER_IRQSAFE));
> + WARN_ON(hardirq_count() && !(timer->flags & TIMER_IRQSAFE));
I think this change is not necessary because in include/linux/preempt.h
we have `#define in_irq() (hardirq_count())`, so we could just leave
this line as it is. What do you think?
>
> /*
> * Must be able to sleep on PREEMPT_RT because of the slowpath in
> @@ -1414,7 +1411,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);
> @@ -1424,6 +1421,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,
More information about the kernel-team
mailing list