From d2fbdde838f270377de4fc20e919aac3941ea55f Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Mon, 16 May 2022 09:54:46 -0700 Subject: kunit: use kmemdup in kunit_filter_tests(), take suite as const kmemdup() is easier than kmalloc() + memcpy(), per lkp bot. Also make the input `suite` as const since we're now always making copies after commit a127b154a8f2 ("kunit: tool: allow filtering test cases via glob"). Reported-by: kernel test robot Signed-off-by: Daniel Latypov Reviewed-by: David Gow Reviewed-by: Brendan Higgins Signed-off-by: Shuah Khan --- lib/kunit/executor.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'lib/kunit/executor.c') diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 96f96e42ce06..572f64e0a41a 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -55,7 +55,7 @@ static void kunit_parse_filter_glob(struct kunit_test_filter *parsed, /* Create a copy of suite with only tests that match test_glob. */ static struct kunit_suite * -kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob) +kunit_filter_tests(const struct kunit_suite *const suite, const char *test_glob) { int n = 0; struct kunit_case *filtered, *test_case; @@ -69,11 +69,9 @@ kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob) if (n == 0) return NULL; - /* Use memcpy to workaround copy->name being const. */ - copy = kmalloc(sizeof(*copy), GFP_KERNEL); + copy = kmemdup(suite, sizeof(*copy), GFP_KERNEL); if (!copy) return ERR_PTR(-ENOMEM); - memcpy(copy, suite, sizeof(*copy)); filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL); if (!filtered) -- cgit v1.2.3 From e5857d396f35e59e6fe96cf1178b0357cc3a1ea4 Mon Sep 17 00:00:00 2001 From: Daniel Latypov Date: Sat, 9 Jul 2022 11:19:58 +0800 Subject: kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We currently store kunit suites in the .kunit_test_suites ELF section as a `struct kunit_suite***` (modulo some `const`s). For every test file, we store a struct kunit_suite** NULL-terminated array. This adds quite a bit of complexity to the test filtering code in the executor. Instead, let's just make the .kunit_test_suites section contain a single giant array of struct kunit_suite pointers, which can then be directly manipulated. This array is not NULL-terminated, and so none of the test filtering code needs to NULL-terminate anything. Tested-by: Maíra Canal Reviewed-by: Brendan Higgins Signed-off-by: Daniel Latypov Co-developed-by: David Gow Signed-off-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/executor.c | 115 +++++++++++++-------------------------------------- 1 file changed, 28 insertions(+), 87 deletions(-) (limited to 'lib/kunit/executor.c') diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 572f64e0a41a..6c489d6c5e5d 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -9,8 +9,8 @@ * These symbols point to the .kunit_test_suites section and are defined in * include/asm-generic/vmlinux.lds.h, and consequently must be extern. */ -extern struct kunit_suite * const * const __kunit_suites_start[]; -extern struct kunit_suite * const * const __kunit_suites_end[]; +extern struct kunit_suite * const __kunit_suites_start[]; +extern struct kunit_suite * const __kunit_suites_end[]; #if IS_BUILTIN(CONFIG_KUNIT) @@ -90,62 +90,18 @@ kunit_filter_tests(const struct kunit_suite *const suite, const char *test_glob) static char *kunit_shutdown; core_param(kunit_shutdown, kunit_shutdown, charp, 0644); -static struct kunit_suite * const * -kunit_filter_subsuite(struct kunit_suite * const * const subsuite, - struct kunit_test_filter *filter) -{ - int i, n = 0; - struct kunit_suite **filtered, *filtered_suite; - - n = 0; - for (i = 0; subsuite[i]; ++i) { - if (glob_match(filter->suite_glob, subsuite[i]->name)) - ++n; - } - - if (n == 0) - return NULL; - - filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL); - if (!filtered) - return ERR_PTR(-ENOMEM); - - n = 0; - for (i = 0; subsuite[i] != NULL; ++i) { - if (!glob_match(filter->suite_glob, subsuite[i]->name)) - continue; - filtered_suite = kunit_filter_tests(subsuite[i], filter->test_glob); - if (IS_ERR(filtered_suite)) - return ERR_CAST(filtered_suite); - else if (filtered_suite) - filtered[n++] = filtered_suite; - } - filtered[n] = NULL; - - return filtered; -} - +/* Stores an array of suites, end points one past the end */ struct suite_set { - struct kunit_suite * const * const *start; - struct kunit_suite * const * const *end; + struct kunit_suite * const *start; + struct kunit_suite * const *end; }; -static void kunit_free_subsuite(struct kunit_suite * const *subsuite) -{ - unsigned int i; - - for (i = 0; subsuite[i]; i++) - kfree(subsuite[i]); - - kfree(subsuite); -} - static void kunit_free_suite_set(struct suite_set suite_set) { - struct kunit_suite * const * const *suites; + struct kunit_suite * const *suites; for (suites = suite_set.start; suites < suite_set.end; suites++) - kunit_free_subsuite(*suites); + kfree(*suites); kfree(suite_set.start); } @@ -154,7 +110,7 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set, int *err) { int i; - struct kunit_suite * const **copy, * const *filtered_subsuite; + struct kunit_suite **copy, *filtered_suite; struct suite_set filtered; struct kunit_test_filter filter; @@ -169,14 +125,19 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set, kunit_parse_filter_glob(&filter, filter_glob); - for (i = 0; i < max; ++i) { - filtered_subsuite = kunit_filter_subsuite(suite_set->start[i], &filter); - if (IS_ERR(filtered_subsuite)) { - *err = PTR_ERR(filtered_subsuite); + for (i = 0; &suite_set->start[i] != suite_set->end; i++) { + if (!glob_match(filter.suite_glob, suite_set->start[i]->name)) + continue; + + filtered_suite = kunit_filter_tests(suite_set->start[i], filter.test_glob); + if (IS_ERR(filtered_suite)) { + *err = PTR_ERR(filtered_suite); return filtered; } - if (filtered_subsuite) - *copy++ = filtered_subsuite; + if (!filtered_suite) + continue; + + *copy++ = filtered_suite; } filtered.end = copy; @@ -199,52 +160,33 @@ static void kunit_handle_shutdown(void) } -static void kunit_print_tap_header(struct suite_set *suite_set) -{ - struct kunit_suite * const * const *suites, * const *subsuite; - int num_of_suites = 0; - - for (suites = suite_set->start; suites < suite_set->end; suites++) - for (subsuite = *suites; *subsuite != NULL; subsuite++) - num_of_suites++; - - pr_info("TAP version 14\n"); - pr_info("1..%d\n", num_of_suites); -} - static void kunit_exec_run_tests(struct suite_set *suite_set) { - struct kunit_suite * const * const *suites; + size_t num_suites = suite_set->end - suite_set->start; - kunit_print_tap_header(suite_set); + pr_info("TAP version 14\n"); + pr_info("1..%zu\n", num_suites); - for (suites = suite_set->start; suites < suite_set->end; suites++) - __kunit_test_suites_init(*suites); + __kunit_test_suites_init(suite_set->start, num_suites); } static void kunit_exec_list_tests(struct suite_set *suite_set) { - unsigned int i; - struct kunit_suite * const * const *suites; + struct kunit_suite * const *suites; struct kunit_case *test_case; /* Hack: print a tap header so kunit.py can find the start of KUnit output. */ pr_info("TAP version 14\n"); for (suites = suite_set->start; suites < suite_set->end; suites++) - for (i = 0; (*suites)[i] != NULL; i++) { - kunit_suite_for_each_test_case((*suites)[i], test_case) { - pr_info("%s.%s\n", (*suites)[i]->name, test_case->name); - } + kunit_suite_for_each_test_case((*suites), test_case) { + pr_info("%s.%s\n", (*suites)->name, test_case->name); } } int kunit_run_all_tests(void) { - struct suite_set suite_set = { - .start = __kunit_suites_start, - .end = __kunit_suites_end, - }; + struct suite_set suite_set = {__kunit_suites_start, __kunit_suites_end}; int err = 0; if (filter_glob_param) { @@ -262,11 +204,10 @@ int kunit_run_all_tests(void) else pr_err("kunit executor: unknown action '%s'\n", action_param); - if (filter_glob_param) { /* a copy was made of each array */ + if (filter_glob_param) { /* a copy was made of each suite */ kunit_free_suite_set(suite_set); } - out: kunit_handle_shutdown(); return err; -- cgit v1.2.3 From 94681e289bf5d10c9db9db143d1a22d8717205c5 Mon Sep 17 00:00:00 2001 From: David Gow Date: Wed, 13 Jul 2022 07:25:27 +0800 Subject: kunit: executor: Fix a memory leak on failure in kunit_filter_tests It's possible that memory allocation for 'filtered' will fail, but for the copy of the suite to succeed. In this case, the copy could be leaked. Properly free 'copy' in the error case for the allocation of 'filtered' failing. Note that there may also have been a similar issue in kunit_filter_subsuites, before it was removed in "kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites". This was reported by clang-analyzer via the kernel test robot, here: https://lore.kernel.org/all/c8073b8e-7b9e-0830-4177-87c12f16349c@intel.com/ And by smatch via Dan Carpenter and the kernel test robot: https://lore.kernel.org/all/202207101328.ASjx88yj-lkp@intel.com/ Fixes: a02353f49162 ("kunit: bail out of test filtering logic quicker if OOM") Reported-by: kernel test robot Reported-by: kernel test robot Reported-by: Dan Carpenter Reviewed-by: Daniel Latypov Reviewed-by: Brendan Higgins Signed-off-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/executor.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'lib/kunit/executor.c') diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 6c489d6c5e5d..5e223327196a 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -74,8 +74,10 @@ kunit_filter_tests(const struct kunit_suite *const suite, const char *test_glob) return ERR_PTR(-ENOMEM); filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL); - if (!filtered) + if (!filtered) { + kfree(copy); return ERR_PTR(-ENOMEM); + } n = 0; kunit_suite_for_each_test_case(suite, test_case) { -- cgit v1.2.3