From patchwork Sat May 27 03:33:52 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Stultz X-Patchwork-Id: 100585 Delivered-To: patches@linaro.org Received: by 10.140.96.100 with SMTP id j91csp536073qge; Fri, 26 May 2017 20:34:02 -0700 (PDT) X-Received: by 10.84.169.3 with SMTP id g3mr6084204plb.37.1495856042213; Fri, 26 May 2017 20:34:02 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1495856042; cv=none; d=google.com; s=arc-20160816; b=oNC4pI4G/+8Sb0cXBAu5KuFBBO+s/6ZdNIwAWoS02cE50LUYWyhQ0JxeW1v9oMb/bK RABPkREpo224URM9zmT2ZCbCF9XoVw8/kyRob5lL2gzKAp4cx332M/Llr5FmWjuIQCD1 NYtLwkQLtb6FMjlMmCIHYzDhbI5mQVdtlvG7FFHUnJ9l1n6k+Ss1Pk3bpVimudPqFU1j wVx0/FzY+iJR5JiEfdWhUWGk8MOV02GD85Ah2WgS15lOCO8hrO7ksfWt1KU9KEGMkZm0 LfG301PRJPM0cVbveaUsM2isu/al7UUYVi6EhRMeT2Q45fOdJAN/YDmbRP0QgIxD97z9 I39A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature:arc-authentication-results; bh=eSdmE153oTfW5bndjZq0gTghCKJMJhQKzVLNIb/AY/A=; b=E/3bQzsOCgq6XB3ezh4pE2RfJsYgCrZMz2ubG7w5SJhPO5m/fvCEbAA9G6k5svyFLv ScH/6z4b3JO1XJZmjaoitgrmvssSBuj1nP3FahJBh+ISMSLRomZgS/kbRG4Tu60ncfx6 8taLh3wKtdqMarng5tVsqXjkHpLmS2ODXWaqHsgMM5azSBGMtnk5pDtpnd3YhP9uCdRl SG4b7F7wvOPVw+uiwgQCUATUrk4JVNx6LKxmyrvh0Mut33bldcQB50l/vbGzZyDjx26J 7TwGE7j5Ib0rFApeJyM3VAvE0bRiFp9hYo+26HKnN8DpM5L9EPwBy4NdYTSqKCyEk/tE ZhSQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org; spf=pass (google.com: domain of john.stultz@linaro.org designates 2607:f8b0:400e:c00::233 as permitted sender) smtp.mailfrom=john.stultz@linaro.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from mail-pf0-x233.google.com (mail-pf0-x233.google.com. [2607:f8b0:400e:c00::233]) by mx.google.com with ESMTPS id x15si33756800plm.276.2017.05.26.20.34.02 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 26 May 2017 20:34:02 -0700 (PDT) Received-SPF: pass (google.com: domain of john.stultz@linaro.org designates 2607:f8b0:400e:c00::233 as permitted sender) client-ip=2607:f8b0:400e:c00::233; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org; spf=pass (google.com: domain of john.stultz@linaro.org designates 2607:f8b0:400e:c00::233 as permitted sender) smtp.mailfrom=john.stultz@linaro.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: by mail-pf0-x233.google.com with SMTP id n23so25105563pfb.2 for ; Fri, 26 May 2017 20:34:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=eSdmE153oTfW5bndjZq0gTghCKJMJhQKzVLNIb/AY/A=; b=FhptszYlEidQm5OOkM464E4/QFiO3tzct+SKa55jrhC0VNsNSZThdxuxKlbk9+G3nr 8ExqTBptcjmgHQANtn9xqae144hs8bq2TpF3ChJUk10AZbLR3Q1QzhYxvVcNZcxYd39z ILY8Z9SoSLBfKcQXdJB4Lp6GT1xwsM7QmGKtc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=eSdmE153oTfW5bndjZq0gTghCKJMJhQKzVLNIb/AY/A=; b=FwYt+3FyvExvUKREvH9P/3C53DCcQOfLGsIfAxn4Ft9eN9Z4ttYalvQ5PNZqSOpj5z brCZ6MrpEUo3OMwy7IhFKqXCGLKqz7CqKFDHiD6MhD16HOmhqrk+f+rqqOSGrrS+/5A9 17Cnav6TUeK2QfBP7bVtc+P0ad90as1D1XjfxgAuw3gpZwrCeKEj6syhlLdTWdoc6apP AxcdB3OdS7bjv+kbj5jifwAiK/mihvpCc6q6yBir0+RRbbUlLBdZYwpiVCY3X2cEl+BX 6CdU3J8e5yMhbPHDnjBZv26cuqsfB3GnBwcVs1ICvyD7L3M4OSEDoqML6tMEeyC1osBQ LXLw== X-Gm-Message-State: AODbwcBfVw+3u+8Er/1o+JZM268/+DvRzzmeaHLjLFIQguJBHNBFPaiY xKsus+74KDIR7kSEp9Q= X-Received: by 10.84.137.1 with SMTP id 1mr63688636plm.128.1495856041914; Fri, 26 May 2017 20:34:01 -0700 (PDT) Return-Path: Received: from localhost.localdomain ([2601:1c2:1002:83f0:4e72:b9ff:fe99:466a]) by smtp.gmail.com with ESMTPSA id d185sm3413255pgc.39.2017.05.26.20.34.00 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 26 May 2017 20:34:00 -0700 (PDT) From: John Stultz To: lkml Cc: John Stultz , Thomas Gleixner , Ingo Molnar , Miroslav Lichvar , Richard Cochran , Prarit Bhargava , Stephen Boyd , Daniel Mentz Subject: [RFC][PATCH 1/4] time: Fix clock->read(clock) race around clocksource changes Date: Fri, 26 May 2017 20:33:52 -0700 Message-Id: <1495856035-6622-2-git-send-email-john.stultz@linaro.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1495856035-6622-1-git-send-email-john.stultz@linaro.org> References: <1495856035-6622-1-git-send-email-john.stultz@linaro.org> 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. 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. 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 Cc: Ingo Molnar Cc: Miroslav Lichvar Cc: Richard Cochran Cc: Prarit Bhargava Cc: Stephen Boyd Cc: Daniel Mentz Signed-off-by: John Stultz --- kernel/time/timekeeping.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-) -- 2.7.4 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). + */ +static inline u64 tk_clock_read(struct tk_read_base *tkr) +{ + struct clocksource *clock = READ_ONCE(tkr->clock); + + return clock->read(clock); +} + #ifdef CONFIG_DEBUG_TIMEKEEPING #define WARNING_FREQ (HZ*300) /* 5 minute rate-limiting */ @@ -175,7 +195,7 @@ static inline u64 timekeeping_get_delta(struct tk_read_base *tkr) */ do { seq = read_seqcount_begin(&tk_core.seq); - now = tkr->read(tkr->clock); + now = tk_clock_read(tkr); last = tkr->cycle_last; mask = tkr->mask; max = tkr->clock->max_cycles; @@ -209,7 +229,7 @@ static inline u64 timekeeping_get_delta(struct tk_read_base *tkr) u64 cycle_now, delta; /* read clocksource */ - cycle_now = tkr->read(tkr->clock); + cycle_now = tk_clock_read(tkr); /* calculate the delta since the last update_wall_time */ delta = clocksource_delta(cycle_now, tkr->cycle_last, tkr->mask); @@ -240,7 +260,7 @@ static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock) tk->tkr_mono.clock = clock; tk->tkr_mono.read = clock->read; tk->tkr_mono.mask = clock->mask; - tk->tkr_mono.cycle_last = tk->tkr_mono.read(clock); + tk->tkr_mono.cycle_last = tk_clock_read(&tk->tkr_mono); tk->tkr_raw.clock = clock; tk->tkr_raw.read = clock->read; @@ -477,7 +497,7 @@ static void halt_fast_timekeeper(struct timekeeper *tk) struct tk_read_base *tkr = &tk->tkr_mono; memcpy(&tkr_dummy, tkr, sizeof(tkr_dummy)); - cycles_at_suspend = tkr->read(tkr->clock); + cycles_at_suspend = tk_clock_read(tkr); tkr_dummy.read = dummy_clock_read; update_fast_timekeeper(&tkr_dummy, &tk_fast_mono); @@ -649,11 +669,10 @@ static void timekeeping_update(struct timekeeper *tk, unsigned int action) */ static void timekeeping_forward_now(struct timekeeper *tk) { - struct clocksource *clock = tk->tkr_mono.clock; u64 cycle_now, delta; u64 nsec; - cycle_now = tk->tkr_mono.read(clock); + cycle_now = tk_clock_read(&tk->tkr_mono); delta = clocksource_delta(cycle_now, tk->tkr_mono.cycle_last, tk->tkr_mono.mask); tk->tkr_mono.cycle_last = cycle_now; tk->tkr_raw.cycle_last = cycle_now; @@ -929,8 +948,7 @@ void ktime_get_snapshot(struct system_time_snapshot *systime_snapshot) do { seq = read_seqcount_begin(&tk_core.seq); - - now = tk->tkr_mono.read(tk->tkr_mono.clock); + now = tk_clock_read(&tk->tkr_mono); systime_snapshot->cs_was_changed_seq = tk->cs_was_changed_seq; systime_snapshot->clock_was_set_seq = tk->clock_was_set_seq; base_real = ktime_add(tk->tkr_mono.base, @@ -1108,7 +1126,7 @@ int get_device_system_crosststamp(int (*get_time_fn) * Check whether the system counter value provided by the * device driver is on the current timekeeping interval. */ - now = tk->tkr_mono.read(tk->tkr_mono.clock); + now = tk_clock_read(&tk->tkr_mono); interval_start = tk->tkr_mono.cycle_last; if (!cycle_between(interval_start, cycles, now)) { clock_was_set_seq = tk->clock_was_set_seq; @@ -1629,7 +1647,7 @@ void timekeeping_resume(void) * The less preferred source will only be tried if there is no better * usable source. The rtc part is handled separately in rtc core code. */ - cycle_now = tk->tkr_mono.read(clock); + cycle_now = tk_clock_read(&tk->tkr_mono); if ((clock->flags & CLOCK_SOURCE_SUSPEND_NONSTOP) && cycle_now > tk->tkr_mono.cycle_last) { u64 nsec, cyc_delta; @@ -2030,7 +2048,7 @@ void update_wall_time(void) #ifdef CONFIG_ARCH_USES_GETTIMEOFFSET offset = real_tk->cycle_interval; #else - offset = clocksource_delta(tk->tkr_mono.read(tk->tkr_mono.clock), + offset = clocksource_delta(tk_clock_read(&tk->tkr_mono), tk->tkr_mono.cycle_last, tk->tkr_mono.mask); #endif