From ed211504929d72b05084dcc9e42c9a1dac45e7fe Mon Sep 17 00:00:00 2001 From: Douglas Mun Date: Sun, 23 Aug 2026 15:55:13 +0800 Subject: [PATCH] supervisor: watch ktimerd and edr_daemon, and restore priority on restart supervisor_watch() covered knetd only. ktimerd and the EDR daemon were unsupervised, and neither failure is visible: timer_softirq_run() drives the TCP timers, the DHCP renewal, the EDR periodic hooks and the CSPRNG reseed, so a dead ktimerd degrades all four silently. Capacity raised 4 -> 8. Found while wiring it: supervisor_restart() never restored priority. task_create_kernel() assigns PRIORITY_NORMAL unconditionally, so a restarted task came back demoted. This was invisible while knetd was the only watched task -- knetd IS PRIORITY_NORMAL, so restoring its priority and failing to restore it produce identical output. Both newly-watched tasks are PRIORITY_HIGH, so the latent bug would have started biting immediately: the task is alive and the restart counter rises, so every status surface reports a healthy recovery, and the only symptom is the timer bottom-half running behind interactive work. Fixed by recording the priority in supervisor_watch() and restoring it in supervisor_restart() before scheduler_add_task(). Set inside the restart function rather than at the call site, for the same reason the rate limit lives there: so no future restart path can skip it. edr_daemon_main() loses `static` (the supervisor stores a void(*)(void)); kernel.c still starts the daemon via edr_daemon_start(). It is watched after edr_daemon_start() returns rather than beside knetd/ktimerd, because the daemon does not exist until then. verify-supervisor.sh step 6: was a FALSE PASS, now grades. The new leg indexed the last sample with "${KT_PRI_SAMPLES[-1]}". macOS ships bash 3.2, which has no negative array subscripting, and under `set -u` that raises "bad array subscript" / "unbound variable" rather than yielding empty. That aborted the two assignments, leaving the operands unset; the `-ne` comparisons then failed on stderr WITHOUT stopping the script, so the step printed its "OK" line and the harness printed RESULT: PASS while grading nothing. The kernel was correct throughout -- the serial log showed ktimerd restarting at priority 3 as intended -- so the only evidence was a stderr line nobody read. That is the exact false-pass shape this suite exists to prevent. Fixed by computing the last index explicitly and guarding the parse: all four fields must be non-empty and numeric before any is compared, because `-ne` treats an empty operand as a syntax error and CONTINUES. Negative-controlled against the real serial log, since a fix that is not falsified is how the false pass shipped: - simulated demotion (post pri 3->2) -> FAIL "DEMOTED 3 -> 2" - ktimerd never restarted (same PID) -> FAIL "not restarted (vacuous)" - broken `ps -l` column layout -> FAIL "expected 2 rows, got 1" - wrong baseline (pre pri 3->2) -> FAIL "baseline not 3" - unmodified correct-kernel log -> OK Fault injection uses ktimerd, not knetd, for the reason above: knetd's PRIORITY_NORMAL cannot witness a demotion. The victim clears its OWN CAP_UNKILLABLE and calls task_terminate(self_pid) -- not task_exit(), which is inert for scheduler-run tasks -- so the capability check itself stays unmodified. Verified: make clean + rebuild 0 warnings under -Werror; full verify-supervisor.sh run PASSes all of steps 0-6, with step 6 reporting pre PID=17356 pri=3 -> post PID=47438 pri=3 (restart proven by the PID change, priority proven unchanged). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CEkhAhgTxbE5TgifyYf8v4 --- src/edr_daemon.c | 8 ++- src/edr_ml.h | 5 ++ src/kernel.c | 26 +++++++- src/shell.c | 9 +++ src/supervisor.c | 13 +++- src/supervisor.h | 21 +++++- src/test_tasks.c | 30 +++++++++ src/test_tasks.h | 3 + verify/verify-supervisor.sh | 127 +++++++++++++++++++++++++++++++++++- 9 files changed, 236 insertions(+), 6 deletions(-) diff --git a/src/edr_daemon.c b/src/edr_daemon.c index 05a43ad..e227496 100644 --- a/src/edr_daemon.c +++ b/src/edr_daemon.c @@ -299,7 +299,13 @@ static void report_statistics(void) { /** * @brief Main EDR daemon loop */ -static void edr_daemon_main(void) { +/* + * NOT static: the supervisor restarts a dead task by calling its entry point, + * so it needs a void(*)(void) it can store. Exposed via edr_ml.h alongside + * edr_daemon_start() rather than being called directly by anyone else -- + * kernel.c still starts the daemon through edr_daemon_start(). + */ +void edr_daemon_main(void) { task_t* self = task_current(); kprintf("[EDR DAEMON] Starting EDR background daemon (PID %d)\n", self->pid); diff --git a/src/edr_ml.h b/src/edr_ml.h index 9ccd972..19111d6 100644 --- a/src/edr_ml.h +++ b/src/edr_ml.h @@ -740,6 +740,11 @@ void edr_response_get_stats(uint32_t* total_responses, uint8_t* log_count); */ int edr_daemon_start(void); +/* The daemon's entry point. Exposed only so the supervisor can restart it + * (supervisor_watch needs the void(*)(void)); use edr_daemon_start() to + * create the task. */ +void edr_daemon_main(void); + /** * @brief Stop the EDR daemon process * diff --git a/src/kernel.c b/src/kernel.c index f870764..28637b1 100644 --- a/src/kernel.c +++ b/src/kernel.c @@ -1152,7 +1152,19 @@ void kernel_main(uint32_t magic, uint32_t info_ptr) { * Registered here, watched by task_supervisor below. */ supervisor_init(); - supervisor_watch("knetd", task_knetd, (uint32_t)pid_knetd); + supervisor_watch("knetd", task_knetd, (uint32_t)pid_knetd, PRIORITY_NORMAL); + + /* ktimerd is watched for the same reason knetd is, and the consequence of + * its death is broader: timer_softirq_run() drives the TCP timers, the DHCP + * renewal, the EDR periodic hooks and the CSPRNG reseed. Nothing notices it + * stop -- there is no "timer stalled" surface -- so an unwatched death is a + * slow, silent degradation rather than a visible fault. + * + * PRIORITY_HIGH is passed explicitly because task_create_kernel() assigns + * PRIORITY_NORMAL and kernel.c raises it separately below; the supervisor + * has to know the intended priority or it restores the wrong one. */ + supervisor_watch("ktimerd", task_ktimerd, (uint32_t)pid_ktimerd, + PRIORITY_HIGH); int pid_supervisor = task_create_kernel(task_supervisor, "supervisor"); if (pid_supervisor < 0) { @@ -1169,6 +1181,18 @@ void kernel_main(uint32_t magic, uint32_t info_ptr) { * as a line that simply is not there rather than as a failure. */ int pid_edr = edr_daemon_start(); + /* Watched here rather than beside knetd/ktimerd above because the daemon + * does not exist until edr_daemon_start() returns -- supervisor_watch() + * refuses a pid that is not live, so registering it earlier would silently + * leave the EDR unsupervised. + * + * Guarded on >= 0: edr_daemon_start() returns -1 on failure, and watching + * pid (uint32_t)-1 would register a slot that can never validate. */ + if (pid_edr >= 0) { + supervisor_watch("edr_daemon", edr_daemon_main, (uint32_t)pid_edr, + PRIORITY_HIGH); + } + /* Protect critical system processes with CAP_UNKILLABLE. * * These ORs are REDUNDANT today: task_create_kernel() grants CAP_ALL diff --git a/src/shell.c b/src/shell.c index 9e4ae1e..5dc8119 100644 --- a/src/shell.c +++ b/src/shell.c @@ -954,6 +954,15 @@ static void parse_and_execute(char* cmd_line) { kprintf("[FAULT] knetd death requested\n"); } } + /* verify-supervisor.sh step 6 only. ktimerd rather than knetd because + * knetd is PRIORITY_NORMAL, the same value task_create_kernel() assigns by + * default -- restoring its priority and failing to restore it are + * indistinguishable. ktimerd is PRIORITY_HIGH, so `ps -l` can witness the + * demotion. */ + else if (strcmp(argv[0], "killktimerd") == 0) { + ktimerd_die_now = 1; + kprintf("[FAULT] ktimerd death requested\n"); + } /* verify-netd-arbitration.sh only, and gated for the same reason killknetd * is: it exists to drive a state no production path reaches yet. * diff --git a/src/supervisor.c b/src/supervisor.c index 970676a..f23c4fb 100644 --- a/src/supervisor.c +++ b/src/supervisor.c @@ -25,7 +25,8 @@ void supervisor_init(void) { supervisor_total_restarts = 0; } -bool supervisor_watch(const char* name, void (*entry)(void), uint32_t pid) { +bool supervisor_watch(const char* name, void (*entry)(void), uint32_t pid, + priority_t priority) { if (!name || !entry || pid == 0) { return false; } @@ -50,6 +51,7 @@ bool supervisor_watch(const char* name, void (*entry)(void), uint32_t pid) { e->entry = entry; e->pid = pid; e->generation = task->generation; + e->priority = priority; e->restarts = 0; e->restarts_in_window = 0; e->window_start_ms = supervisor_now_ms(); @@ -119,6 +121,15 @@ static bool supervisor_restart(supervisor_entry_t* e) { * instruction: the RX counter simply stops advancing. Steps 1 and 2 of * verify-supervisor.sh both pass in that state, which is exactly why that * harness also asserts frames are parsed AFTER the restart. */ + /* Restore the registered priority BEFORE enqueueing. task_create_kernel() + * assigns PRIORITY_NORMAL unconditionally, so a restarted ktimerd or + * edr_daemon would otherwise come back demoted from PRIORITY_HIGH -- alive, + * counted as a successful restart, and reported healthy by every status + * surface, with degraded latency as the only symptom. Set it here rather + * than at the call site so no future restart path can skip it, the same + * reason the rate limit lives in this function. */ + task_set_priority(fresh, e->priority); + scheduler_add_task(fresh); e->pid = (uint32_t)pid; diff --git a/src/supervisor.h b/src/supervisor.h index 8381e2a..6a0e22d 100644 --- a/src/supervisor.h +++ b/src/supervisor.h @@ -3,6 +3,7 @@ #include #include +#include "process.h" /* priority_t */ /*============================================================================= * SYSTEM TASK SUPERVISOR (doc/NETDAEMON_DESIGN.md item 4, PR D2) @@ -44,7 +45,13 @@ * NETWORK_ISOLATION.md item 1's "did it run" flag was reverted for masking. *===========================================================================*/ -#define SUPERVISOR_MAX_TASKS 4 +/* + * Capacity. Was 4 when knetd was the only watched task; now 3 are watched + * (ktimerd, knetd, edr_daemon) and the header must leave room to add one + * without a silent "table full" -- supervisor_watch() prints and returns false + * in that case, which is visible in the boot log but easy to scroll past. + */ +#define SUPERVISOR_MAX_TASKS 8 /* * Restart budget. Deliberately small: these are system tasks that should never @@ -66,6 +73,15 @@ typedef struct { void (*entry)(void); uint32_t pid; uint32_t generation; + /* + * Priority to restore on restart. task_create_kernel() always assigns + * PRIORITY_NORMAL, so without this a restarted ktimerd or edr_daemon comes + * back DEMOTED from PRIORITY_HIGH -- it runs, every status surface reports + * a healthy restart, and the only symptom is worse latency under load. + * knetd never set a priority, so this was invisible while it was the only + * watched task. + */ + priority_t priority; uint32_t restarts; /* total, for the whole uptime */ uint32_t restarts_in_window; uint32_t window_start_ms; @@ -79,7 +95,8 @@ void supervisor_init(void); * restart can distinguish "still the task I registered" from "slot reused". * Returns false if the table is full or the pid is not live. */ -bool supervisor_watch(const char* name, void (*entry)(void), uint32_t pid); +bool supervisor_watch(const char* name, void (*entry)(void), uint32_t pid, + priority_t priority); /* * One supervision pass: check every watched task and restart the dead ones. diff --git a/src/test_tasks.c b/src/test_tasks.c index dbbb716..68669d9 100644 --- a/src/test_tasks.c +++ b/src/test_tasks.c @@ -219,10 +219,40 @@ void task_idle(void) { * the interrupt-corruption bug that broke password login, ECDSA * verification, and the SSH handshake. *=============================================================================*/ +#ifdef TINYOS_FAULT_INJECT +/* + * Fault injection for verify-supervisor.sh step 6 (priority restoration). + * + * Same self-opt-out idiom as knetd_die_now below, and for the same reason: the + * victim clears its OWN CAP_UNKILLABLE so no production path learns to bypass + * the capability check. + * + * ktimerd rather than knetd because knetd is PRIORITY_NORMAL, which is also + * task_create_kernel()'s default -- so a restarted knetd reads back the correct + * priority whether or not the supervisor restores it, and the demotion bug is + * structurally invisible there. ktimerd is PRIORITY_HIGH, so it is the smallest + * task that can witness the difference. + */ +volatile int ktimerd_die_now = 0; +#endif + void task_ktimerd(void) { kprintf("[KTIMERD] Timer bottom-half task started [OK]\n"); while (1) { timer_softirq_run(); +#ifdef TINYOS_FAULT_INJECT + if (ktimerd_die_now) { + task_t* self = scheduler_get_current_task(); + ktimerd_die_now = 0; + if (self) { + kprintf("[KTIMERD] fault injection: exiting on request\n"); + self->capabilities &= ~CAP_UNKILLABLE; + uint32_t self_pid = self->pid; + task_terminate(self_pid); + scheduler_yield(); + } + } +#endif /* Yield; we'll be rescheduled on the next tick. timer_softirq_run() * is cheap (a flag check) when nothing is pending. */ scheduler_yield(); diff --git a/src/test_tasks.h b/src/test_tasks.h index 286c72c..8c99624 100644 --- a/src/test_tasks.h +++ b/src/test_tasks.h @@ -28,6 +28,9 @@ void task_exit_test(void); */ void task_idle(void); void task_ktimerd(void); /* timer bottom-half task */ +#ifdef TINYOS_FAULT_INJECT +extern volatile int ktimerd_die_now; +#endif void task_knetd(void); /* RX bottom-half task (doc/NETWORK_ISOLATION.md) */ #ifdef TINYOS_FAULT_INJECT diff --git a/verify/verify-supervisor.sh b/verify/verify-supervisor.sh index 61eb3f6..0ce4b7e 100755 --- a/verify/verify-supervisor.sh +++ b/verify/verify-supervisor.sh @@ -179,6 +179,14 @@ grep -q "TINYOS_FAULT_INJECT" src/test_tasks.c \ || guard_fail "src/test_tasks.c has no TINYOS_FAULT_INJECT hook; knetd cannot be made to die, so the restart path cannot be exercised at all" +# Step 6's ktimerd hook. Guarded separately for the same reason: a tree without +# it runs steps 1-5 perfectly and then fails step 6's "expected 2 ps -l rows" +# with a message about parsing, which reads as a harness bug rather than a +# missing hook. +grep -q "ktimerd_die_now" src/test_tasks.c \ + || guard_fail "src/test_tasks.c has no ktimerd_die_now hook; step 6 cannot + restart a PRIORITY_HIGH task, so priority restoration is NOT covered" + # Step 5's repeat-death budget. Guarded separately from the flag above because a # tree with only the single-death hook runs steps 1-3 perfectly and then reports # gave-up == 0 at step 5a -- which is indistinguishable from a broken limiter. @@ -257,6 +265,11 @@ export TINYOS_HOOK_SETTLE="sleep 8; true" # to witness that is to send frames and prove they are NOT parsed. A give-up # that still restarts is worse than no limiter at all, and the counter alone # cannot distinguish the two. +# Step 6: let the supervisor observe ktimerd's death and restart it before the +# second `ps -l` reads its priority back. No frames involved -- this leg is +# about the restarted task's PRIORITY, not about networking. +export TINYOS_HOOK_KTSETTLE="sleep 6; true" + export TINYOS_HOOK_INJECT3="sleep 3; $INJECT_CMD >/dev/null 2>&1; sleep 6; true" # Sequence (in the KERNEL shell -- `killknetd` is a kshell builtin, and the @@ -315,6 +328,9 @@ ifconfig=>Supervisor;\ >INJECT2;\ ifconfig=>RX ring;\ ps -l=>knetd;\ +killktimerd=>ktimerd death requested;\ +>KTSETTLE;\ +ps -l=>ktimerd;\ killknetd 8=>knetd death requested x8;\ >SETTLE;\ ifconfig=>Supervisor;\ @@ -561,6 +577,114 @@ if [ "$RX_GU_POST" -ne "$RX_GU_PRE" ]; then fi echo " [step 5c] daemon stayed dead; no frames parsed after the give-up: OK" +# --------------------------------------------------------------------------- +# STEP 6: the restarted task comes back at its REGISTERED PRIORITY. +# +# WHY THIS IS NOT TESTED WITH knetd +# +# knetd is PRIORITY_NORMAL, which is also the value task_create_kernel() +# assigns unconditionally. So a restarted knetd reads back the correct priority +# whether or not supervisor_restart() restores it -- restoring and not +# restoring are indistinguishable there, and steps 1-3 above pass either way. +# That is exactly why the demotion bug survived: knetd was the only watched +# task, and it is the one task that cannot witness this. +# +# ktimerd is PRIORITY_HIGH (3), so after a restart it must still read 3. A +# supervisor that does not restore priority brings it back as PRIORITY_NORMAL +# (2): the task RUNS, the restart counter rises, "has died" and "restarted" +# both print, and every status surface reports a healthy recovery. The only +# symptom is that the timer bottom-half -- TCP timers, DHCP renewal, EDR hooks, +# CSPRNG reseed -- is now scheduled behind interactive work. That is a silent +# latency regression, which is the shape of bug this suite exists to catch. +# +# Read from `ps -l` column 3 (PID STATE PRI ...), for the two samples taken +# either side of the killktimerd. +KT_PRI_SAMPLES=() +while IFS= read -r line; do KT_PRI_SAMPLES+=("$line"); done < <( + grep -a "ktimerd" "$SERIAL" | tr -d '\r' \ + | sed -n 's/^ *\([0-9][0-9]*\) *[A-Za-z][A-Za-z]* *\([0-9]\) .*ktimerd.*/\1 \2/p') + +if [ "${#KT_PRI_SAMPLES[@]}" -lt 2 ]; then + fail_with "expected 2 'ps -l' rows for ktimerd, got ${#KT_PRI_SAMPLES[@]}" \ + "Step 6 reads ktimerd's priority before and after its restart. Without" \ + "both rows the comparison cannot be made, so priority restoration is" \ + "NOT proven -- do not read this run as covering it." \ + "" \ + "rows seen: ${KT_PRI_SAMPLES[*]:-none}" +fi + +# Index the LAST row explicitly rather than with [-1]. +# +# macOS ships bash 3.2, which has no negative array subscripting, and under +# `set -u` "${a[-1]}" is not merely empty -- it raises "bad array subscript" / +# "unbound variable". That aborts the two assignments below, leaving +# KT_PID_POST and KT_PRI_POST unset; the `-ne` comparisons in (b) and (c) then +# fail on stderr WITHOUT stopping the script, so this step printed its "OK" +# line and the harness printed RESULT: PASS while grading nothing at all. +# +# That is the exact false-pass shape this suite exists to prevent, and it was +# invisible because the kernel was correct the whole time: the serial log +# showed ktimerd restarting at priority 3 as intended, so the only evidence of +# the defect was a stderr line nobody was reading. +KT_LAST=$(( ${#KT_PRI_SAMPLES[@]} - 1 )) +KT_PID_PRE=$(echo "${KT_PRI_SAMPLES[0]}" | awk '{print $1}') +KT_PRI_PRE=$(echo "${KT_PRI_SAMPLES[0]}" | awk '{print $2}') +KT_PID_POST=$(echo "${KT_PRI_SAMPLES[$KT_LAST]}" | awk '{print $1}') +KT_PRI_POST=$(echo "${KT_PRI_SAMPLES[$KT_LAST]}" | awk '{print $2}') + +# Guard the parse itself. Every comparison below is an arithmetic `-ne`, which +# treats an empty operand as a syntax error on stderr and CONTINUES -- so a +# silently-empty field reaches the "OK" line as a pass. Assert the four fields +# are non-empty and numeric before any of them is compared. +for _f in "$KT_PID_PRE" "$KT_PRI_PRE" "$KT_PID_POST" "$KT_PRI_POST"; do + case "$_f" in + ''|*[!0-9]*) + fail_with "step 6 could not parse ktimerd's ps -l rows" \ + "Parsed: pre PID='$KT_PID_PRE' pri='$KT_PRI_PRE'," \ + "post PID='$KT_PID_POST' pri='$KT_PRI_POST'." \ + "A non-numeric or empty field means the 'ps -l' column layout" \ + "changed and the sed no longer matches. Priority restoration is" \ + "NOT proven by this run." \ + "" \ + "rows seen: ${KT_PRI_SAMPLES[*]:-none}" + ;; + esac +done +echo " ktimerd: pre PID=$KT_PID_PRE pri=$KT_PRI_PRE post PID=$KT_PID_POST pri=$KT_PRI_POST" + +# (a) POSITIVE CONTROL: it must actually have been restarted. Without this the +# priority comparison is vacuous -- an unchanged priority on a task that never +# died proves nothing at all, and killktimerd failing silently would read as a +# pass. Same false-pass shape as step 5c's control above. +if [ "$KT_PID_PRE" = "$KT_PID_POST" ]; then + fail_with "ktimerd was NOT restarted (PID $KT_PID_PRE both times)" \ + "The priority comparison below is vacuous unless the task actually" \ + "died and came back. Either killktimerd did not land, or the" \ + "supervisor is not watching ktimerd -- check for a" \ + "\"[SUPERVISOR] watching 'ktimerd'\" line in the boot log." +fi + +# (b) the baseline must be HIGH, or the comparison grades the wrong thing. +if [ "$KT_PRI_PRE" -ne 3 ]; then + fail_with "ktimerd's pre-kill priority is $KT_PRI_PRE, expected 3 (PRIORITY_HIGH)" \ + "kernel.c raises ktimerd to PRIORITY_HIGH after creating it. If it is" \ + "not 3 here, this leg cannot distinguish a restored priority from a" \ + "defaulted one, because PRIORITY_NORMAL (2) is what a demotion also" \ + "produces." +fi + +# (c) THE CLAIM. +if [ "$KT_PRI_POST" -ne "$KT_PRI_PRE" ]; then + fail_with "ktimerd came back DEMOTED: priority $KT_PRI_PRE -> $KT_PRI_POST" \ + "supervisor_restart() called task_create_kernel(), which assigns" \ + "PRIORITY_NORMAL unconditionally, and did not restore the priority" \ + "recorded by supervisor_watch(). The task is alive and the restart" \ + "counter rose, so every status surface reports a healthy recovery --" \ + "the only symptom is that the timer bottom-half now runs behind" \ + "interactive work." +fi +echo " [step 6] ktimerd restarted as PID $KT_PID_POST at priority $KT_PRI_POST (unchanged): OK" + # Positive control for (c). Without this, 5c passes on a run where the third # injection never reached the guest at all -- "no frames parsed" and "no frames # sent" are the same reading, which is the failure mode recorded in memory @@ -573,9 +697,10 @@ if [ "$RX_POST" -le "$RX_PRE" ]; then fi echo "" -echo "RESULT: PASS — restart AND give-up both proven" +echo "RESULT: PASS — restart, priority restoration AND give-up all proven" echo " [1-3] knetd killed, observed dead, restarted as PID $NEW_PID (was $DIED_PID)," echo " RX cpl0 $RX_PRE -> $RX_POST across the kill; gave-up 0 at that point." +echo " [6] ktimerd restarted as PID $KT_PID_POST at priority $KT_PRI_POST, unchanged from $KT_PRI_PRE." echo " [5] 8 back-to-back deaths exhausted the budget: gave-up $GU_GAVEUP, announced" echo " on the console, and RX stayed pinned at $RX_GU_PRE across a further" echo " injection -- the daemon stayed dead."