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

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