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