| 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
| ||
|
Message-ID: <87pmlrkgi3.ffs@tglx>
Date: Fri, 08 Apr 2022 12:37:40 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Steven Rostedt <rostedt@...dmis.org>,
LKML <linux-kernel@...r.kernel.org>
Cc: jstultz@...gle.com, Stephen Boyd <sboyd@...nel.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Peter Zijlstra <peterz@...radead.org>,
Marcel Holtmann <marcel@...tmann.org>,
Johan Hedberg <johan.hedberg@...il.com>,
Luiz Augusto von Dentz <luiz.dentz@...il.com>,
Eric Dumazet <edumazet@...gle.com>,
Guenter Roeck <linux@...ck-us.net>
Subject: Re: [RFC][PATCH] timers: Add del_time_free() to be called before
freeing timers
On Thu, Apr 07 2022 at 16:17, Steven Rostedt wrote:
> diff --git a/include/linux/timer.h b/include/linux/timer.h
> index fda13c9d1256..cc76ab0659f3 100644
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h
> @@ -67,11 +67,12 @@ struct timer_list {
> #define TIMER_DEFERRABLE 0x00080000
> #define TIMER_PINNED 0x00100000
> #define TIMER_IRQSAFE 0x00200000
> +#define TIMER_FREED 0x00400000
> #define TIMER_INIT_FLAGS (TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE)
> -#define TIMER_ARRAYSHIFT 22
> -#define TIMER_ARRAYMASK 0xFFC00000
> +#define TIMER_ARRAYSHIFT 23
> +#define TIMER_ARRAYMASK 0xFF800000
#define LVL_BITS 6
#define LVL_SIZE (1UL << LVL_BITS)
#if HZ > 100
# define LVL_DEPTH 9
# else
# define LVL_DEPTH 8
#endif
#define WHEEL_SIZE (LVL_SIZE * LVL_DEPTH)
WHEEL_SIZE(HZ > 100) = (1 << 6) * 9 = 576
max_index(HZ > 100) = 575 = 0x23f
0x23f << ARRAY_SHIFT = 0x11f800000
Q: Does 0x11f800000 fit into 32bit?
A: No, and we are not making flags 64bit just because.
If you want a bit you have to steal it from the CPUMASK field, but you
could simply set timer->function to NULL, so you can spare extra checks,
no?
> @@ -966,6 +966,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option
>
> BUG_ON(!timer->function);
>
> + WARN_ON(timer->flags & TIMER_FREED);
So this would become:
- BUG_ON(!timer->function);
+ if (WARN_ON(!timer->function))
+ return -EBROKEN;
You really want to return here and not proceed.
That needs a corresponding change in add_timer_on() plus:
- fn = timer->function;
+ fn = READ_ONCE(timer->function);
in expire_timers().
> +int del_timer_free(struct timer_list *timer)
> +{
> + struct timer_base *base;
> + unsigned long flags;
> + int ret;
> +
> + debug_assert_init(timer);
> +
> + for (;;) {
> + ret = -1;
> + base = lock_timer_base(timer, &flags);
> +
> + if (base->running_timer != timer)
> + ret = detach_if_pending(timer, base, true);
> +
> + if (ret >= 0) {
> + timer->flags |= TIMER_FREED;
> + raw_spin_unlock_irqrestore(&base->lock, flags);
> + break;
> + }
> +
> + raw_spin_unlock_irqrestore(&base->lock, flags);
> +
> + del_timer_wait_running(timer);
> + cpu_relax();
> + }
> +
> + return ret;
> +}
So this reimplements del_timer_sync() just slightly different for no
reason. Something like the uncompiled below.
Thanks,
tglx
---
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -183,12 +183,17 @@ extern int timer_reduce(struct timer_lis
extern void add_timer(struct timer_list *timer);
extern int try_to_del_timer_sync(struct timer_list *timer);
+extern int __del_timer_sync(struct timer_list *timer, bool free);
-#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
- extern int del_timer_sync(struct timer_list *timer);
-#else
-# define del_timer_sync(t) del_timer(t)
-#endif
+static inline int del_timer_sync(struct timer_list *timer)
+{
+ return __del_timer_sync(timer, false);
+}
+
+static inline int del_timer_sync_free(struct timer_list *timer)
+{
+ return __del_timer_sync(timer, true);
+}
#define del_singleshot_timer_sync(t) del_timer_sync(t)
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -964,7 +964,8 @@ static inline int
unsigned int idx = UINT_MAX;
int ret = 0;
- BUG_ON(!timer->function);
+ if (WARN_ON(!timer->function))
+ return -EINVAL;
/*
* This is a common optimization triggered by the networking code - if
@@ -1140,7 +1141,8 @@ EXPORT_SYMBOL(timer_reduce);
*/
void add_timer(struct timer_list *timer)
{
- BUG_ON(timer_pending(timer));
+ if (WARN_ON(timer_pending(timer)))
+ return;
__mod_timer(timer, timer->expires, MOD_TIMER_NOTPENDING);
}
EXPORT_SYMBOL(add_timer);
@@ -1157,7 +1159,8 @@ void add_timer_on(struct timer_list *tim
struct timer_base *new_base, *base;
unsigned long flags;
- BUG_ON(timer_pending(timer) || !timer->function);
+ if (WARN_ON(timer_pending(timer) || !timer->function))
+ return;
new_base = get_timer_cpu_base(timer->flags, cpu);
@@ -1213,14 +1216,7 @@ int del_timer(struct timer_list *timer)
}
EXPORT_SYMBOL(del_timer);
-/**
- * try_to_del_timer_sync - Try to deactivate a timer
- * @timer: timer to delete
- *
- * This function tries to deactivate a timer. Upon successful (ret >= 0)
- * exit the timer is not queued and the handler is not running on any CPU.
- */
-int try_to_del_timer_sync(struct timer_list *timer)
+static int __try_to_del_timer_sync(struct timer_list *timer, boot free)
{
struct timer_base *base;
unsigned long flags;
@@ -1232,11 +1228,25 @@ int try_to_del_timer_sync(struct timer_l
if (base->running_timer != timer)
ret = detach_if_pending(timer, base, true);
+ if (free)
+ timer->function = NULL;
raw_spin_unlock_irqrestore(&base->lock, flags);
return ret;
}
+
+/**
+ * try_to_del_timer_sync - Try to deactivate a timer
+ * @timer: timer to delete
+ *
+ * This function tries to deactivate a timer. Upon successful (ret >= 0)
+ * exit the timer is not queued and the handler is not running on any CPU.
+ */
+int try_to_del_timer_sync(struct timer_list *timer)
+{
+ return __try_to_del_timer_sync(timer, false);
+}
EXPORT_SYMBOL(try_to_del_timer_sync);
#ifdef CONFIG_PREEMPT_RT
@@ -1312,7 +1322,6 @@ static inline void timer_sync_wait_runni
static inline void del_timer_wait_running(struct timer_list *timer) { }
#endif
-#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
/**
* del_timer_sync - deactivate a timer and wait for the handler to finish.
* @timer: the timer to be deactivated
@@ -1349,7 +1358,7 @@ static inline void del_timer_wait_runnin
*
* The function returns whether it has deactivated a pending timer or not.
*/
-int del_timer_sync(struct timer_list *timer)
+int __del_timer_sync(struct timer_list *timer, bool free)
{
int ret;
@@ -1379,7 +1388,7 @@ int del_timer_sync(struct timer_list *ti
lockdep_assert_preemption_enabled();
do {
- ret = try_to_del_timer_sync(timer);
+ ret = __try_to_del_timer_sync(timer, free);
if (unlikely(ret < 0)) {
del_timer_wait_running(timer);
@@ -1389,8 +1398,7 @@ int del_timer_sync(struct timer_list *ti
return ret;
}
-EXPORT_SYMBOL(del_timer_sync);
-#endif
+EXPORT_SYMBOL(__del_timer_sync);
static void call_timer_fn(struct timer_list *timer,
void (*fn)(struct timer_list *),
Powered by blists - more mailing lists