linux-kernel - Re: [RFC][PATCH 1/4] time: Fix clock->read(clock) race around clocksource changes

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 27 May 2017 09:31:39 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Miroslav Lichvar <mlichvar@...hat.com>,
        Richard Cochran <richardcochran@...il.com>,
        Prarit Bhargava <prarit@...hat.com>,
        Stephen Boyd <stephen.boyd@...aro.org>,
        Daniel Mentz <danielmentz@...gle.com>
Subject: Re: [RFC][PATCH 1/4] time: Fix clock->read(clock) race around
 clocksource changes


* John Stultz <john.stultz@...aro.org> wrote:

> In some testing on arm64 platforms, I was seeing null ptr
> crashes in the kselftest/timers clocksource-switch test.
> 
> This was happening in a read function like:
> u64 clocksource_mmio_readl_down(struct clocksource *c)
> {
>     return ~(u64)readl_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
> }
> 
> Where the callers enter the seqlock, and then call something
> like:
>     cycle_now = tkr->read(tkr->clock);
> 
> The problem seeming to be that since the read and clock
> references are happening separately, its possible the
> clocksource change happens in between and we end up calling the
> old read with the new clocksource, (or vice-versa) which causes
> the to_mmio_clksrc() in the read function to run off into space.

Maybe:

  s/clock/->clock pointer
  s/read/old ->read() function

for increased clarity - because both are ambiguous in an English sentence.

> 
> This patch tries to address the issue by providing a helper
> function that atomically reads the clock value and then calls
> the clock->read(clock) call so that we always call the read
> funciton with the appropriate clocksource and don't accidentally
> mix them.

s/call/function

because 'call' is used both as a verb and as noun here, making for a difficult 
read.

> 
> The one exception where this helper isn't necessary is for the
> fast-timekepers which use their own locking and update logic
> to the tkr structures.
> 
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Miroslav Lichvar <mlichvar@...hat.com>
> Cc: Richard Cochran <richardcochran@...il.com>
> Cc: Prarit Bhargava <prarit@...hat.com>
> Cc: Stephen Boyd <stephen.boyd@...aro.org>
> Cc: Daniel Mentz <danielmentz@...gle.com>
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
>  kernel/time/timekeeping.c | 40 +++++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 9652bc5..abc1968 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -118,6 +118,26 @@ static inline void tk_update_sleep_time(struct timekeeper *tk, ktime_t delta)
>  	tk->offs_boot = ktime_add(tk->offs_boot, delta);
>  }
>  
> +/*
> + * tk_clock_read - atomic clocksource read() helper
> + *
> + * This helper is necessary to use in the read paths as, while the seqlock
> + * ensures we don't return a bad value while structures are updated, it
> + * doesn't protect from potential crashes. There is the possibility that
> + * the tkr's clocksource may change between the read reference, and the
> + * clock refernce passed to the read function.  This can cause crashes if
> + * the wrong clocksource is passed to the wrong read function.
> + * This isn't necessary to use when holding the timekeeper_lock or doing
> + * a read of the fast-timekeeper tkrs (which is protected by its own locking
> + * and update logic).

typo:

  s/refernce/reference

I'd also do:

  s/as/because

because while 'as' is not wrong, it's somewhat harder to parse on first reading.

Other than that:

  Acked-by: Ingo Molnar <mingo@...nel.org>

Thanks,

	Ingo