Re: [PATCH v14 2/5] revocable: Add KUnit test cases

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kunit: leaked kthreads and UAF on KUNIT_ASSERT_* failure in concurrent test
--

commit 5bce09dba3407560b819288b85de7f22b1f0a604
Author: Tzung-Bi Shih <tzungbi@kernel.org>

revocable: Add KUnit test cases

This commit adds KUnit test cases for the revocable API. The tests cover
basic functionality, revocation, macro helpers, and concurrent access.

> diff --git a/drivers/base/test/revocable-test.c b/drivers/base/test/revocable-test.c
> new file mode 100644
> index 0000000000000..10472303c01c3
> --- /dev/null
> +++ b/drivers/base/test/revocable-test.c
> @@ -0,0 +1,560 @@
[ ... ]
> +static struct test_concurrent_access_context *
> +revocable_test_concurrent_access_prepare(struct kunit *test,
> +					 struct revocable *rev, bool dynamic)
> +{
> +	struct test_concurrent_access_context *ctx;
> +	int i;
> +
> +	ctx = kunit_kmalloc_array(test, 3, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
> +
> +	for (i = 0; i < 3; ++i) {
> +		init_completion(&ctx[i].started);
> +		init_completion(&ctx[i].enter);
> +
> +		if (i == 0) {
> +			ctx[i].rev = rev;
> +			ctx[i].thread = kthread_run(
> +				test_concurrent_access_provider, ctx + i,
> +				"provider%d", i);
> +		} else {
> +			ctx[i].test = test;
> +			init_completion(&ctx[i].entered);
> +			init_completion(&ctx[i].exit);
> +			revocable_handle_init(rev, &ctx[i].rh);
> +			if (dynamic)
> +				KUNIT_EXPECT_EQ(test, get_refcount(rev), 1 + i);
> +
> +			ctx[i].thread = kthread_run(
> +				test_concurrent_access_consumer, ctx + i,
> +				"consumer%d", i);
> +		}
> +		KUNIT_ASSERT_FALSE(test, IS_ERR(ctx[i].thread));

[Severity: Medium]
If this assertion fails (for instance, if a later kthread_run() call fails),
the KUnit framework will immediately abort the test.

Because the test is aborted, the manual kthread_stop_put() teardown loops
found in the caller functions will be bypassed. Does this leave the previously
spawned kthreads running indefinitely against the ctx array that KUnit
automatically frees?

[ ... ]
> +static void revocable_embedded_test_concurrent_access(struct kunit *test)
> +{
> +	struct revocable rev_on_stack, *rev = &rev_on_stack;
> +	void *real_res = (void *)0x12345678, *res;
> +	struct test_concurrent_access_context *ctx;
> +	int ret, i;
> +
> +	ret = revocable_init(rev, real_res);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	KUNIT_EXPECT_FALSE(test, rev->dynamic);
> +
> +	ctx = revocable_test_concurrent_access_prepare(test, rev, false);
> +
> +	ctx[1].expected_res = real_res;
> +	/* consumer1 enters read-side critical section. */
> +	complete(&ctx[1].enter);
> +	wait_for_completion(&ctx[1].entered);
> +
> +	/* provider0 revokes the resource. */
> +	complete(&ctx[0].enter);
> +	ret = read_poll_timeout(rcu_access_pointer, res, !res, 1000, 100000,
> +				false, rev->res);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: Medium]
If read_poll_timeout() reaches its timeout, ret will be non-zero and
KUNIT_ASSERT_EQ() will abort the test. 

Will this bypass the kthread_stop_put() teardown loop below, resulting in a
use-after-free when the still-running kthreads attempt to access the freed ctx
array or the rev_on_stack variable that goes out of scope?

> +	/* provider0 can't exit.  It's waiting for the grace period. */
> +
> +	ctx[2].expected_res = NULL;
> +	/* consumer2 enters read-side critical section. */
> +	complete(&ctx[2].enter);
> +	wait_for_completion(&ctx[2].entered);
> +
> +	/* consumer{1,2} exit read-side critical section. */
> +	for (i = 1; i < 3; ++i) {
> +		complete(&ctx[i].exit);
> +		kthread_stop_put(ctx[i].thread);
> +		revocable_handle_destroy(&ctx[i].rh);
> +	}
> +
> +	/* provider0 exits as all readers exit their critical section. */
> +	kthread_stop_put(ctx[0].thread);
> +
> +	revocable_destroy(rev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011121246.162759-1-tzungbi@kernel.org?part=2