Messages in this thread | | | Date | Sat, 27 May 2017 09:31:39 +0200 | From | Ingo Molnar <> | Subject | Re: [RFC][PATCH 1/4] time: Fix clock->read(clock) race around clocksource changes |
| |
* John Stultz <john.stultz@linaro.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@linutronix.de> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Miroslav Lichvar <mlichvar@redhat.com> > Cc: Richard Cochran <richardcochran@gmail.com> > Cc: Prarit Bhargava <prarit@redhat.com> > Cc: Stephen Boyd <stephen.boyd@linaro.org> > Cc: Daniel Mentz <danielmentz@google.com> > Signed-off-by: John Stultz <john.stultz@linaro.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@kernel.org>
Thanks,
Ingo
| |