Re: [PATCH v6 4/9] rv: Fix ha_invariant_passed_ns silent bypass of invariant check

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.

[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.

[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.

[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