Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [High] The patch unmasks a preexisting integer underflow bug in `ha_check_invariant_ns` that will falsely trigger invariant violations during early boot when `time_ns < expire_ns`. - [High] Per-object monitor destruction frees storage via `kfree_rcu` while background timers can still be executing, leading to Use-After-Free. - [High] New monitor storage is published to the RCU hash table before its fields are fully initialized, exposing readers to NULL dereferences. - [High] Lockless updates to the monitor's state and environment variables allow concurrent events to evaluate constraints against stale environment data. - [High] An integer underflow in `ha_start_timer_ns()` and `ha_start_timer_jiffy()` silently masks invariant violations if the elapsed time exceeds the expiration limit upon state entry. -- --- Patch [4]: [PATCH v5 4/9] rv: Fix ha_invariant_passed_ns silent bypass of invariant check --- commit d0917ae23deee9db8a74a56834fa74dbc75f6b60 Author: Wen Yang <[email protected]> rv: Fix ha_invariant_passed_ns silent bypass of invariant check When env_store is U64_MAX, ha_invariant_passed_ns() returns 0 immediately without initializing env_store to the current clock. Subsequent calls find env_store still at U64_MAX, causing the elapsed comparison to wrap and always report the invariant as satisfied, silently masking any violations. This commit fixes it by calling ha_reset_clk_ns() to establish the guard on the first invocation. > 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] This is a pre-existing issue, but does this subtraction underflow during early boot, which is now exposed by this initialization? Looking at ha_check_invariant_ns(): include/rv/ha_monitor.h:ha_check_invariant_ns() { return READ_ONCE(ha_mon->env_store[env]) >= time_ns - expire_ns; } If an ns-based monitor is started during early boot, time_ns can be smaller than expire_ns, causing time_ns - expire_ns to underflow to a huge unsigned value. Since this patch now correctly initializes env_store to time_ns, the check env_store >= huge_value will evaluate to false, spuriously failing the invariant immediately. > return ha_get_env(ha_mon, env, time_ns); > } > [ ... ] [Severity: High] This is a pre-existing issue, but does this calculation underflow if the timeout has already been violated? Looking at ha_start_timer_ns() in include/rv/ha_monitor.h: 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 > expire, the subtraction expire - passed underflows to a massive value, which gets converted to a huge positive delay. Could this schedule the verification timer billions of years in the future instead of failing immediately, silently masking the violation? This same pattern appears in ha_start_timer_jiffy(): include/rv/ha_monitor.h:ha_start_timer_jiffy() { ... u64 passed = ha_invariant_passed_jiffy(ha_mon, env, time_ns); ha_start_timer_ns(ha_mon, ENV_MAX_STORED, jiffies_to_nsecs(expire - passed), time_ns); ... } [Severity: High] This is a pre-existing issue, but can this sequence lead to a use-after-free if a background timer is still executing? Looking at da_destroy_storage() in include/rv/da_monitor.h: include/rv/da_monitor.h:da_destroy_storage() { ... da_monitor_reset_hook(&mon_storage->rv.da_mon); hash_del_rcu(&mon_storage->node); if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL) call_rcu(&mon_storage->rcu, da_pool_return_cb); else kfree_rcu(mon_storage, rcu); ... } da_monitor_reset_hook() cancels the timer via non-synchronous timer_delete() or hrtimer_try_to_cancel(), then the storage is immediately queued for freeing via kfree_rcu(). If the timer callback (__ha_monitor_timer_callback) is preempted before it calls guard(rcu)(), the RCU grace period could complete and free the memory. When the timer thread resumes, it reads the ha_mon_destroying flag from freed memory. [Severity: High] This is a pre-existing issue, but are new monitor objects published to the RCU hash table before they are fully initialized? Looking at da_create_empty_storage() and da_create_storage() in include/rv/da_monitor.h: include/rv/da_monitor.h:da_create_empty_storage() { ... hash_add_rcu(da_monitor_ht, &mon_storage->node, id); mon_storage->id = id; return mon_storage; } include/rv/da_monitor.h:da_create_storage() { ... mon_storage = da_create_empty_storage(id); if (!mon_storage) return NULL; mon_storage->target = target; ... } hash_add_rcu() makes the node visible to concurrent RCU readers before mon_storage->id is assigned, and before mon_storage->target is populated in da_create_storage(). Can a concurrent lookup find the node with a NULL target, leading to a NULL pointer dereference in the caller? [Severity: High] This is a pre-existing issue, but can the state machine and environment guards fall out of sync during concurrent events? Looking at da_event() in include/rv/da_monitor.h: include/rv/da_monitor.h:da_event() { ... if (likely(try_cmpxchg(&da_mon->curr_state, &curr_state, next_state))) { if (!da_monitor_event_hook(da_mon, curr_state, event, next_state, id)) return false; ... } The new state is published locklessly via try_cmpxchg() before da_monitor_event_hook() executes to update the environment variables. For global monitors, if multiple CPUs execute da_event() concurrently, could CPU 2 observe the new state and process a subsequent event before CPU 1 finishes updating the environment variables, causing CPU 2 to evaluate constraints using stale clock values? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
