Re: [PATCH 03/10] rv: Cache timer budget when starting HA timers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rv: instant invariant failure on initial state for HA monitors
- [High] rv: lockless TOCTOU race on timer expiration budget causes spurious failures
--

commit ab59c80faa625da1231e21adf0d94a5978fe4a17
Author: Gabriele Monaco <gmonaco@redhat.com>

rv: Cache timer budget when starting HA timers

Commit ab2900ae252b ("rv: Simplify hybrid automata monitors's clock
variables") simplified storage but introduces the assumption that the
expires value passed to ha_check_invariant() is the same used to arm the
timer. This commit caches the armed budget to ensure invariant checks
remain consistent with the timer's duration.

> diff --git a/include/rv/ha_monitor.h b/include/rv/ha_monitor.h
> index 87cf91abdf679..332671f3ce2c2 100644
> --- a/include/rv/ha_monitor.h
> +++ b/include/rv/ha_monitor.h

[ ... ]

> @@ -341,9 +342,13 @@ static inline void ha_reset_clk_ns(struct ha_monitor *ha_mon, enum envs env, u64
>  	WRITE_ONCE(ha_mon->env_store[env], time_ns);
>  }
>  static inline bool ha_check_invariant_ns(struct ha_monitor *ha_mon, enum envs env,
> -					 u64 time_ns, u64 expire_ns)
> +					 u64 time_ns)
>  {
> -	return READ_ONCE(ha_mon->env_store[env]) >= time_ns - expire_ns;
> +	if (HA_TIMER_TYPE == HA_TIMER_WHEEL)
> +		return time_ns - READ_ONCE(ha_mon->env_store[env]) <= ha_mon->expire;
> +	if (HA_TIMER_TYPE == HA_TIMER_HRTIMER)
> +		return time_ns <= ktime_to_ns(hrtimer_get_expires(&ha_mon->hrtimer));

[Severity: High]
What happens to monitors with initial state invariants when they process their
first event?

When a monitor starts, it is set to the initial state, but
ha_setup_invariants() is not called yet to arm the timer or cache the
expiration budget. When the first event arrives, ha_verify_invariants()
evaluates the invariant against the uninitialized timer budget.

For hrtimers, the expiration is 0, which evaluates time_ns <= 0 and triggers
immediate failure. For timer wheels, ha_mon->expire is ENV_INVALID_VALUE,
which could overflow or evaluate incorrectly.

Could this instantly halt any generated monitor that uses an initial state
invariant upon receiving its first event?

> +	return true;
>  }

[ ... ]

> @@ -420,22 +413,30 @@ static inline void ha_setup_timer(struct ha_monitor *ha_mon)
>  
>  	if (RV_MON_TYPE == RV_MON_PER_CPU)
>  		mode |= TIMER_PINNED;
> +	ha_mon->expire = ENV_INVALID_VALUE;
>  	timer_setup(&ha_mon->timer, ha_monitor_timer_callback, mode);
>  }
> +static inline void _ha_start_timer(struct ha_monitor *ha_mon, u64 expire,
> +				   u64 time_ns)
> +{
> +	mod_timer(&ha_mon->timer, get_jiffies_64() + expire);
> +}
>  static inline void ha_start_timer_jiffy(struct ha_monitor *ha_mon, enum envs env,
>  					u64 expire, u64 time_ns)
>  {
>  	u64 passed = ha_invariant_passed_jiffy(ha_mon, env, time_ns);
>  
> -	mod_timer(&ha_mon->timer, get_jiffies_64() + expire - passed);
> +	ha_mon->expire = expire;
> +	_ha_start_timer(ha_mon, expire - passed, time_ns);
>  }
>  static inline void ha_start_timer_ns(struct ha_monitor *ha_mon, enum envs env,
>  				     u64 expire, u64 time_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);
> +	ha_mon->expire = expire;

[Severity: High]
Can a concurrent event on another CPU evaluate the invariant using a stale
timer budget?

The da_event() loop locklessly updates curr_state via try_cmpxchg(), making
the new state globally visible immediately. However, the auxiliary timer budget
(ha_mon->expire or hrtimer expiration) is updated afterwards here.

If a concurrent event occurs on another CPU and reads the newly updated
curr_state, it could call ha_check_invariant_ns() and read the stale,
un-updated timer budget. Could this race result in a false-positive constraint
failure and incorrectly halt the monitor?

> +	_ha_start_timer(ha_mon, nsecs_to_jiffies(expire - passed + TICK_NSEC - 1),
> +			      time_ns);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929124908.177676-1-gmonaco@redhat.com?part=3