hooks: Avoid torn accesses in the hook task

The hook task runs at the lowest priority, and both reads from and
writes to a data structure that can be changed out from under it at any
time. This is unsafe, and can cause missed hook events and double hook
events.

For example, the hook task reads __deferred_until[i], a 64-bit value, in
two 32-bit reads. If the hook task is interrupted during this read, and
the interruption changes the value, the hook task may read a totally
bogus value. This is rare, as overflows across this 32-bit boundary
don't happen often, but leads to unpredicable behavior when they do.

The writes the hook task does are also problematic, since for instance
the hook may have been rescheduled just after the slow old hook task
entered its if clause deciding to run the hook, but before it clobbered
__deferred_until[i] back to zero. Things get worse if the hook routine
pointer ever changes, though I don't think we're currently doing that
anywhere today.

Instead, disable interrupts while the hook data structure is being
manipulated as a makeshift lock around it. Remove the defer_new_call
variable as well, since the new deadline is now computed atomically (and
without the possibility of torn reads).

BUG=b:178660461, b:179062230
BRANCH=None
TEST=Run suspend 2500 on Boten, observe no spurious s0ix timeouts.

Signed-off-by: Evan Green <evgreen@chromium.org>
Change-Id: Iff0d596014e1e79dd9691d363fdc8e54bfe2dff0
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2805222
Reviewed-by: Wai-Hong Tam <waihong@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2816156
Tested-by: Abe Levkoy <alevkoy@chromium.org>
Reviewed-by: Abe Levkoy <alevkoy@chromium.org>
Commit-Queue: Abe Levkoy <alevkoy@chromium.org>

1 files changed, 12 insertions, 16 deletions

diff --git a/common/hooks.c b/common/hooks.c
index 4c2a4e48c6..a491d026a4 100644
--- a/common/hooks.c
+++ b/common/hooks.c
@@ -65,7 +65,6 @@ static const struct hook_ptrs hook_list[] = {
 };
 
 /* Times for deferrable functions */
-static int defer_new_call;
 static int hook_task_started;
 
 #ifdef CONFIG_HOOK_DEBUG
@@ -157,12 +156,6 @@ int hook_call_deferred(const struct deferred_data *data, int us)
 	} else {
 		/* Set alarm */
 		__deferred_until[i] = get_time().val + us;
-		/*
-		 * Flag that hook_call_deferred() has been called.  If the hook
-		 * task is already active, this will allow it to go through the
-		 * loop one more time before sleeping.
-		 */
-		defer_new_call = 1;
 
 		/* Wake task so it can re-sleep for the proper time */
 		if (hook_task_started)
@@ -191,20 +184,24 @@ void hook_task(void *u)
 		int next = 0;
 		int i;
 
+		interrupt_disable();
 		/* Handle deferred routines */
 		for (i = 0; i < DEFERRED_FUNCS_COUNT; i++) {
 			if (__deferred_until[i] && __deferred_until[i] < t) {
-				CPRINTS("hook call deferred 0x%pP",
-					__deferred_funcs[i].routine);
 				/*
 				 * Call deferred function.  Clear timer first,
 				 * so it can request itself be called later.
 				 */
 				__deferred_until[i] = 0;
+				interrupt_enable();
+				CPRINTS("hook call deferred 0x%pP",
+					__deferred_funcs[i].routine);
 				__deferred_funcs[i].routine();
+				interrupt_disable();
 			}
 		}
 
+		interrupt_enable();
 		if (t - last_tick >= HOOK_TICK_INTERVAL) {
 #ifdef CONFIG_HOOK_DEBUG
 			record_hook_delay(t, last_tick, HOOK_TICK_INTERVAL,
@@ -230,9 +227,7 @@ void hook_task(void *u)
 		if (last_tick + HOOK_TICK_INTERVAL > t)
 			next = last_tick + HOOK_TICK_INTERVAL - t;
 
-		/* Wake earlier if needed by a deferred routine */
-		defer_new_call = 0;
-
+		interrupt_disable();
 		for (i = 0; i < DEFERRED_FUNCS_COUNT && next > 0; i++) {
 			if (!__deferred_until[i])
 				continue;
@@ -243,12 +238,13 @@ void hook_task(void *u)
 				next = __deferred_until[i] - t;
 		}
 
+		interrupt_enable();
+
 		/*
-		 * If nothing is immediately pending, and hook_call_deferred()
-		 * hasn't been called since we started calculating next, sleep
-		 * until the next event.
+		 * If nothing is immediately pending, sleep until the next
+		 * event.
 		 */
-		if (next > 0 && !defer_new_call)
+		if (next > 0)
 			task_wait_event(next);
 	}
 }