LKML Archive on lore.kernel.org
 help / color / Atom feed
* [RT PATCH 0/3] hrtimer: RT fixes for hrtimer_grab_expiry_lock()
@ 2019-08-21  9:24 Julien Grall
  2019-08-21  9:24 ` [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock() Julien Grall
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Julien Grall @ 2019-08-21  9:24 UTC (permalink / raw)
  To: linux-rt-users; +Cc: tglx, linux-kernel, maz, bigeasy, rostedt, Julien Grall

Hi all,

This small series contains a few fixes for the hrtimer code in RT linux
(v5.2.9-rt3-rebase).

The patch #2 contains a error I managed to reproduce. The other two are
were found while looking at the code.

Cheers,

Julien Grall (3):
  hrtimer: Use READ_ONCE to access timer->base in
    hrimer_grab_expiry_lock()
  hrtimer: Don't grab the expiry lock for non-soft hrtimer
  hrtimer: Prevent using uninitialized spin_lock in
    hrtimer_grab_expiry_lock()

 kernel/time/hrtimer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.11.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock()
  2019-08-21  9:24 [RT PATCH 0/3] hrtimer: RT fixes for hrtimer_grab_expiry_lock() Julien Grall
@ 2019-08-21  9:24 ` Julien Grall
  2019-08-21 13:44   ` Sebastian Andrzej Siewior
  2019-08-23  2:12   ` [tip: timers/core] hrtimer: Protect lockless access to timer->base tip-bot2 for Julien Grall
  2019-08-21  9:24 ` [RT PATCH 2/3] hrtimer: Don't grab the expiry lock for non-soft hrtimer Julien Grall
  2019-08-21  9:24 ` [RT PATCH 3/3] hrtimer: Prevent using uninitialized spin_lock in hrtimer_grab_expiry_lock() Julien Grall
  2 siblings, 2 replies; 16+ messages in thread
From: Julien Grall @ 2019-08-21  9:24 UTC (permalink / raw)
  To: linux-rt-users; +Cc: tglx, linux-kernel, maz, bigeasy, rostedt, Julien Grall

The update to timer->base is protected by the base->cpu_base->lock().
However, hrtimer_grab_expirty_lock() does not access it with the lock.

So it would theorically be possible to have timer->base changed under
our feet. We need to prevent the compiler to refetch timer->base so the
check and the access is performed on the same base.

Other access of timer->base are either done with a lock or protected
with READ_ONCE(). So use READ_ONCE() in hrtimer_grab_expirty_lock().

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

This is rather theoritical so far as I don't have a reproducer for this.
---
 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 7d7db8802131..b869e816e96a 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -932,7 +932,7 @@ EXPORT_SYMBOL_GPL(hrtimer_forward);
 
 void hrtimer_grab_expiry_lock(const struct hrtimer *timer)
 {
-	struct hrtimer_clock_base *base = timer->base;
+	struct hrtimer_clock_base *base = READ_ONCE(timer->base);
 
 	if (base && base->cpu_base) {
 		spin_lock(&base->cpu_base->softirq_expiry_lock);
-- 
2.11.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RT PATCH 2/3] hrtimer: Don't grab the expiry lock for non-soft hrtimer
  2019-08-21  9:24 [RT PATCH 0/3] hrtimer: RT fixes for hrtimer_grab_expiry_lock() Julien Grall
  2019-08-21  9:24 ` [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock() Julien Grall
@ 2019-08-21  9:24 ` Julien Grall
  2019-08-21 13:49   ` Sebastian Andrzej Siewior
  2019-08-21  9:24 ` [RT PATCH 3/3] hrtimer: Prevent using uninitialized spin_lock in hrtimer_grab_expiry_lock() Julien Grall
  2 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2019-08-21  9:24 UTC (permalink / raw)
  To: linux-rt-users; +Cc: tglx, linux-kernel, maz, bigeasy, rostedt, Julien Grall

There are no guarantee the hrtimer_cancel() will be called on the same
CPU as the non-soft hrtimer is running on so the following scenario
can happen.

CPU0                                         |  CPU1
                                             |
                                             | hrtimer_interrupt()
                                             |   raw_spin_lock_irqsave(&cpu_save->lock)
hrtimer_cancel()                             |      __run_hrtimer_run_queues()
  hrtimer_try_to_cancel()                    |      __run_hrtimer()
    lock_hrtimer_base()                      |        base->running = timer;
                                             |        raw_spin_unlock_irqrestore(&cpu_save->lock)
      raw_spin_lock_irqsave(cpu_base->lock)  |        fn(timer);
    hrtimer_callback_running()               |

hrtimer_callback_running() will be returning true as the callback is
running somewhere else. This means hrtimer_try_to_cancel() would return -1.
Therefore hrtimer_grab_expiry_lock() would be called.

non-soft hrtimer may be used when the timer needs to be manipulated from
a non-preemptible context. This is for instance the case of KVM Arm
timers. The following splat can be seen in the log:

[  157.449545] 000: BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:968
[  157.449569] 000: in_atomic(): 1, irqs_disabled(): 0, pid: 990, name: kvm-vcpu-1
[  157.449579] 000: 2 locks held by kvm-vcpu-1/990:
[  157.449592] 000:  #0: 00000000c2fc8217 (&vcpu->mutex){+.+.}, at: kvm_vcpu_ioctl+0x70/0xae0
[  157.449638] 000:  #1: 0000000096863801 (&cpu_base->softirq_expiry_lock){+.+.}, at: hrtimer_grab_expiry_lock+0x24/0x40
[  157.449677] 000: Preemption disabled at:
[  157.449679] 000: [<ffff0000111a4538>] schedule+0x30/0xd8
[  157.449702] 000: CPU: 0 PID: 990 Comm: kvm-vcpu-1 Tainted: G        W 5.2.0-rt1-00001-gd368139e892f #104
[  157.449712] 000: Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jan 23 2017
[  157.449718] 000: Call trace:
[  157.449722] 000:  dump_backtrace+0x0/0x130
[  157.449730] 000:  show_stack+0x14/0x20
[  157.449738] 000:  dump_stack+0xbc/0x104
[  157.449747] 000:  ___might_sleep+0x198/0x238
[  157.449756] 000:  rt_spin_lock+0x5c/0x70
[  157.449765] 000:  hrtimer_grab_expiry_lock+0x24/0x40
[  157.449773] 000:  hrtimer_cancel+0x1c/0x38
[  157.449780] 000:  kvm_timer_vcpu_load+0x78/0x3e0

An hrtimer is always either running in softirq or not. This cannot be
changed after it is instantiated. So we can rely on timer->is_soft
for checking whether the lock can be grabbed.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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 b869e816e96a..119414a2f59c 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 (base && base->cpu_base) {
+	if (timer->is_soft && base && base->cpu_base) {
 		spin_lock(&base->cpu_base->softirq_expiry_lock);
 		spin_unlock(&base->cpu_base->softirq_expiry_lock);
 	}
-- 
2.11.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RT PATCH 3/3] hrtimer: Prevent using uninitialized spin_lock in hrtimer_grab_expiry_lock()
  2019-08-21  9:24 [RT PATCH 0/3] hrtimer: RT fixes for hrtimer_grab_expiry_lock() Julien Grall
  2019-08-21  9:24 ` [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock() Julien Grall
  2019-08-21  9:24 ` [RT PATCH 2/3] hrtimer: Don't grab the expiry lock for non-soft hrtimer Julien Grall
@ 2019-08-21  9:24 ` Julien Grall
  2019-08-21 14:02   ` Thomas Gleixner
  2019-08-23  2:12   ` [tip: timers/core] hrtimer: Don't take expiry_lock when timer is currently migrated tip-bot2 for Julien Grall
  2 siblings, 2 replies; 16+ messages in thread
From: Julien Grall @ 2019-08-21  9:24 UTC (permalink / raw)
  To: linux-rt-users; +Cc: tglx, linux-kernel, maz, bigeasy, rostedt, Julien Grall

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 <julien.grall@arm.com>

---

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock()
  2019-08-21  9:24 ` [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock() Julien Grall
@ 2019-08-21 13:44   ` Sebastian Andrzej Siewior
  2019-08-21 13:50     ` Thomas Gleixner
  2019-08-23  2:12   ` [tip: timers/core] hrtimer: Protect lockless access to timer->base tip-bot2 for Julien Grall
  1 sibling, 1 reply; 16+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-08-21 13:44 UTC (permalink / raw)
  To: Julien Grall; +Cc: linux-rt-users, tglx, linux-kernel, maz, rostedt

On 2019-08-21 10:24:07 [+0100], Julien Grall wrote:
> The update to timer->base is protected by the base->cpu_base->lock().
> However, hrtimer_grab_expirty_lock() does not access it with the lock.
> 
> So it would theorically be possible to have timer->base changed under
> our feet. We need to prevent the compiler to refetch timer->base so the
> check and the access is performed on the same base.

It is not a problem if the timer's bases changes. We get here because we
want to help the timer to complete its callback.
The base can only change if the timer gets re-armed on another CPU which
means is completed callback. In every case we can cancel the timer on
the next iteration.

Sebastian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RT PATCH 2/3] hrtimer: Don't grab the expiry lock for non-soft hrtimer
  2019-08-21  9:24 ` [RT PATCH 2/3] hrtimer: Don't grab the expiry lock for non-soft hrtimer Julien Grall
@ 2019-08-21 13:49   ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 16+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-08-21 13:49 UTC (permalink / raw)
  To: Julien Grall; +Cc: linux-rt-users, tglx, linux-kernel, maz, rostedt

On 2019-08-21 10:24:08 [+0100], Julien Grall wrote:
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index b869e816e96a..119414a2f59c 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 (base && base->cpu_base) {
> +	if (timer->is_soft && base && base->cpu_base) {
>  		spin_lock(&base->cpu_base->softirq_expiry_lock);
>  		spin_unlock(&base->cpu_base->softirq_expiry_lock);
>  	}

right, much simpler.

Sebastian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock()
  2019-08-21 13:44   ` Sebastian Andrzej Siewior
@ 2019-08-21 13:50     ` Thomas Gleixner
  2019-08-21 13:59       ` Sebastian Andrzej Siewior
  2019-09-26 21:47       ` Eric Dumazet
  0 siblings, 2 replies; 16+ messages in thread
From: Thomas Gleixner @ 2019-08-21 13:50 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Julien Grall, linux-rt-users, linux-kernel, maz, rostedt

On Wed, 21 Aug 2019, Sebastian Andrzej Siewior wrote:

> On 2019-08-21 10:24:07 [+0100], Julien Grall wrote:
> > The update to timer->base is protected by the base->cpu_base->lock().
> > However, hrtimer_grab_expirty_lock() does not access it with the lock.
> > 
> > So it would theorically be possible to have timer->base changed under
> > our feet. We need to prevent the compiler to refetch timer->base so the
> > check and the access is performed on the same base.
> 
> It is not a problem if the timer's bases changes. We get here because we
> want to help the timer to complete its callback.
> The base can only change if the timer gets re-armed on another CPU which
> means is completed callback. In every case we can cancel the timer on
> the next iteration.

It _IS_ a problem when the base changes and the compiler reloads

   CPU0	  	       	   	CPU1
   base = timer->base;

   lock(base->....);
				switch base

   reload
	base = timer->base;

   unlock(base->....);

See?

   

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock()
  2019-08-21 13:50     ` Thomas Gleixner
@ 2019-08-21 13:59       ` Sebastian Andrzej Siewior
  2019-09-26 21:47       ` Eric Dumazet
  1 sibling, 0 replies; 16+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-08-21 13:59 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Julien Grall, linux-rt-users, linux-kernel, maz, rostedt

On 2019-08-21 15:50:33 [+0200], Thomas Gleixner wrote:
> On Wed, 21 Aug 2019, Sebastian Andrzej Siewior wrote:
> 
> > On 2019-08-21 10:24:07 [+0100], Julien Grall wrote:
> > > The update to timer->base is protected by the base->cpu_base->lock().
> > > However, hrtimer_grab_expirty_lock() does not access it with the lock.
> > > 
> > > So it would theorically be possible to have timer->base changed under
> > > our feet. We need to prevent the compiler to refetch timer->base so the
> > > check and the access is performed on the same base.
> > 
> > It is not a problem if the timer's bases changes. We get here because we
> > want to help the timer to complete its callback.
> > The base can only change if the timer gets re-armed on another CPU which
> > means is completed callback. In every case we can cancel the timer on
> > the next iteration.
> 
> It _IS_ a problem when the base changes and the compiler reloads
> 
>    CPU0	  	       	   	CPU1
>    base = timer->base;
> 
>    lock(base->....);
> 				switch base
> 
>    reload
> 	base = timer->base;
> 
>    unlock(base->....);
> 
> See?
so read_once() it is then.

Sebastian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RT PATCH 3/3] hrtimer: Prevent using uninitialized spin_lock in hrtimer_grab_expiry_lock()
  2019-08-21  9:24 ` [RT PATCH 3/3] hrtimer: Prevent using uninitialized spin_lock in hrtimer_grab_expiry_lock() Julien Grall
@ 2019-08-21 14:02   ` Thomas Gleixner
  2019-08-22 10:59     ` Julien Grall
  2019-08-23  2:12   ` [tip: timers/core] hrtimer: Don't take expiry_lock when timer is currently migrated tip-bot2 for Julien Grall
  1 sibling, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2019-08-21 14:02 UTC (permalink / raw)
  To: Julien Grall; +Cc: linux-rt-users, linux-kernel, maz, bigeasy, rostedt

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RT PATCH 3/3] hrtimer: Prevent using uninitialized spin_lock in hrtimer_grab_expiry_lock()
  2019-08-21 14:02   ` Thomas Gleixner
@ 2019-08-22 10:59     ` Julien Grall
  0 siblings, 0 replies; 16+ messages in thread
From: Julien Grall @ 2019-08-22 10:59 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: linux-rt-users, linux-kernel, maz, bigeasy, rostedt

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [tip: timers/core] hrtimer: Don't take expiry_lock when timer is currently migrated
  2019-08-21  9:24 ` [RT PATCH 3/3] hrtimer: Prevent using uninitialized spin_lock in hrtimer_grab_expiry_lock() Julien Grall
  2019-08-21 14:02   ` Thomas Gleixner
@ 2019-08-23  2:12   ` tip-bot2 for Julien Grall
  2019-09-04 14:15     ` [PATCH] hrtimer: Add a missing bracket and hide `migration_base' on !SMP Sebastian Andrzej Siewior
  1 sibling, 1 reply; 16+ messages in thread
From: tip-bot2 for Julien Grall @ 2019-08-23  2:12 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: linux-kernel, Thomas Gleixner, 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 <julien.grall@arm.com>
AuthorDate:    Wed, 21 Aug 2019 10:24:09 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
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 <julien.grall@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20190821092409.13225-4-julien.grall@arm.com


---
 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;
 	}

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [tip: timers/core] hrtimer: Protect lockless access to timer->base
  2019-08-21  9:24 ` [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock() Julien Grall
  2019-08-21 13:44   ` Sebastian Andrzej Siewior
@ 2019-08-23  2:12   ` tip-bot2 for Julien Grall
  1 sibling, 0 replies; 16+ messages in thread
From: tip-bot2 for Julien Grall @ 2019-08-23  2:12 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: linux-kernel, Thomas Gleixner, Julien Grall

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     dd2261ed45aaeddeb77768f291d604179bcab096
Gitweb:        https://git.kernel.org/tip/dd2261ed45aaeddeb77768f291d604179bcab096
Author:        Julien Grall <julien.grall@arm.com>
AuthorDate:    Wed, 21 Aug 2019 10:24:07 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 21 Aug 2019 16:10:01 +02:00

hrtimer: Protect lockless access to timer->base

The update to timer->base is protected by the base->cpu_base->lock().
However, hrtimer_cancel_wait_running() does access it lockless.  So the
compiler is allowed to refetch timer->base which can cause havoc when the
timer base is changed concurrently.

Use READ_ONCE() to prevent this.

[ tglx: Adapted from a RT patch ]

Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/20190821092409.13225-2-julien.grall@arm.com
---
 kernel/time/hrtimer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 8333537..f48864e 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1214,7 +1214,8 @@ static void hrtimer_sync_wait_running(struct hrtimer_cpu_base *cpu_base,
  */
 void hrtimer_cancel_wait_running(const struct hrtimer *timer)
 {
-	struct hrtimer_clock_base *base = timer->base;
+	/* 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) {
 		cpu_relax();

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] hrtimer: Add a missing bracket and hide `migration_base' on !SMP
  2019-08-23  2:12   ` [tip: timers/core] hrtimer: Don't take expiry_lock when timer is currently migrated tip-bot2 for Julien Grall
@ 2019-09-04 14:15     ` Sebastian Andrzej Siewior
  2019-09-04 14:39       ` Thomas Gleixner
  0 siblings, 1 reply; 16+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-09-04 14:15 UTC (permalink / raw)
  To: linux-kernel; +Cc: Thomas Gleixner, Julien Grall

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 <bigeasy@linutronix.de>
---
 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] hrtimer: Add a missing bracket and hide `migration_base' on !SMP
  2019-09-04 14:15     ` [PATCH] hrtimer: Add a missing bracket and hide `migration_base' on !SMP Sebastian Andrzej Siewior
@ 2019-09-04 14:39       ` Thomas Gleixner
  2019-09-04 14:55         ` [PATCH v2] " Sebastian Andrzej Siewior
  0 siblings, 1 reply; 16+ messages in thread
From: Thomas Gleixner @ 2019-09-04 14:39 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior; +Cc: linux-kernel, Julien Grall

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 <bigeasy@linutronix.de>
> ---
>  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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] hrtimer: Add a missing bracket and hide `migration_base' on !SMP
  2019-09-04 14:39       ` Thomas Gleixner
@ 2019-09-04 14:55         ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 16+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-09-04 14:55 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: linux-kernel, Julien Grall

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 <bigeasy@linutronix.de>
---
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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock()
  2019-08-21 13:50     ` Thomas Gleixner
  2019-08-21 13:59       ` Sebastian Andrzej Siewior
@ 2019-09-26 21:47       ` Eric Dumazet
  1 sibling, 0 replies; 16+ messages in thread
From: Eric Dumazet @ 2019-09-26 21:47 UTC (permalink / raw)
  To: Thomas Gleixner, Sebastian Andrzej Siewior
  Cc: Julien Grall, linux-rt-users, linux-kernel, maz, rostedt



On 8/21/19 6:50 AM, Thomas Gleixner wrote:
> On Wed, 21 Aug 2019, Sebastian Andrzej Siewior wrote:
> 
>> On 2019-08-21 10:24:07 [+0100], Julien Grall wrote:
>>> The update to timer->base is protected by the base->cpu_base->lock().
>>> However, hrtimer_grab_expirty_lock() does not access it with the lock.
>>>
>>> So it would theorically be possible to have timer->base changed under
>>> our feet. We need to prevent the compiler to refetch timer->base so the
>>> check and the access is performed on the same base.
>>
>> It is not a problem if the timer's bases changes. We get here because we
>> want to help the timer to complete its callback.
>> The base can only change if the timer gets re-armed on another CPU which
>> means is completed callback. In every case we can cancel the timer on
>> the next iteration.
> 
> It _IS_ a problem when the base changes and the compiler reloads
> 
>    CPU0	  	       	   	CPU1
>    base = timer->base;
> 
>    lock(base->....);
> 				switch base
> 
>    reload
> 	base = timer->base;
> 
>    unlock(base->....);
> 

It seems we could hit a similar problem in lock_hrtimer_base()

 base = timer->base;

 if (likely(base != &migration_base)) {

     <reload base : could point to &migration_base>

     raw_spin_lock_irqsave(&base->cpu_base->lock, *flags);

Probably not a big deal, since migration_base-cpu_base->lock can be locked just fine,
(without lockdep complaining that the lock has not been initialized since we use raw_ variant),
but this could cause unnecessary false sharing.


diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 0d4dc241c0fb498036c91a571e65cb00f5d19ba6..fa881c03e0a1a351186a8d8f798dd7471067a951 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -164,7 +164,7 @@ struct hrtimer_clock_base *lock_hrtimer_base(const struct hrtimer *timer,
 	struct hrtimer_clock_base *base;
 
 	for (;;) {
-		base = timer->base;
+		base = READ_ONCE(timer->base);
 		if (likely(base != &migration_base)) {
 			raw_spin_lock_irqsave(&base->cpu_base->lock, *flags);
 			if (likely(base == timer->base))



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-21  9:24 [RT PATCH 0/3] hrtimer: RT fixes for hrtimer_grab_expiry_lock() Julien Grall
2019-08-21  9:24 ` [RT PATCH 1/3] hrtimer: Use READ_ONCE to access timer->base in hrimer_grab_expiry_lock() Julien Grall
2019-08-21 13:44   ` Sebastian Andrzej Siewior
2019-08-21 13:50     ` Thomas Gleixner
2019-08-21 13:59       ` Sebastian Andrzej Siewior
2019-09-26 21:47       ` Eric Dumazet
2019-08-23  2:12   ` [tip: timers/core] hrtimer: Protect lockless access to timer->base tip-bot2 for Julien Grall
2019-08-21  9:24 ` [RT PATCH 2/3] hrtimer: Don't grab the expiry lock for non-soft hrtimer Julien Grall
2019-08-21 13:49   ` Sebastian Andrzej Siewior
2019-08-21  9:24 ` [RT PATCH 3/3] hrtimer: Prevent using uninitialized spin_lock in hrtimer_grab_expiry_lock() Julien Grall
2019-08-21 14:02   ` Thomas Gleixner
2019-08-22 10:59     ` Julien Grall
2019-08-23  2:12   ` [tip: timers/core] hrtimer: Don't take expiry_lock when timer is currently migrated tip-bot2 for Julien Grall
2019-09-04 14:15     ` [PATCH] hrtimer: Add a missing bracket and hide `migration_base' on !SMP Sebastian Andrzej Siewior
2019-09-04 14:39       ` Thomas Gleixner
2019-09-04 14:55         ` [PATCH v2] " Sebastian Andrzej Siewior

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git