Re: [PATCH v7 05/14] rv: Add a KUnit test for the storage free path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rv: Use-After-Free in rv_test_rvtest_arm_after_destroy due to dropped RCU lock
--

commit 62c45f491a4a036b9788ee3cd149fc4c81f32003
Author: Wen Yang <wen.yang@linux.dev>

rv: Add a KUnit test for the storage free path

The test resolves a storage, desstroys it with da_destroy_storage(), and
then arms the HA timer through the stale pointer. The free hook must
disarm that timer too, so it never fires and no reaction is reported.

> diff --git a/kernel/trace/rv/rv_monitors_test.c b/kernel/trace/rv/rv_monitors_test.c
> index 3ad11195e664a..b30e49419e34b 100644
> --- a/kernel/trace/rv/rv_monitors_test.c
> +++ b/kernel/trace/rv/rv_monitors_test.c
> @@ -146,6 +227,83 @@ static void rv_test_dummy(struct kunit *test)
>  	active_ctx = NULL;
>  }
>  
> +/*
> + * rv_test_rvtest_underflow - the HA clock math must not underflow as u64
> + *
> + * An invariant checked while the uptime is below the budget holds, and a
> + * timer armed after the budget elapsed expires now.
> + */
> +static void rv_test_rvtest_underflow(struct kunit *test)
> +{
> +	struct ha_monitor *ha_mon;
> +	struct da_monitor *da_mon;
> +	const int id = 4242;
> +
> +	prepare_test(test, &rvtest_mon);
> +	da_handle_start_run_event(id, NULL, start_rvtest);
> +
> +	scoped_guard(rcu)
> +		da_mon = da_get_monitor(id, NULL);
> +	KUNIT_ASSERT_NOT_NULL(test, da_mon);
> +	ha_mon = to_ha_monitor(da_mon);
> +
> +	/* Uptime below the budget: time_ns - expire_ns would underflow. */
> +	ha_reset_clk_ns(ha_mon, clk_rvtest, 0);

[Severity: Medium]
Are we dereferencing an RCU-protected pointer outside of its RCU read-side
critical section here?

The scoped_guard(rcu) is used without braces, meaning the RCU read-side
critical section ends immediately after da_get_monitor(). The da_mon variable
is then used by to_ha_monitor() and passed to ha_reset_clk_ns().

Even if there is no concurrent deletion in this specific test environment,
isn't this an RCU read-side violation that dereferences an RCU-protected
pointer after the lock is released?

> +	KUNIT_EXPECT_TRUE(test, ha_check_invariant_ns(ha_mon, clk_rvtest, 0,
> +						      RVTEST_BUDGET_NS));
> +
> +	/*
> +	 * Budget already elapsed: expire - passed would underflow and queue
> +	 * the timer at KTIME_SEC_MAX.
> +	 */
> +	da_monitor_reset(&ha_mon->da_mon);
> +	ha_reset_clk_ns(ha_mon, clk_rvtest, 0);
> +	ha_start_timer_ns(ha_mon, clk_rvtest, RVTEST_BUDGET_NS,
> +			  RVTEST_BUDGET_NS + NSEC_PER_SEC);
> +	KUNIT_EXPECT_TRUE(test,
> +			  ktime_before(hrtimer_get_expires(&ha_mon->hrtimer),
> +				       ktime_get() + NSEC_PER_SEC));
> +
> +	ha_cancel_timer_sync(ha_mon);
> +}
> +
> +/*
> + * rv_test_rvtest_arm_after_destroy - a storage outlived by its own timer
> + *
> + * da_destroy_storage() cancels the timer, then unlinks:
> + *
> + *	da_monitor_reset_hook()		cancels
> + *	hash_del_rcu()
> + *	call_rcu()
> + *
> + * A reader that resolved the storage before the unlink can arm past that
> + * cancel.  Handlers resolve and use under one guard(rcu)(), so the two steps
> + * are issued separately here.
> + */
> +static void rv_test_rvtest_arm_after_destroy(struct kunit *test)
> +{
> +	struct rv_kunit_ctx *ctx = test->priv;
> +	const int id = 4242;
> +	struct da_monitor *stale;
> +
> +	prepare_test(test, &rvtest_mon);
> +
> +	da_handle_start_run_event(id, NULL, start_rvtest);
> +
> +	scoped_guard(rcu)			/* resolve before the unlink */
> +		stale = da_get_monitor(id, NULL);
> +	KUNIT_ASSERT_NOT_NULL(test, stale);
> +
> +	da_destroy_storage(id);			/* cancel, unlink, call_rcu */
> +	ha_start_timer_ns(to_ha_monitor(stale), clk_rvtest,
> +			  RVTEST_BUDGET_NS, ha_get_ns());

[Severity: Medium]
Does this sequence result in an actual use-after-free rather than just
simulating one?

Similar to the above, stale is resolved within a scoped_guard(rcu) that
ends immediately. da_destroy_storage() then unlinks the object and defers
its destruction via call_rcu(). 

Since the test thread no longer holds the RCU read lock, it can be preempted,
allowing the RCU grace period to complete and free the stale object. When
the test thread resumes, doesn't calling ha_start_timer_ns() write to the
freed memory?

To safely simulate the race mentioned in the comment, shouldn't the test hold
the RCU lock continuously across both the lookup and the timer arming steps?

> +
> +	rcu_barrier();
> +
> +	msleep(600);
> +	RV_KUNIT_EXPECT_NO_REACTION(test, ctx);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791745088.git.wen.yang@linux.dev?part=5