Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Check-then-act (TOCTOU) race condition in first-invocation guard reset allows concurrent thread execution to overwrite an initialized invariant timestamp. Pre-existing issues: - [High] `ha_check_invariant_ns` calculates `time_ns - expire_ns`, which can underflow and cause valid invariants to falsely fail when the system uptime is less than `expire_ns`. - [High] `ha_start_timer_ns` and `ha_start_timer_jiffy` suffer from integer underflow when elapsed time (`passed`) exceeds the timeout (`expire`), causing monitor timers to be scheduled far into the future (or at a random time) instead of expiring immediately. -- --- Patch [4]: [PATCH v6 4/9] rv: Fix ha_invariant_passed_ns silent bypass of invariant check --- commit 1e15107a6865e6fddc264988e2a5c771a535486a Author: Wen Yang <[email protected]> rv: Fix ha_invariant_passed_ns silent bypass of invariant check This patch fixes a silent bypass in ha_invariant_passed_ns() and ha_invariant_passed_jiffy() by initializing env_store to the current clock on the first invocation. It replaces the early return with a call to ha_reset_clk_ns() to establish the invariant guard. > diff --git a/include/rv/ha_monitor.h b/include/rv/ha_monitor.h > index 6e1c7fe5449a3..e1738d199b289 100644 > --- a/include/rv/ha_monitor.h > +++ b/include/rv/ha_monitor.h > @@ -355,7 +355,7 @@ static inline u64 ha_invariant_passed_ns(struct > ha_monitor *ha_mon, enum envs en > if (env < 0 || env >= ENV_MAX_STORED) > return 0; > if (ha_monitor_env_invalid(ha_mon, env)) > - return 0; > + ha_reset_clk_ns(ha_mon, env, time_ns); [Severity: High] Does this lockless state check and modification allow a check-then-act race? Looking at ha_invariant_passed_ns() in include/rv/ha_monitor.h, if concurrent threads execute this path for the same multi-state monitor, could Thread A and Thread B both evaluate ha_monitor_env_invalid() as true? This could lead to one thread blindly overwriting the invariant start time initialized by the other, inappropriately pushing the epoch forward and masking violations. > return ha_get_env(ha_mon, env, time_ns); > } > > @@ -375,6 +375,7 @@ static inline bool ha_check_invariant_jiffy(struct > ha_monitor *ha_mon, enum envs > { > return time_after64(READ_ONCE(ha_mon->env_store[env]), get_jiffies_64() > - expire_jiffy); > } > + [Severity: High] This isn't a bug introduced by this patch, but is there a risk of integer underflow in the related ns check function? include/rv/ha_monitor.h:ha_check_invariant_ns() { return READ_ONCE(ha_mon->env_store[env]) >= time_ns - expire_ns; } If the system uptime (time_ns) is smaller than expire_ns (e.g. for a monitor started early during boot with a large timeout budget), could time_ns - expire_ns underflow? This would immediately trigger a false positive invariant violation that could cause a kernel panic if the panic reactor is configured. [Severity: High] This is a pre-existing issue, but could integer underflow occur in the timer setup functions when the elapsed time exceeds the timeout? include/rv/ha_monitor.h:ha_start_timer_ns() { u64 passed = ha_invariant_passed_ns(ha_mon, env, time_ns); ha_start_timer_jiffy(ha_mon, ENV_MAX_STORED, nsecs_to_jiffies(expire - passed + TICK_NSEC - 1), time_ns); } If passed is greater than expire, expire - passed underflows to a massive unsigned 64-bit value. When passed to nsecs_to_jiffies() (or jiffies_to_nsecs() in ha_start_timer_jiffy), this can result in timers being scheduled far into the future (or at a random time) instead of expiring immediately, nullifying the monitor's ability to enforce latency budgets. > /* > * ha_invariant_passed_jiffy - prepare the invariant and return the time > since reset > */ > @@ -383,7 +384,7 @@ static inline u64 ha_invariant_passed_jiffy(struct > ha_monitor *ha_mon, enum envs > if (env < 0 || env >= ENV_MAX_STORED) > return 0; > if (ha_monitor_env_invalid(ha_mon, env)) > - return 0; > + ha_reset_clk_jiffy(ha_mon, env); [Severity: High] Can concurrent threads race when resetting the jiffy clock here as well? Similarly to ha_invariant_passed_ns(), could multiple CPUs evaluating ha_monitor_env_invalid() at the same time in ha_invariant_passed_jiffy() cause an initialized invariant timestamp to be silently overwritten? > return ha_get_env(ha_mon, env, time_ns); > } > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
