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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
/*
* 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/cover.1787243842.git.wen.yang@linux.dev?part=4