| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250814124459.3484753-1-marievic@google.com>
Date: Thu, 14 Aug 2025 12:44:59 +0000
From: Marie Zhussupova <marievic@...gle.com>
To: rmoar@...gle.com, davidgow@...gle.com, shuah@...nel.org,
brendan.higgins@...ux.dev
Cc: linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
linux-kernel@...r.kernel.org, Marie Zhussupova <marievic@...gle.com>
Subject: [RFC PATCH] kunit: Put checking for existing named resource between
locks to prevent race conditions in the Resource API
Named resources attached to a KUnit test must be unique. Currently, the
check for existing named resources in kunit_add_named_resource() is
performed without a lock. While KUnit is largely single-threaded, a
race condition could theoretically lead to multiple resources with the
same name being added.
To prevent this, the uniqueness check logic has been moved into the locked
portion of the __kunit_add_resource() function. This ensures that the
check and resource addition are atomic, preventing duplicate named
resources.
Signed-off-by: Marie Zhussupova <marievic@...gle.com>
---
This patch raises a fundamental question about KUnit's concurrency model.
What are the thread-safety guarantees that KUnit should provide? Should
these guarantees be formally defined before we introduce changes like
this one?
It would be great to have everyone's thoughts on this, as it could have
implications on how we have handle concurrency in KUnit going forward.
---
include/kunit/resource.h | 84 ++++++++++++++++++++++++++++++----------
lib/kunit/resource.c | 12 +++++-
2 files changed, 74 insertions(+), 22 deletions(-)
diff --git a/include/kunit/resource.h b/include/kunit/resource.h
index 4ad69a2642a5..7b33f332769c 100644
--- a/include/kunit/resource.h
+++ b/include/kunit/resource.h
@@ -148,12 +148,14 @@ static inline void kunit_put_resource(struct kunit_resource *res)
* If an init function is supplied, @data is passed to it instead.
* @free: a user-supplied function to free the resource (if needed).
* @res: The resource.
+ * @name: name of the resource if there is one.
* @data: value to pass to init function or set in resource data field.
*/
int __kunit_add_resource(struct kunit *test,
kunit_resource_init_t init,
kunit_resource_free_t free,
struct kunit_resource *res,
+ const char *name,
void *data);
/**
@@ -173,7 +175,7 @@ static inline int kunit_add_resource(struct kunit *test,
void *data)
{
res->should_kfree = false;
- return __kunit_add_resource(test, init, free, res, data);
+ return __kunit_add_resource(test, init, free, res, NULL, data);
}
static inline struct kunit_resource *
@@ -195,21 +197,14 @@ static inline int kunit_add_named_resource(struct kunit *test,
const char *name,
void *data)
{
- struct kunit_resource *existing;
if (!name)
return -EINVAL;
- existing = kunit_find_named_resource(test, name);
- if (existing) {
- kunit_put_resource(existing);
- return -EEXIST;
- }
-
res->name = name;
res->should_kfree = false;
- return __kunit_add_resource(test, init, free, res, data);
+ return __kunit_add_resource(test, init, free, res, name, data);
}
/**
@@ -249,7 +244,7 @@ kunit_alloc_and_get_resource(struct kunit *test,
res->should_kfree = true;
- ret = __kunit_add_resource(test, init, free, res, context);
+ ret = __kunit_add_resource(test, init, free, res, NULL, context);
if (!ret) {
/*
* bump refcount for get; kunit_resource_put() should be called
@@ -290,7 +285,7 @@ static inline void *kunit_alloc_resource(struct kunit *test,
return NULL;
res->should_kfree = true;
- if (!__kunit_add_resource(test, init, free, res, context))
+ if (!__kunit_add_resource(test, init, free, res, NULL, context))
return res->data;
return NULL;
@@ -314,29 +309,54 @@ static inline bool kunit_resource_name_match(struct kunit *test,
}
/**
- * kunit_find_resource() - Find a resource using match function/data.
+ * kunit_find_resource_unlocked() - Find a resource using match function/data
+ * without locking.
* @test: Test case to which the resource belongs.
- * @match: match function to be applied to resources/match data.
- * @match_data: data to be used in matching.
+ * @match: Match function to be applied to resources/match data.
+ * @match_data: Data to be used in matching.
+ *
+ * Finds a resource in @test->resources using @match, but does not acquire
+ * the test's lock.
+ *
+ * Note: This function is for specialized use only as it is **NOT thread-safe**
+ * regarding the test's resource list and test reference count. Callers must prevent
+ * potential race conditions, typically by providing external locking. The thread-safe
+ * alternative, kunit_find_resource(), should be used in most situations.
*/
static inline struct kunit_resource *
-kunit_find_resource(struct kunit *test,
- kunit_resource_match_t match,
- void *match_data)
+kunit_find_resource_unlocked(struct kunit *test,
+ kunit_resource_match_t match,
+ void *match_data)
{
struct kunit_resource *res, *found = NULL;
- unsigned long flags;
-
- spin_lock_irqsave(&test->lock, flags);
list_for_each_entry_reverse(res, &test->resources, node) {
- if (match(test, res, (void *)match_data)) {
+ if (match(test, res, match_data)) {
found = res;
kunit_get_resource(found);
break;
}
}
+ return found;
+}
+
+/**
+ * kunit_find_resource() - Find a resource using match function/data.
+ * @test: Test case to which the resource belongs.
+ * @match: match function to be applied to resources/match data.
+ * @match_data: data to be used in matching.
+ */
+static inline struct kunit_resource *
+kunit_find_resource(struct kunit *test,
+ kunit_resource_match_t match,
+ void *match_data)
+{
+ struct kunit_resource *found = NULL;
+ unsigned long flags;
+
+ spin_lock_irqsave(&test->lock, flags);
+ found = kunit_find_resource_unlocked(test, match, match_data);
spin_unlock_irqrestore(&test->lock, flags);
return found;
@@ -355,6 +375,28 @@ kunit_find_named_resource(struct kunit *test,
(void *)name);
}
+/**
+ * kunit_find_named_resource_unlocked() - Find a resource using match name
+ * without locking.
+ * @test: Test case to which the resource belongs.
+ * @name: Match name.
+ *
+ * Finds a resource in @test->resources using its name, but does not acquire
+ * the test's resource lock.
+ *
+ * Note: This function is for specialized use only as it is **NOT thread-safe**
+ * regarding the test's resource list and test reference count. Callers must prevent
+ * potential race conditions, typically by providing external locking. The thread-safe
+ * alternative, kunit_find_named_resource(), should be used in most situations.
+ */
+static inline struct kunit_resource *
+kunit_find_named_resource_unlocked(struct kunit *test,
+ const char *name)
+{
+ return kunit_find_resource_unlocked(test, kunit_resource_name_match,
+ (void *)name);
+}
+
/**
* kunit_destroy_resource() - Find a kunit_resource and destroy it.
* @test: Test case to which the resource belongs.
diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
index f0209252b179..dae708f81f97 100644
--- a/lib/kunit/resource.c
+++ b/lib/kunit/resource.c
@@ -20,10 +20,12 @@ int __kunit_add_resource(struct kunit *test,
kunit_resource_init_t init,
kunit_resource_free_t free,
struct kunit_resource *res,
+ const char *name,
void *data)
{
int ret = 0;
unsigned long flags;
+ struct kunit_resource *existing;
res->free = free;
kref_init(&res->refcount);
@@ -37,6 +39,14 @@ int __kunit_add_resource(struct kunit *test,
}
spin_lock_irqsave(&test->lock, flags);
+ if (name) {
+ existing = kunit_find_named_resource_unlocked(test, name);
+ if (existing) {
+ kunit_put_resource(existing);
+ spin_unlock_irqrestore(&test->lock, flags);
+ return -EEXIST;
+ }
+ }
list_add_tail(&res->node, &test->resources);
/* refcount for list is established by kref_init() */
spin_unlock_irqrestore(&test->lock, flags);
@@ -107,7 +117,7 @@ int kunit_add_action(struct kunit *test, void (*action)(void *), void *ctx)
action_ctx->res.should_kfree = true;
/* As init is NULL, this cannot fail. */
- __kunit_add_resource(test, NULL, __kunit_action_free, &action_ctx->res, action_ctx);
+ __kunit_add_resource(test, NULL, __kunit_action_free, &action_ctx->res, NULL, action_ctx);
return 0;
}
--
2.51.0.rc0.215.g125493bb4a-goog
Powered by blists - more mailing lists