From 59729170afcd4900e08997a482467ffda8d88c7f Mon Sep 17 00:00:00 2001 From: David Gow Date: Sat, 2 Apr 2022 12:35:29 +0800 Subject: [PATCH] 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: Daniel Latypov Signed-off-by: David Gow Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- lib/kunit/kunit-test.c | 35 +++++++++++++++++++++++++++++++++++ lib/kunit/resource.c | 8 ++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index 8e2fe083a549..13d0bd8b07a9 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -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), diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c index 8f8057aad78f..b3ba0d98d89e 100644 --- a/lib/kunit/resource.c +++ b/lib/kunit/resource.c @@ -98,11 +98,15 @@ 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); - kunit_put_resource(res); + + if (was_linked) + kunit_put_resource(res); } EXPORT_SYMBOL_GPL(kunit_remove_resource);