From cdebea6968faafa955b3cc9196003e7f17f78955 Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Mon, 28 Mar 2022 10:41:43 -0700 Subject: kunit: split resource API impl from test.c into new resource.c We've split out the declarations from include/kunit/test.h into resource.h. This patch splits out the definitions as well for consistency. A side effect of this is git blame won't properly track history by default, users need to run $ git blame -L ,1 -C13 lib/kunit/resource.c Signed-off-by: Daniel Latypov Reviewed-by: David Gow Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- lib/kunit/resource.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 lib/kunit/resource.c (limited to 'lib/kunit/resource.c') diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c new file mode 100644 index 000000000000..8f8057aad78f --- /dev/null +++ b/lib/kunit/resource.c @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit resource API for test managed resources (allocations, etc.). + * + * Copyright (C) 2022, Google LLC. + * Author: Daniel Latypov + */ + +#include +#include +#include + +/* + * Used for static resources and when a kunit_resource * has been created by + * kunit_alloc_resource(). When an init function is supplied, @data is passed + * into the init function; otherwise, we simply set the resource data field to + * the data value passed in. + */ +int kunit_add_resource(struct kunit *test, + kunit_resource_init_t init, + kunit_resource_free_t free, + struct kunit_resource *res, + void *data) +{ + int ret = 0; + unsigned long flags; + + res->free = free; + kref_init(&res->refcount); + + if (init) { + ret = init(res, data); + if (ret) + return ret; + } else { + res->data = data; + } + + spin_lock_irqsave(&test->lock, flags); + list_add_tail(&res->node, &test->resources); + /* refcount for list is established by kref_init() */ + spin_unlock_irqrestore(&test->lock, flags); + + return ret; +} +EXPORT_SYMBOL_GPL(kunit_add_resource); + +int kunit_add_named_resource(struct kunit *test, + kunit_resource_init_t init, + kunit_resource_free_t free, + struct kunit_resource *res, + 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; + + return kunit_add_resource(test, init, free, res, data); +} +EXPORT_SYMBOL_GPL(kunit_add_named_resource); + +struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test, + kunit_resource_init_t init, + kunit_resource_free_t free, + gfp_t internal_gfp, + void *data) +{ + struct kunit_resource *res; + int ret; + + res = kzalloc(sizeof(*res), internal_gfp); + if (!res) + return NULL; + + ret = kunit_add_resource(test, init, free, res, data); + if (!ret) { + /* + * bump refcount for get; kunit_resource_put() should be called + * when done. + */ + kunit_get_resource(res); + return res; + } + return NULL; +} +EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource); + +void kunit_remove_resource(struct kunit *test, struct kunit_resource *res) +{ + unsigned long flags; + + spin_lock_irqsave(&test->lock, flags); + list_del(&res->node); + spin_unlock_irqrestore(&test->lock, flags); + kunit_put_resource(res); +} +EXPORT_SYMBOL_GPL(kunit_remove_resource); + +int kunit_destroy_resource(struct kunit *test, kunit_resource_match_t match, + void *match_data) +{ + struct kunit_resource *res = kunit_find_resource(test, match, + match_data); + + if (!res) + return -ENOENT; + + kunit_remove_resource(test, res); + + /* We have a reference also via _find(); drop it. */ + kunit_put_resource(res); + + return 0; +} +EXPORT_SYMBOL_GPL(kunit_destroy_resource); -- cgit From 59729170afcd4900e08997a482467ffda8d88c7f Mon Sep 17 00:00:00 2001 From: David Gow Date: Sat, 2 Apr 2022 12:35:29 +0800 Subject: 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/resource.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'lib/kunit/resource.c') 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); -- cgit From ad69172ec930075d25e14220841dd96375088d28 Mon Sep 17 00:00:00 2001 From: David Gow Date: Sat, 2 Apr 2022 12:35:30 +0800 Subject: kunit: Rework kunit_resource allocation policy KUnit's test-managed resources can be created in two ways: - Using the kunit_add_resource() family of functions, which accept a struct kunit_resource pointer, typically allocated statically or on the stack during the test. - Using the kunit_alloc_resource() family of functions, which allocate a struct kunit_resource using kzalloc() behind the scenes. Both of these families of functions accept a 'free' function to be called when the resource is finally disposed of. At present, KUnit will kfree() the resource if this 'free' function is specified, and will not if it is NULL. However, this can lead kunit_alloc_resource() to leak memory (if no 'free' function is passed in), or kunit_add_resource() to incorrectly kfree() memory which was allocated by some other means (on the stack, as part of a larger allocation, etc), if a 'free' function is provided. Instead, always kfree() if the resource was allocated with kunit_alloc_resource(), and never kfree() if it was passed into kunit_add_resource() by the user. (If the user of kunit_add_resource() wishes the resource be kfree()ed, they can call kfree() on the resource from within the 'free' function. This is implemented by adding a 'should_free' member to struct kunit_resource and setting it appropriately. To facilitate this, the various resource add/alloc functions have been refactored somewhat, making them all call a __kunit_add_resource() helper after setting the 'should_free' member appropriately. In the process, all other functions have been made static inline functions. Signed-off-by: David Gow Tested-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- lib/kunit/resource.c | 64 ++++++---------------------------------------------- 1 file changed, 7 insertions(+), 57 deletions(-) (limited to 'lib/kunit/resource.c') diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c index b3ba0d98d89e..c414df922f34 100644 --- a/lib/kunit/resource.c +++ b/lib/kunit/resource.c @@ -14,13 +14,13 @@ * Used for static resources and when a kunit_resource * has been created by * kunit_alloc_resource(). When an init function is supplied, @data is passed * into the init function; otherwise, we simply set the resource data field to - * the data value passed in. + * the data value passed in. Doesn't initialize res->should_kfree. */ -int kunit_add_resource(struct kunit *test, - kunit_resource_init_t init, - kunit_resource_free_t free, - struct kunit_resource *res, - void *data) +int __kunit_add_resource(struct kunit *test, + kunit_resource_init_t init, + kunit_resource_free_t free, + struct kunit_resource *res, + void *data) { int ret = 0; unsigned long flags; @@ -43,57 +43,7 @@ int kunit_add_resource(struct kunit *test, return ret; } -EXPORT_SYMBOL_GPL(kunit_add_resource); - -int kunit_add_named_resource(struct kunit *test, - kunit_resource_init_t init, - kunit_resource_free_t free, - struct kunit_resource *res, - 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; - - return kunit_add_resource(test, init, free, res, data); -} -EXPORT_SYMBOL_GPL(kunit_add_named_resource); - -struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test, - kunit_resource_init_t init, - kunit_resource_free_t free, - gfp_t internal_gfp, - void *data) -{ - struct kunit_resource *res; - int ret; - - res = kzalloc(sizeof(*res), internal_gfp); - if (!res) - return NULL; - - ret = kunit_add_resource(test, init, free, res, data); - if (!ret) { - /* - * bump refcount for get; kunit_resource_put() should be called - * when done. - */ - kunit_get_resource(res); - return res; - } - return NULL; -} -EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource); +EXPORT_SYMBOL_GPL(__kunit_add_resource); void kunit_remove_resource(struct kunit *test, struct kunit_resource *res) { -- cgit