migration_base is used as a placeholder when an hrtimer is switching
between base (see switch_hrtimer_timer_base). It is possible
theoritically possible to have timer->base equal to migration_base.
Even if it is a placeholder, it would pass all the current check in
hrtimer_grab_expiry_lock() leading to use softirq_expiry_lock
uninitialized.
This is can be prevented by checking whether the base is equal to
the placeholder (i.e. migration_base).
Furthermore, all the path leading to hrtimer_grab_expiry_lock() assumes
timer->base and timer->base->cpu_base are always non-NULL. So it is safe
to remove the NULL checks here.
Signed-off-by: Julien Grall <[email protected]>
---
I don't have a reproducer so far, but I can't see why it would not be
possible to happen.
---
kernel/time/hrtimer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 119414a2f59c..5eb45a868de9 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -934,7 +934,7 @@ void hrtimer_grab_expiry_lock(const struct hrtimer *timer)
{
struct hrtimer_clock_base *base = READ_ONCE(timer->base);
- if (timer->is_soft && base && base->cpu_base) {
+ if (timer->is_soft && base != &migration_base) {
spin_lock(&base->cpu_base->softirq_expiry_lock);
spin_unlock(&base->cpu_base->softirq_expiry_lock);
}
--
2.11.0
On Wed, 21 Aug 2019, Julien Grall wrote:
> migration_base is used as a placeholder when an hrtimer is switching
> between base (see switch_hrtimer_timer_base). It is possible
> theoritically possible to have timer->base equal to migration_base.
>
> Even if it is a placeholder, it would pass all the current check in
> hrtimer_grab_expiry_lock() leading to use softirq_expiry_lock
> uninitialized.
>
> This is can be prevented by checking whether the base is equal to
> the placeholder (i.e. migration_base).
That's a lame argument. The point is that it does not make sense to do that
on migration base, but not for the reason you are giving (uninitialized
lock).
If base == migration_base then there is no point to lock soft_expiry_lock
simply because the timer is not executing the callback in soft irq context
and the whole lock/unlock dance can be avoided.
But, yes. Good catch.
Thanks,
tglx
Hi Thomas,
Thank you for the review.
On 21/08/2019 15:02, Thomas Gleixner wrote:
> On Wed, 21 Aug 2019, Julien Grall wrote:
>
>> migration_base is used as a placeholder when an hrtimer is switching
>> between base (see switch_hrtimer_timer_base). It is possible
>> theoritically possible to have timer->base equal to migration_base.
>>
>> Even if it is a placeholder, it would pass all the current check in
>> hrtimer_grab_expiry_lock() leading to use softirq_expiry_lock
>> uninitialized.
>>
>> This is can be prevented by checking whether the base is equal to
>> the placeholder (i.e. migration_base).
>
> That's a lame argument. The point is that it does not make sense to do that
> on migration base, but not for the reason you are giving (uninitialized
> lock).
Fair point, I will update the commit message.
>
> If base == migration_base then there is no point to lock soft_expiry_lock
> simply because the timer is not executing the callback in soft irq context
> and the whole lock/unlock dance can be avoided.
>
> But, yes. Good catch.
Do you want me to resend the series or can I just provide an update to the
commit message here?
Cheers,
--
Julien Grall
The following commit has been merged into the timers/core branch of tip:
Commit-ID: 68b2c8c1e421096f4b46ac2ac502d25ca067a2a6
Gitweb: https://git.kernel.org/tip/68b2c8c1e421096f4b46ac2ac502d25ca067a2a6
Author: Julien Grall <[email protected]>
AuthorDate: Wed, 21 Aug 2019 10:24:09 +01:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 21 Aug 2019 16:10:01 +02:00
hrtimer: Don't take expiry_lock when timer is currently migrated
migration_base is used as a placeholder when an hrtimer is migrated to a
different CPU. In the case that hrtimer_cancel_wait_running() hits a timer
which is currently migrated it would pointlessly acquire the expiry lock of
the migration base, which is even not initialized.
Surely it could be initialized, but there is absolutely no point in
acquiring this lock because the timer is guaranteed not to run it's
callback for which the caller waits to finish on that base. So it would
just do the inc/lock/dec/unlock dance for nothing.
As the base switch is short and non-preemptible, there is no issue when the
wait function returns immediately.
The timer base and base->cpu_base cannot be NULL in the code path which is
invoking that, so just replace those checks with a check whether base is
migration base.
[ tglx: Updated from RT patch. Massaged changelog. Added comment. ]
Signed-off-by: Julien Grall <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
kernel/time/hrtimer.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index f48864e..ebbd0fb 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1217,7 +1217,11 @@ void hrtimer_cancel_wait_running(const struct hrtimer *timer)
/* Lockless read. Prevent the compiler from reloading it below */
struct hrtimer_clock_base *base = READ_ONCE(timer->base);
- if (!timer->is_soft || !base || !base->cpu_base) {
+ /*
+ * Just relax if the timer expires in hard interrupt context or if
+ * it is currently on the migration base.
+ */
+ if (!timer->is_soft || base == &migration_base)
cpu_relax();
return;
}
Commit
68b2c8c1e4210 ("hrtimer: Don't take expiry_lock when timer is currently migrated")
missed to add a bracket at the end of the if statement leading to
compile errors.
Since that commit the variable `migration_base' is always used but only
available on SMP configuration thus leading to another compile error.
The changelog says
"The timer base and base->cpu_base cannot be NULL in the code path"
so it is safe to limit this check to SMP configurations only.
Add the missing bracket to the if statement and hide `migration_base'
behind CONFIG_SMP bars.
Fixes: 68b2c8c1e4210 ("hrtimer: Don't take expiry_lock when timer is currently migrated")
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
kernel/time/hrtimer.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index f5a1a5e16216c..bc84c74ae5b96 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1216,12 +1216,16 @@ void hrtimer_cancel_wait_running(const struct hrtimer *timer)
{
/* Lockless read. Prevent the compiler from reloading it below */
struct hrtimer_clock_base *base = READ_ONCE(timer->base);
+ bool is_migration_base = false;
/*
* Just relax if the timer expires in hard interrupt context or if
* it is currently on the migration base.
*/
- if (!timer->is_soft || base == &migration_base)
+#ifdef CONFIG_SMP
+ is_migration_base = base == &migration_base;
+#endif
+ if (timer->is_soft || is_migration_base) {
cpu_relax();
return;
}
--
2.23.0
On Wed, 4 Sep 2019, Sebastian Andrzej Siewior wrote:
> Commit
> 68b2c8c1e4210 ("hrtimer: Don't take expiry_lock when timer is currently migrated")
That is the same information as in the fixes tag. So you can say something
like this:
The recent change to avoid taking the expiry lock when a timer is currently
migrated missed .....
> missed to add a bracket at the end of the if statement leading to
> compile errors.
> Since that commit the variable `migration_base' is always used but only
> available on SMP configuration thus leading to another compile error.
> The changelog says
> "The timer base and base->cpu_base cannot be NULL in the code path"
>
> so it is safe to limit this check to SMP configurations only.
>
> Add the missing bracket to the if statement and hide `migration_base'
> behind CONFIG_SMP bars.
>
> Fixes: 68b2c8c1e4210 ("hrtimer: Don't take expiry_lock when timer is currently migrated")
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> ---
> kernel/time/hrtimer.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index f5a1a5e16216c..bc84c74ae5b96 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1216,12 +1216,16 @@ void hrtimer_cancel_wait_running(const struct hrtimer *timer)
> {
> /* Lockless read. Prevent the compiler from reloading it below */
> struct hrtimer_clock_base *base = READ_ONCE(timer->base);
> + bool is_migration_base = false;
>
> /*
> * Just relax if the timer expires in hard interrupt context or if
> * it is currently on the migration base.
> */
> - if (!timer->is_soft || base == &migration_base)
> +#ifdef CONFIG_SMP
> + is_migration_base = base == &migration_base;
> +#endif
That's beyond ugly.
What's wrong with:
if (!timer->is_soft || is_migration_base(base))
and have two helpers in the relevant ifdeffed section?
Thanks,
tglx
The recent change to avoid taking the expiry lock when a timer is
currently migrated missed to add a bracket at the end of the if
statement leading to compile errors.
Since that commit the variable `migration_base' is always used but only
available on SMP configuration thus leading to another compile error.
The changelog says
"The timer base and base->cpu_base cannot be NULL in the code path"
so it is safe to limit this check to SMP configurations only.
Add the missing bracket to the if statement and hide `migration_base'
behind CONFIG_SMP bars.
Fixes: 68b2c8c1e4210 ("hrtimer: Don't take expiry_lock when timer is currently migrated")
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
---
v1…v2:
- use is_migration_base() as a helper function
- slightly reword the commit message
kernel/time/hrtimer.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index f5a1a5e16216c..eaf31ac38efbb 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -140,6 +140,11 @@ static struct hrtimer_cpu_base migration_cpu_base = {
#define migration_base migration_cpu_base.clock_base[0]
+static bool is_migration_base(struct hrtimer_clock_base *base)
+{
+ return base == &migration_base;
+}
+
/*
* We are using hashed locking: holding per_cpu(hrtimer_bases)[n].lock
* means that all timers which are tied to this base via timer->base are
@@ -264,6 +269,11 @@ switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base,
#else /* CONFIG_SMP */
+static bool is_migration_base(struct hrtimer_clock_base *base)
+{
+ return false;
+}
+
static inline struct hrtimer_clock_base *
lock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags)
{
@@ -1221,7 +1231,7 @@ void hrtimer_cancel_wait_running(const struct hrtimer *timer)
* Just relax if the timer expires in hard interrupt context or if
* it is currently on the migration base.
*/
- if (!timer->is_soft || base == &migration_base)
+ if (!timer->is_soft || is_migration_base(base)) {
cpu_relax();
return;
}
--
2.23.0