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 <[email protected]> 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/[email protected]?part=3
