NACK/Cmnt: [SRU][F][PATCH 4/9] timers: Split [try_to_]del_timer[_sync]() to prepare for shutdown mode
Massimiliano Pellizzer
massimiliano.pellizzer at canonical.com
Thu Nov 14 20:01:33 UTC 2024
On Thu, 14 Nov 2024 at 20:25, Magali Lemes <magali.lemes at canonical.com> wrote:
>
> 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?
>
You are right, I can send a v2 without this change.
Thanks for the review.
> >
> > /*
> > * 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,
--
Massimiliano Pellizzer
More information about the kernel-team
mailing list