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 <[email protected]>
---
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
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
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?
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
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 <[email protected]>
AuthorDate: Wed, 21 Aug 2019 10:24:07 +01:00
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
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();
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))