From patchwork Thu Jun 1 03:07:56 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Stultz X-Patchwork-Id: 100811 Delivered-To: patches@linaro.org Received: by 10.140.96.100 with SMTP id j91csp612358qge; Wed, 31 May 2017 20:08:03 -0700 (PDT) X-Received: by 10.98.29.79 with SMTP id d76mr34500109pfd.141.1496286483375; Wed, 31 May 2017 20:08:03 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1496286483; cv=none; d=google.com; s=arc-20160816; b=1DWHvtcaVqer4C8YrWTwm5Xdwzh44/nkFyu6eNA130cJ3GwWhWjdXfkt8hkosPeWY9 RDoj/uUGvSdjr2H/U0V8PUFKSwcwd1kLY8fztCemGwc+Erf6aiRx8uzWVf+oh3tLPmbc IU+vRSGWXA3juob2NAF5IVvodSOYV+p0HOXkSLEHY5kB7yCXmvpfIZKJ98UJWtj6GLu+ eIKbPIlyQjVnvFmBDWUINvlrt6qa9+1CVHdUD6jTFPuqsEYur0wcR24DhjNmtEkChD4X 7GpaAAp+vW6c5cxLzqVbcjMvuwxMAts88cb/0nHH4P+iGxYCXUl3x0P+t/4qrgJyAgkb 4f8Q== 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=fk9no4IRZh5X+9A6LInx7ePel4XeZQjOafNUpWg8ttk=; b=Sz6BAeRTfWosC+LkYKLfowXhrqEvASXQ0rkmGXnLyyl0GNS4f0uEwWVEIhyAH8V2s4 qyc9d1fmo8vaTS/uPcr2FaxzzQljsl/dPkd8234heIlO/pd4d1MX2Qh7B6bGGyLge8UM TRfPrRf0VR/bPOoooz6Wy2xYpZPrPwBMO+k6xBPrMzaCG1b2MDnx8RiMteQ6QqrUC063 dAYQFiKNmpiJcreFAW1yWLN5kRulzlXzutT3a1InXtHrv9/j/EHCaQ4l6EB+9fkDgihH 1R41o5J/YNEmj9GuX3u1/PS//P2oUeH5J7OJ3v6ukpeFTTAqgIHXjrnF1Tl9/yoOIbXF IOeg== 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::229 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-x229.google.com (mail-pf0-x229.google.com. [2607:f8b0:400e:c00::229]) by mx.google.com with ESMTPS id w124si18904103pgb.147.2017.05.31.20.08.03 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 31 May 2017 20:08:03 -0700 (PDT) Received-SPF: pass (google.com: domain of john.stultz@linaro.org designates 2607:f8b0:400e:c00::229 as permitted sender) client-ip=2607:f8b0:400e:c00::229; 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::229 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-x229.google.com with SMTP id n23so24595442pfb.2 for ; Wed, 31 May 2017 20:08:03 -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=fk9no4IRZh5X+9A6LInx7ePel4XeZQjOafNUpWg8ttk=; b=OTb0vG1a27miV01NCAdML5i91duhfYeXGCwINjWNm7xx9UHjQunN2/5uqJ+XxZf/Sd NkNIIC2T7DlKxngC8tmMq+nphVrCVVPWuMQHosct+iUjnA1yBQNV6zKnSKx8r03On1N5 T3EP+KxuoKBQ6PWF4m8U+7AHeXi93Kd4QA9CI= 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=fk9no4IRZh5X+9A6LInx7ePel4XeZQjOafNUpWg8ttk=; b=MnEFAo8OjqGkOJHBvPZgEafdxTWlTxPdmJ+O76h8GrVUD7N8IR8FzN6vgEJMSf5jeH RXjdxXvhbEXHCmU6y1nSAlmtqLxzW++b1FU1WxKFiHncggUldXlT3seremN9uTAhm/Cf VTjLa0hTI4B/XLBlDNqdeXPo7JMwCaBNXvRCRmXTzs2l6snC93GxaQboNe4binO2Pe2p 1MqoxulONi+fcuEvp0qJogK7QxoF5kaW+1shAwdNb97OfQUu6afSe3sm6GsR2sz3zbBQ ogzN0+fFIsSVrrdGCBLl33ZcE0wYkK465vVR0evKkvqg8PLr6FpX+BEZJRv3cR4yV+aN ax1Q== X-Gm-Message-State: AODbwcAmWqtcqIO3y/mBc02B/edUuRnlKmlN8Y7pkvYvQUcDm+oQeqiF AFkxW1e7jZ5qp60qAyo= X-Received: by 10.98.7.149 with SMTP id 21mr33576891pfh.54.1496286483024; Wed, 31 May 2017 20:08:03 -0700 (PDT) Return-Path: Received: from localhost.localdomain ([2601:1c2:1002:83f0:4e72:b9ff:fe99:466a]) by smtp.gmail.com with ESMTPSA id s68sm29137756pgc.5.2017.05.31.20.08.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 31 May 2017 20:08:02 -0700 (PDT) From: John Stultz To: lkml Cc: John Stultz , Thomas Gleixner , Ingo Molnar , Miroslav Lichvar , Richard Cochran , Prarit Bhargava , Stephen Boyd , Daniel Mentz , stable Subject: [PATCH 1/3 v2] time: Fix clock->read(clock) race around clocksource changes Date: Wed, 31 May 2017 20:07:56 -0700 Message-Id: <1496286478-13584-2-git-send-email-john.stultz@linaro.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1496286478-13584-1-git-send-email-john.stultz@linaro.org> References: <1496286478-13584-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 pointer references are happening separately, its possible the clocksource change happens in between and we end up calling the old ->read() function 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) function 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 Cc: stable Acked-by: Ingo Molnar Signed-off-by: John Stultz --- v2: Addressed Ingo's feedback on wording --- 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..797c73e 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 because, 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 reference 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