[v2,1/2] kunit: Make kunit_remove_resource() idempotent

Commit Message

David Gow April 2, 2022, 4:35 a.m. UTC

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 <[e-mail]>
Signed-off-by: David Gow <[e-mail]>
Reviewed-by: Brendan Higgins <[e-mail]>
---

Changes since v1:
https://lore.kernel.org/linux-kselftest/[e-mail]/
- Rebased on top of Daniel's split of the resource system into
  resource.{c,h}
  - https://lore.kernel.org/linux-kselftest/[e-mail]/
  - https://lore.kernel.org/linux-kselftest/[e-mail]/

 lib/kunit/kunit-test.c | 35 +++++++++++++++++++++++++++++++++++
 lib/kunit/resource.c   |  8 ++++++--
 2 files changed, 41 insertions(+), 2 deletions(-)

Patch

diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index 555601d17f79..9005034558aa 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 b8bced246217..09ec392d2323 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);