Commit 59729170 authored by David Gow's avatar David Gow Committed by Shuah Khan
Browse files

kunit: Make kunit_remove_resource() idempotent 



The kunit_remove_resource() function is used to unlink a resource from
the list of resources in the test, making it no longer show up in
kunit_find_resource().

However, this could lead to a race condition if two threads called
kunit_remove_resource() on the same resource at the same time: the
resource would be removed from the list twice (causing a crash at the
second list_del()), and the refcount for the resource would be
decremented twice (instead of once, for the reference held by the
resource list).

Fix both problems, the first by using list_del_init(), and the second by
checking if the resource has already been removed using list_empty(),
and only decrementing its refcount if it has not.

Also add a KUnit test for the kunit_remove_resource() function which
tests this behaviour.

Reported-by: default avatarDaniel Latypov <dlatypov@google.com>
Signed-off-by: default avatarDavid Gow <davidgow@google.com>
Reviewed-by: default avatarBrendan Higgins <brendanhiggins@google.com>
Signed-off-by: default avatarShuah Khan <skhan@linuxfoundation.org>
parent 1ff522b6

lib/kunit/kunit-test.c

+35 −0
Original line number Diff line number Diff line
@@ -190,6 +190,40 @@ static void kunit_resource_test_destroy_resource(struct kunit *test) 
	KUNIT_EXPECT_TRUE(test, list_empty(&ctx->test.resources));

}



static void kunit_resource_test_remove_resource(struct kunit *test)

{

	struct kunit_test_resource_context *ctx = test->priv;

	struct kunit_resource *res = kunit_alloc_and_get_resource(

			&ctx->test,

			fake_resource_init,

			fake_resource_free,

			GFP_KERNEL,

			ctx);



	/* The resource is in the list */

	KUNIT_EXPECT_FALSE(test, list_empty(&ctx->test.resources));



	/* Remove the resource. The pointer is still valid, but it can't be

	 * found.

	 */

	kunit_remove_resource(test, res);

	KUNIT_EXPECT_TRUE(test, list_empty(&ctx->test.resources));

	/* We haven't been freed yet. */

	KUNIT_EXPECT_TRUE(test, ctx->is_resource_initialized);



	/* Removing the resource multiple times is valid. */

	kunit_remove_resource(test, res);

	KUNIT_EXPECT_TRUE(test, list_empty(&ctx->test.resources));

	/* Despite having been removed twice (from only one reference), the

	 * resource still has not been freed.

	 */

	KUNIT_EXPECT_TRUE(test, ctx->is_resource_initialized);



	/* Free the resource. */

	kunit_put_resource(res);

	KUNIT_EXPECT_FALSE(test, ctx->is_resource_initialized);

}



static void kunit_resource_test_cleanup_resources(struct kunit *test)

{

	int i;
@@ -387,6 +421,7 @@ static struct kunit_case kunit_resource_test_cases[] = { 
	KUNIT_CASE(kunit_resource_test_init_resources),

	KUNIT_CASE(kunit_resource_test_alloc_resource),

	KUNIT_CASE(kunit_resource_test_destroy_resource),

	KUNIT_CASE(kunit_resource_test_remove_resource),

	KUNIT_CASE(kunit_resource_test_cleanup_resources),

	KUNIT_CASE(kunit_resource_test_proper_free_ordering),

	KUNIT_CASE(kunit_resource_test_static),

lib/kunit/resource.c

+6 −2
Original line number Diff line number Diff line
@@ -98,10 +98,14 @@ EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource); 
void kunit_remove_resource(struct kunit *test, struct kunit_resource *res)

{

	unsigned long flags;

	bool was_linked;



	spin_lock_irqsave(&test->lock, flags);

	list_del(&res->node);

	was_linked = !list_empty(&res->node);

	list_del_init(&res->node);

	spin_unlock_irqrestore(&test->lock, flags);



	if (was_linked)

		kunit_put_resource(res);

}

EXPORT_SYMBOL_GPL(kunit_remove_resource);