[PATCH RFC] Fix data races in timers

[PATCH RFC] Fix data races in timers

[Paul E. McKenney]

Hello!

In CONFIG_KCSAN_STRICT=y mode, KCSAN finds (a very few) data races,
one set on the hlist_node ->pprev field and another set on tmigr_event
structure's ->ignore field.  Therefore, fix these applying READ_ONCE()
and WRITE_ONCE().

1.	Mark updates to hlist_node ->pprev field.

2.	Mark updates to tmigr_event ->field.

						Thanx, Paul

------------------------------------------------------------------------

 include/linux/list.h          |    6 +++---
 kernel/time/timer.c           |    2 +-
 kernel/time/timer_migration.c |    6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

[PATCH 1/2] timers: Mark updates to hlist_node ->pprev field

[Paul E. McKenney]

The hlist_unhashed_lockless() function locklessly samples the hlist_node
structure's ->pprev field, but detach_timer(), hlist_move_list(), and
hlist_splice_init() all use plain C-language stores to update this field,
despite the "The READ_ONCE() is paired with the various WRITE_ONCE() in
hlist helpers that are defined below" in the hlist_unhashed_lockless()
header comment.

Therefore use WRITE_ONCE() for these ->pprev updates.

KCSAN located this issue.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/list.h | 6 +++---
 kernel/time/timer.c  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/list.h b/include/linux/list.h
index 77fb62f79928..e3753695e76c 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -1172,7 +1172,7 @@ static inline void hlist_move_list(struct hlist_head *old,
 {
 	new->first = old->first;
 	if (new->first)
-		new->first->pprev = &new->first;
+		WRITE_ONCE(new->first->pprev, &new->first);
 	old->first = NULL;
 }
 
@@ -1189,10 +1189,10 @@ static inline void hlist_splice_init(struct hlist_head *from,
 				     struct hlist_head *to)
 {
 	if (to->first)
-		to->first->pprev = &last->next;
+		WRITE_ONCE(to->first->pprev, &last->next);
 	last->next = to->first;
 	to->first = from->first;
-	from->first->pprev = &to->first;
+	WRITE_ONCE(from->first->pprev, &to->first);
 	from->first = NULL;
 }
 
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index ae9abf14688e..42afdcb229d8 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -890,7 +890,7 @@ static inline void detach_timer(struct timer_list *timer, bool clear_pending)
 
 	__hlist_del(entry);
 	if (clear_pending)
-		entry->pprev = NULL;
+		WRITE_ONCE(entry->pprev, NULL);
 	entry->next = LIST_POISON2;
 }
 
-- 
2.40.1

[PATCH 2/2] timers/migration: Mark updates to tmigr_event ->field

[Paul E. McKenney]

The tmigr_event structure's ->ignore field are sometimes accessed
locklessly, but the __tmigr_cpu_activate(), tmigr_cpu_new_timer(),
and __tmigr_cpu_deactivate() functions do not mark accesses to this field.

Therefore, use READ_ONCE() for the tmigr_cpu_new_timer() function's
lockless load and WRITE_ONCE() for the __tmigr_cpu_activate() and
__tmigr_cpu_deactivate() functions' stores.

KCSAN located this issue.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
Cc: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Thomas Gleixner <tglx@kernel.org>
---
 kernel/time/timer_migration.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/time/timer_migration.c b/kernel/time/timer_migration.c
index 059d43355e65..f920e73fff51 100644
--- a/kernel/time/timer_migration.c
+++ b/kernel/time/timer_migration.c
@@ -715,7 +715,7 @@ static void __tmigr_cpu_activate(struct tmigr_cpu *tmc)
 
 	trace_tmigr_cpu_active(tmc);
 
-	tmc->cpuevt.ignore = true;
+	WRITE_ONCE(tmc->cpuevt.ignore, true);
 	WRITE_ONCE(tmc->wakeup, KTIME_MAX);
 
 	walk_groups(&tmigr_active_up, &data, tmc);
@@ -1258,7 +1258,7 @@ u64 tmigr_cpu_new_timer(u64 nextexp)
 	ret = READ_ONCE(tmc->wakeup);
 	if (nextexp != KTIME_MAX) {
 		if (nextexp != tmc->cpuevt.nextevt.expires ||
-		    tmc->cpuevt.ignore) {
+		    READ_ONCE(tmc->cpuevt.ignore)) {
 			ret = tmigr_new_timer(tmc, nextexp);
 			/*
 			 * Make sure the reevaluation of timers in idle path
@@ -1362,7 +1362,7 @@ static u64 __tmigr_cpu_deactivate(struct tmigr_cpu *tmc, u64 nextexp)
 	 * or CPU goes offline.
 	 */
 	if (nextexp != KTIME_MAX)
-		tmc->cpuevt.ignore = false;
+		WRITE_ONCE(tmc->cpuevt.ignore, false);
 
 	walk_groups(&tmigr_inactive_up, &data, tmc);
 	return data.firstexp;
-- 
2.40.1