Serialize tc_windup() calls and modification of some timehands members.

If a user thread from e.g. clock_settime(2) is in the midst of changing
the boottime or calling tc_windup() when it is interrupted by hardclock(9),
the timehands could be left in a damaged state.

So protect tc_windup() calls with a mutex, timecounter_mtx.  hardclock(9)
merely attempts to enter the mutex instead of spinning because it cannot
afford to wait around.  In practice hardclock(9) will skip tc_windup() very
rarely, and when it does skip there aren't any negative effects because the
skip indicates that a user thread is already calling, or about to call,
tc_windup() anyway.

Based on FreeBSD r303387 and NetBSD sys/kern/kern_tc.c,v1.30

Discussed with mpi@ and visa@.  Tons of nice technical detail about
lockless reads from visa@.

OK visa@

1 files changed, 20 insertions, 4 deletions

diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c
index ba8126bc33c..5fde04474a7 100644
--- a/sys/kern/kern_tc.c
+++ b/sys/kern/kern_tc.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_tc.c,v 1.35 2019/01/19 01:53:44 cheloha Exp $ */
+/*	$OpenBSD: kern_tc.c,v 1.36 2019/01/20 01:13:03 cheloha Exp $ */
 
 /*
  * Copyright (c) 2000 Poul-Henning Kamp <phk@FreeBSD.org>
@@ -24,6 +24,7 @@
 #include <sys/param.h>
 #include <sys/atomic.h>
 #include <sys/kernel.h>
+#include <sys/mutex.h>
 #include <sys/timeout.h>
 #include <sys/sysctl.h>
 #include <sys/syslog.h>
@@ -102,6 +103,12 @@ static struct timehands th0 = {
 	&th1
 };
 
+/*
+ * Protects writes to anything accessed during tc_windup().
+ * tc_windup() must be called before leaving this mutex.
+ */
+struct mutex timecounter_mtx = MUTEX_INITIALIZER(IPL_CLOCK);
+
 static struct timehands *volatile timehands = &th0;
 struct timecounter *timecounter = &dummy_timecounter;
 static struct timecounter *timecounters = &dummy_timecounter;
@@ -339,7 +346,6 @@ tc_getfrequency(void)
 /*
  * Step our concept of UTC, aka the realtime clock.
  * This is done by modifying our estimate of when we booted.
- * XXX: not locked.
  */
 void
 tc_setrealtimeclock(const struct timespec *ts)
@@ -347,15 +353,19 @@ tc_setrealtimeclock(const struct timespec *ts)
 	struct timespec ts2;
 	struct bintime bt, bt2;
 
+	mtx_enter(&timecounter_mtx);
 	binuptime(&bt2);
 	timespec2bintime(ts, &bt);
 	bintime_sub(&bt, &bt2);
 	bintime_add(&bt2, &timehands->th_boottime);
 	timehands->th_boottime = bt;
-	enqueue_randomness(ts->tv_sec);
 
 	/* XXX fiddle all the little crinkly bits around the fiords... */
 	tc_windup();
+	mtx_leave(&timecounter_mtx);
+
+	enqueue_randomness(ts->tv_sec);
+
 	if (timestepwarnings) {
 		bintime2timespec(&bt2, &ts2);
 		log(LOG_INFO, "Time stepped from %lld.%09ld to %lld.%09ld\n",
@@ -367,7 +377,6 @@ tc_setrealtimeclock(const struct timespec *ts)
 /*
  * Step the monotonic and realtime clocks, triggering any timeouts that
  * should have occurred across the interval.
- * XXX: not locked.
  */
 void
 tc_setclock(const struct timespec *ts)
@@ -390,6 +399,7 @@ tc_setclock(const struct timespec *ts)
 
 	enqueue_randomness(ts->tv_sec);
 
+	mtx_enter(&timecounter_mtx);
 	timespec2bintime(ts, &bt);
 	bintime_sub(&bt, &timehands->th_boottime);
 	bt2 = timehands->th_offset;
@@ -397,6 +407,7 @@ tc_setclock(const struct timespec *ts)
 
 	/* XXX fiddle all the little crinkly bits around the fiords... */
 	tc_windup();
+	mtx_leave(&timecounter_mtx);
 
 #ifndef SMALL_KERNEL
 	/* convert the bintime to ticks */
@@ -426,6 +437,8 @@ tc_windup(void)
 	u_int delta, ncount, ogen;
 	int i;
 
+	MUTEX_ASSERT_LOCKED(&timecounter_mtx);
+
 	/*
 	 * Make the next timehands a copy of the current one, but do not
 	 * overwrite the generation or next pointer.  While we update
@@ -605,8 +618,11 @@ tc_ticktock(void)
 
 	if (++count < tc_tick)
 		return;
+	if (!mtx_enter_try(&timecounter_mtx))
+		return;
 	count = 0;
 	tc_windup();
+	mtx_leave(&timecounter_mtx);
 }
 
 void