Skip to contentSkip to conversation

Patch "time: Fix clock->read(clock) race around clocksource changes" has been added to the 4.9-stable tree

PATCH [email protected] 1 participant 1 message Jun 26, 2017

Subsystems (2)
lore
Share & tools
lore

[email protected]

author maintainer Applied
#0 lore raw

This is a note to let you know that I've just added the patch titled

    time: Fix clock->read(clock) race around clocksource changes

to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
time-fix-clock-read-clock-race-around-clocksource-changes.patch
and it can be found in the queue-4.9 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From ceea5e3771ed2378668455fa21861bead7504df5 Mon Sep 17 00:00:00 2001

From: John Stultz <[email protected]>
Date: Thu, 8 Jun 2017 16:44:20 -0700
Subject: time: Fix clock->read(clock) race around clocksource changes

From: John Stultz <[email protected]>

commit ceea5e3771ed2378668455fa21861bead7504df5 upstream.

In tests, which excercise switching of clocksources, a NULL pointer dereference can be observed on AMR64 platforms in the clocksource read() function:

u64 clocksource_mmio_readl_down(struct clocksource *c)

{
	return ~(u64)readl_relaxed(to_mmio_clksrc(c)->reg) & c->mask;
}

This is called from the core timekeeping code via:

	cycle_now = tkr->read(tkr->clock);

tkr->read is the cached tkr->clock->read() function pointer. When the clocksource is changed then tkr->clock and tkr->read are updated sequentially. The code above results in a sequential load operation of tkr->read and tkr->clock as well.

If the store to tkr->clock hits between the loads of tkr->read and tkr->clock, then the old read() function is called with the new clock pointer. As a consequence the read() function dereferences a different data structure and the resulting 'reg' pointer can point anywhere including NULL.

This problem was introduced when the timekeeping code was switched over to use struct tk_read_base. Before that, it was theoretically possible as well when the compiler decided to reload clock in the code sequence:

     now = tk->clock->read(tk->clock);

Add a helper function which avoids the issue by reading tk_read_base->clock once into a local variable clk and then issue the read function via clk->read(clk). This guarantees that the read() function always gets the proper clocksource pointer handed in.

Since there is now no use for the tkr.read pointer, this patch also removes it, and to address stopping the fast timekeeper during suspend/resume, it introduces a dummy clocksource to use rather then just a dummy read function.

---
 include/linux/timekeeper_internal.h |    1 
 kernel/time/timekeeping.c           |   52 ++++++++++++++++++++++++------------
 2 files changed, 36 insertions(+), 17 deletions(-)
include/linux/timekeeper_internal.h +0 -1
@@ -29,7 +29,6 @@
  */
 struct tk_read_base {
 	struct clocksource	*clock;
-	cycle_t			(*read)(struct clocksource *cs);
 	cycle_t			mask;
 	cycle_t			cycle_last;
 	u32			mult;
kernel/time/timekeeping.c +36 -16

134 lines not on this page · raw message

Patches currently in stable-queue which might be from [email protected] are

queue-4.9/time-fix-clock-read-clock-race-around-clocksource-changes.patch queue-4.9/time-fix-clock_monotonic_raw-sub-nanosecond-accounting.patch