[PATCH 0/4] kunit: Fix some bugs in kunit

[Jinjie Ruan]

The test_cases is not freed in kunit_free_suite_set().

And the copy pointer may be moved in kunit_filter_suites().

The filtered_suite and filtered_suite->test_cases allocated in the last
kunit_filter_attr_tests() in last inner for loop may be leaked if
kunit_filter_suites() fails.

If kunit_filter_suites() succeeds, not only copy but also filtered_suite
and filtered_suite->test_cases should be freed.

Jinjie Ruan (4):
kunit: Fix missed memory release in kunit_free_suite_set()
kunit: Fix the wrong kfree of copy for kunit_filter_suites()
kunit: Fix possible memory leak in kunit_filter_suites()
kunit: test: Fix the possible memory leak in executor_test

lib/kunit/executor.c | 23 +++++++++++++++++------
lib/kunit/executor_test.c | 24 ++++++++++++++++++------
2 files changed, 35 insertions(+), 12 deletions(-)

--
2.34.1

[Jinjie Ruan]

modprobe cpumask_kunit and rmmod cpumask_kunit, kmemleak detect
a suspected memory leak as below.

If kunit_filter_suites() in kunit_module_init() succeeds, the
suite_set.start will not be NULL and the kunit_free_suite_set() in
kunit_module_exit() should free all the memory which has not
been freed. However the test_cases in suites is left out.

unreferenced object 0xffff54ac47e83200 (size 512):
comm "modprobe", pid 592, jiffies 4294913238 (age 1367.612s)
hex dump (first 32 bytes):
84 13 1a f0 d3 b6 ff ff 30 68 1a f0 d3 b6 ff ff ........0h......
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<000000008dec63a2>] slab_post_alloc_hook+0xb8/0x368
[<00000000ec280d8e>] __kmem_cache_alloc_node+0x174/0x290
[<00000000896c7740>] __kmalloc+0x60/0x2c0
[<000000007a50fa06>] kunit_filter_suites+0x254/0x5b8
[<0000000078cc98e2>] kunit_module_notify+0xf4/0x240
[<0000000033cea952>] notifier_call_chain+0x98/0x17c
[<00000000973d05cc>] notifier_call_chain_robust+0x4c/0xa4
[<000000005f95895f>] blocking_notifier_call_chain_robust+0x4c/0x74
[<0000000048e36fa7>] load_module+0x1a2c/0x1c40
[<0000000004eb8a91>] init_module_from_file+0x94/0xcc
[<0000000037dbba28>] idempotent_init_module+0x184/0x278
[<00000000161b75cb>] __arm64_sys_finit_module+0x68/0xa8
[<000000006dc1669b>] invoke_syscall+0x44/0x100
[<00000000fa87e304>] el0_svc_common.constprop.1+0x68/0xe0
[<000000009d8ad866>] do_el0_svc+0x1c/0x28
[<000000005b83c607>] el0_svc+0x3c/0xc4

Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites")
Fixes: b67abaad4d25 ("kunit: Allow kunit test modules to use test filtering")
Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
---
lib/kunit/executor.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index a6348489d45f..a037a46fae5e 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -137,8 +137,10 @@ void kunit_free_suite_set(struct kunit_suite_set suite_set)
{
struct kunit_suite * const *suites;

- for (suites = suite_set.start; suites < suite_set.end; suites++)
+ for (suites = suite_set.start; suites < suite_set.end; suites++) {
+ kfree((*suites)->test_cases);
kfree(*suites);
+ }
kfree(suite_set.start);
}

--
2.34.1

[Jinjie Ruan]

If the outer layer for loop is iterated more than once and it fails not
in the first iteration, the copy pointer has been moved. So it should free
the original copy's backup copy_start.

Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")

Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
---

lib/kunit/executor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index a037a46fae5e..9358ed2df839 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -243,7 +243,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,

free_copy:
if (*err)
- kfree(copy);
+ kfree(copy_start);

return filtered;
}
--
2.34.1

[Jinjie Ruan]

If the outer layer for loop is iterated more than once and it fails not

in the first iteration, the filtered_suite and filtered_suite->test_cases

allocated in the last kunit_filter_attr_tests() in last inner for loop

is leaked.

So add a new free_filtered_suite err label and free the filtered_suite
and filtered_suite->test_cases so far. And change kmalloc_array of copy
to kcalloc to Clear the copy to make the kfree safe.

Fixes: 5d31f71efcb6 ("kunit: add kunit.filter_glob cmdline option to filter suites")
Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")

Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
---

lib/kunit/executor.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 9358ed2df839..1236b3cd2fbb 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -157,10 +157,11 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
struct kunit_suite_set filtered = {NULL, NULL};
struct kunit_glob_filter parsed_glob;
struct kunit_attr_filter *parsed_filters = NULL;
+ struct kunit_suite * const *suites;

const size_t max = suite_set->end - suite_set->start;

- copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL);
+ copy = kcalloc(max, sizeof(*filtered.start), GFP_KERNEL);
if (!copy) { /* won't be able to run anything, return an empty set */
return filtered;
}
@@ -195,7 +196,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
parsed_glob.test_glob);
if (IS_ERR(filtered_suite)) {
*err = PTR_ERR(filtered_suite);
- goto free_parsed_filters;
+ goto free_filtered_suite;
}
}
if (filter_count > 0 && parsed_filters != NULL) {
@@ -212,11 +213,11 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
filtered_suite = new_filtered_suite;

if (*err)
- goto free_parsed_filters;
+ goto free_filtered_suite;

if (IS_ERR(filtered_suite)) {
*err = PTR_ERR(filtered_suite);
- goto free_parsed_filters;
+ goto free_filtered_suite;
}
if (!filtered_suite)
break;
@@ -231,6 +232,14 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
filtered.start = copy_start;
filtered.end = copy;

+free_filtered_suite:
+ if (*err) {
+ for (suites = copy_start; suites < copy; suites++) {
+ kfree((*suites)->test_cases);
+ kfree(*suites);
+ }
+ }
+
free_parsed_filters:
if (filter_count)
kfree(parsed_filters);
--
2.34.1

[Jinjie Ruan]

If kunit_filter_suites() succeeds, not only copy but also filtered_suite
and filtered_suite->test_cases should be freed.

So use kunit_free_suite_set() to free the filtered_suite,
filtered_suite->test_cases and copy as kunit_module_exit() and
kunit_run_all_tests() do it.

Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites")

Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
---

lib/kunit/executor_test.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index b4f6f96b2844..987b81dce01e 100644
--- a/lib/kunit/executor_test.c
+++ b/lib/kunit/executor_test.c
@@ -56,7 +56,6 @@ static void filter_suites_test(struct kunit *test)
got = kunit_filter_suites(&suite_set, "suite2", NULL, NULL, &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

/* Validate we just have suite2 */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
@@ -64,6 +63,9 @@ static void filter_suites_test(struct kunit *test)

/* Contains one element (end is 1 past end) */
KUNIT_ASSERT_EQ(test, got.end - got.start, 1);
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void filter_suites_test_glob_test(struct kunit *test)
@@ -82,7 +84,6 @@ static void filter_suites_test_glob_test(struct kunit *test)
got = kunit_filter_suites(&suite_set, "suite2.test2", NULL, NULL, &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

/* Validate we just have suite2 */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
@@ -93,6 +94,9 @@ static void filter_suites_test_glob_test(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test2");
KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void filter_suites_to_empty_test(struct kunit *test)
@@ -109,10 +113,12 @@ static void filter_suites_to_empty_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, "not_found", NULL, NULL, &err);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start); /* just in case */

KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
"should be empty to indicate no match");
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void parse_filter_attr_test(struct kunit *test)
@@ -172,7 +178,6 @@ static void filter_attr_test(struct kunit *test)
got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

/* Validate we just have normal_suite */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
@@ -183,6 +188,9 @@ static void filter_attr_test(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
KUNIT_EXPECT_STREQ(test, got.start[0]->test_cases[0].name, "normal");
KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void filter_attr_empty_test(struct kunit *test)
@@ -200,10 +208,12 @@ static void filter_attr_empty_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start); /* just in case */

KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
"should be empty to indicate no match");
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void filter_attr_skip_test(struct kunit *test)
@@ -222,7 +232,6 @@ static void filter_attr_skip_test(struct kunit *test)
got = kunit_filter_suites(&suite_set, NULL, filter, "skip", &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

/* Validate we have both the slow and normal test */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
@@ -233,6 +242,9 @@ static void filter_attr_skip_test(struct kunit *test)
/* Now ensure slow is skipped and normal is not */
KUNIT_EXPECT_EQ(test, got.start[0]->test_cases[0].status, KUNIT_SKIPPED);
KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].status);
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static struct kunit_case executor_test_cases[] = {
--
2.34.1

[kernel test robot]

Hi Jinjie,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on next-20230914]
[cannot apply to v6.6-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Jinjie-Ruan/kunit-Fix-missed-memory-release-in-kunit_free_suite_set/20230914-194915
base: linus/master
patch link: https://lore.kernel.org/r/20230914114629.1517650-5-ruanjinjie%40huawei.com
patch subject: [PATCH 4/4] kunit: test: Fix the possible memory leak in executor_test
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230914/202309142251...@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230914/202309142251...@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <l...@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309142251...@intel.com/

All warnings (new ones prefixed by >>):

In file included from lib/kunit/executor.c:353:
>> lib/kunit/executor_test.c:274:13: warning: 'kfree_at_end' defined but not used [-Wunused-function]
274 | static void kfree_at_end(struct kunit *test, const void *to_free)
| ^~~~~~~~~~~~


vim +/kfree_at_end +274 lib/kunit/executor_test.c

1d71307a6f94df Daniel Latypov 2021-04-20 270
1d71307a6f94df Daniel Latypov 2021-04-20 271 /* Use the resource API to register a call to kfree(to_free).
1d71307a6f94df Daniel Latypov 2021-04-20 272 * Since we never actually use the resource, it's safe to use on const data.
1d71307a6f94df Daniel Latypov 2021-04-20 273 */
1d71307a6f94df Daniel Latypov 2021-04-20 @274 static void kfree_at_end(struct kunit *test, const void *to_free)
1d71307a6f94df Daniel Latypov 2021-04-20 275 {
1d71307a6f94df Daniel Latypov 2021-04-20 276 /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */
1d71307a6f94df Daniel Latypov 2021-04-20 277 if (IS_ERR_OR_NULL(to_free))
1d71307a6f94df Daniel Latypov 2021-04-20 278 return;
00e63f8afcfc6b David Gow 2023-05-25 279
00e63f8afcfc6b David Gow 2023-05-25 280 kunit_add_action(test,
00e63f8afcfc6b David Gow 2023-05-25 281 (kunit_action_t *)kfree,
1d71307a6f94df Daniel Latypov 2021-04-20 282 (void *)to_free);
1d71307a6f94df Daniel Latypov 2021-04-20 283 }
1d71307a6f94df Daniel Latypov 2021-04-20 284

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[Rae Moar]

Hello!

This looks good to me.

Reviewed-by: Rae Moar <rm...@google.com>

Thanks!

-Rae

> ---
> lib/kunit/executor.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index a6348489d45f..a037a46fae5e 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -137,8 +137,10 @@ void kunit_free_suite_set(struct kunit_suite_set suite_set)
> {
> struct kunit_suite * const *suites;
>
> - for (suites = suite_set.start; suites < suite_set.end; suites++)
> + for (suites = suite_set.start; suites < suite_set.end; suites++) {
> + kfree((*suites)->test_cases);
> kfree(*suites);
> + }
> kfree(suite_set.start);
> }
>
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230914114629.1517650-2-ruanjinjie%40huawei.com.

[Rae Moar]

On Thu, Sep 14, 2023 at 7:47 AM 'Jinjie Ruan' via KUnit Development
<kuni...@googlegroups.com> wrote:
>

> If the outer layer for loop is iterated more than once and it fails not
> in the first iteration, the copy pointer has been moved. So it should free
> the original copy's backup copy_start.
>
> Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")
> Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>

Hello!

This looks good to me as well.

Reviewed-by: Rae Moar <rm...@google.com>

Thanks!

-Rae

> ---
> lib/kunit/executor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index a037a46fae5e..9358ed2df839 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -243,7 +243,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>
> free_copy:
> if (*err)
> - kfree(copy);
> + kfree(copy_start);
>
> return filtered;
> }
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230914114629.1517650-3-ruanjinjie%40huawei.com.

[Rae Moar]

On Thu, Sep 14, 2023 at 7:47 AM 'Jinjie Ruan' via KUnit Development
<kuni...@googlegroups.com> wrote:
>

> If the outer layer for loop is iterated more than once and it fails not
> in the first iteration, the filtered_suite and filtered_suite->test_cases
> allocated in the last kunit_filter_attr_tests() in last inner for loop
> is leaked.
>
> So add a new free_filtered_suite err label and free the filtered_suite
> and filtered_suite->test_cases so far. And change kmalloc_array of copy
> to kcalloc to Clear the copy to make the kfree safe.
>
> Fixes: 5d31f71efcb6 ("kunit: add kunit.filter_glob cmdline option to filter suites")
> Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
> Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>

Hello!

This looks good to me. I just have one comment below.

Reviewed-by: Rae Moar <rm...@google.com>

Thanks!
-Rae

As this is pretty similar code to kunit_free_suite_set, I wish you
could use that method instead but I'm not actually sure it would be
cleaner.

> free_parsed_filters:
> if (filter_count)
> kfree(parsed_filters);
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230914114629.1517650-4-ruanjinjie%40huawei.com.

[Rae Moar]

On Thu, Sep 14, 2023 at 7:47 AM 'Jinjie Ruan' via KUnit Development
<kuni...@googlegroups.com> wrote:
>

> If kunit_filter_suites() succeeds, not only copy but also filtered_suite
> and filtered_suite->test_cases should be freed.
>
> So use kunit_free_suite_set() to free the filtered_suite,
> filtered_suite->test_cases and copy as kunit_module_exit() and
> kunit_run_all_tests() do it.
>
> Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites")
> Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>

Hello!

This looks mostly good to me. But I have one notable comment. See below.

Thanks!
-Rae

> ---
> lib/kunit/executor_test.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> index b4f6f96b2844..987b81dce01e 100644
> --- a/lib/kunit/executor_test.c
> +++ b/lib/kunit/executor_test.c
> @@ -56,7 +56,6 @@ static void filter_suites_test(struct kunit *test)
> got = kunit_filter_suites(&suite_set, "suite2", NULL, NULL, &err);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> KUNIT_ASSERT_EQ(test, err, 0);
> - kfree_at_end(test, got.start);
>
> /* Validate we just have suite2 */
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
> @@ -64,6 +63,9 @@ static void filter_suites_test(struct kunit *test)
>
> /* Contains one element (end is 1 past end) */
> KUNIT_ASSERT_EQ(test, got.end - got.start, 1);
> +
> + if (!err)
> + kunit_free_suite_set(got);

I definitely appreciate the change to free all of "got" rather than
just "got.start".

However, kfree_at_end used deferred actions to ensure the kfree would
occur at the end of the test. With this change, if the test fails the
suite set could not be freed.

Intead, is there any way to alter the function kfree_at_end (could be
renamed) to take in "got" and then use deferred actions to ensure
kunit_free_suite_set is called at the end of the test?

Let me know what you think about this.

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230914114629.1517650-5-ruanjinjie%40huawei.com.

[Ruan Jinjie]

There is a slight difference between here and kunit_free_suite_set(), it
do not kfree(suite_set.start) which is kfree(copy_start) here as it is
the first kcalloc.

[Ruan Jinjie]

Only when suite_set.start != NULL which is equivalent to err == 0 the
suite set will be freed in kunit_module_exit(), and
kunit_free_suite_set() will be called only when kunit_filter_suites()
succeeds with err == 0. So judging from the use of kunit_filter_suites()
in kunit_module_exit() and kunit_run_all_tests(), it only wants to free
the suite set when kunit_filter_suites() succeeds with err == 0. So in
kunit_filter_suites() , we free
copy, filtered_suite and filtered_suite->test_cases if (*err) is true.

So if the test fails the suite set will be freed also. And there's a
double free problem in kfree_at_end(test, got.start) if the test fails.
So only free the suite set when err == 0.


737 static void kunit_module_init(struct module *mod)
738 {
739 struct kunit_suite_set suite_set = {
740 mod->kunit_suites, mod->kunit_suites +
mod->num_kunit_suites,
741 };
742 const char *action = kunit_action();
743 int err = 0;
744
745 suite_set = kunit_filter_suites(&suite_set,
746 kunit_filter_glob() ?: "*.*",
747 kunit_filter(),
kunit_filter_action(),
748 &err);

765 static void kunit_module_exit(struct module *mod)
766 {
767 struct kunit_suite_set suite_set = {
768 mod->kunit_suites, mod->kunit_suites +
mod->num_kunit_suites,
769 };
770 const char *action = kunit_action();
771
772 if (!action)
773 __kunit_test_suites_exit(mod->kunit_suites,
774 mod->num_kunit_suites);
775
776 if (suite_set.start)
777 kunit_free_suite_set(suite_set);
778 }



314 int kunit_run_all_tests(void)
315 {
......
325 if (filter_glob_param || filter_param) {
326 suite_set = kunit_filter_suites(&suite_set,
filter_glob_param,
327 filter_param, filter_action_param,
&err);
328 if (err) {
329 pr_err("kunit executor: error filtering
suites: %d\n", err);
330 goto out;
331 }
332 }
......
342
343 if (filter_glob_param || filter_param) { /* a copy was made
of each suite */
344 kunit_free_suite_set(suite_set);
345 }
346
347 out:
348 kunit_handle_shutdown();
349 return err;
350 }

>
> Intead, is there any way to alter the function kfree_at_end (could be
> renamed) to take in "got" and then use deferred actions to ensure
> kunit_free_suite_set is called at the end of the test?

It is good iead. And it may be fine to call kfree_at_end(test, got) if
err == 0 to avoid double free issue.

[Ruan Jinjie]

On 2023/9/20 5:19, Rae Moar wrote:

I try it and it seems unfeasible because the got is a local struct
kunit_suite_set and kunit_free_suite_set use it in another func will
cause wild-memory-access as the struct kunit_suite_set has been freed
already.

[ 49.490158] general protection fault, probably for non-canonical
address 0xe006fbfff71d514d: 0000 [#1] PREEMPT SMP KASAN
[ 49.493858] KASAN: maybe wild-memory-access in range
[0x0037ffffb8ea8a68-0x0037ffffb8ea8a6f]
[ 49.495391] CPU: 2 PID: 1439 Comm: kunit_try_catch Tainted: G B
N 6.6.0-rc2+ #29
[ 49.496578] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS 1.15.0-1 04/01/2014
[ 49.497715] RIP: 0010:kunit_free_suite_set+0x8e/0x150
[ 49.498419] Code: 4d 89 fe 49 c1 ee 03 49 01 de 48 89 e8 48 c1 e8 03
80 3c 18 00 75 7d 4c 8b 65 00 49 8d bc 24 20 01 00 00 48 89 f8 48 c1 e8
03 <80> 3c 18 00 0f 85 81 00 00 00 49 8b bc 24 20 01 00 00 e8 2b 90 e0
[ 49.500918] RSP: 0000:ffff8881047f7e18 EFLAGS: 00010207
[ 49.501627] RAX: 0006fffff71d514d RBX: dffffc0000000000 RCX:
1ffff11020814d9e
[ 49.502597] RDX: 1ffff110214f6fbc RSI: 0000000000000004 RDI:
0037ffffb8ea8a6c
[ 49.503553] RBP: ffffffff811d098f R08: 0000000000000001 R09:
ffffed1020814d99
[ 49.504526] R10: ffff8881040a6ccb R11: 0000000000000400 R12:
0037ffffb8ea894c
[ 49.505489] R13: ffff88810a7b7dd8 R14: ffffed10214f6fbc R15:
ffff88810a7b7de0
[ 49.506470] FS: 0000000000000000(0000) GS:ffff888119d00000(0000)
knlGS:0000000000000000
[ 49.507497] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 49.508200] CR2: ffff88811948dfff CR3: 0000000005286001 CR4:
0000000000770ee0
[ 49.509078] DR0: ffffffff8fdd6ce8 DR1: ffffffff8fdd6ce9 DR2:
ffffffff8fdd6cea
[ 49.509909] DR3: ffffffff8fdd6ceb DR6: 00000000fffe0ff0 DR7:
0000000000000600
[ 49.510790] PKRU: 55555554
[ 49.511127] Call Trace:
[ 49.511448] <TASK>
[ 49.511715] ? die_addr+0x3d/0xa0
[ 49.512133] ? exc_general_protection+0x144/0x220
[ 49.512725] ? asm_exc_general_protection+0x22/0x30
[ 49.513323] ? do_exit+0x125f/0x2240
[ 49.513785] ? kunit_free_suite_set+0x8e/0x150
[ 49.514330] ? _raw_spin_lock_irqsave+0x8d/0xe0
[ 49.514901] kunit_remove_resource+0x191/0x2a0
[ 49.515464] ? __sched_text_end+0xa/0xa
[ 49.515949] ? __sched_text_end+0xa/0xa
[ 49.516432] kunit_cleanup+0x6f/0x110
[ 49.516885] ? kunit_cleanup+0x110/0x110
[ 49.517374] kunit_generic_run_threadfn_adapter+0x4a/0x90
[ 49.518045] ? kunit_try_catch_throw+0x80/0x80
[ 49.518623] kthread+0x2b5/0x380
[ 49.519032] ? kthread_complete_and_exit+0x20/0x20
[ 49.519626] ret_from_fork+0x2d/0x70
[ 49.520078] ? kthread_complete_and_exit+0x20/0x20
[ 49.520679] ret_from_fork_asm+0x11/0x20
[ 49.521179] </TASK>
[ 49.521459] Modules linked in:
[ 49.521849] Dumping ftrace buffer:
[ 49.522273] (ftrace buffer empty)
[ 49.522764] ---[ end trace 0000000000000000 ]---
[ 49.523416] RIP: 0010:kunit_free_suite_set+0x8e/0x150
[ 49.524049] Code: 4d 89 fe 49 c1 ee 03 49 01 de 48 89 e8 48 c1 e8 03
80 3c 18 00 75 7d 4c 8b 65 00 49 8d bc 24 20 01 00 00 48 89 f8 48 c1 e8
03 <80> 3c 18 00 0f 85 81 00 00 00 49 8b bc 24 20 01 00 00 e8 2b 90 e0
[ 49.526357] RSP: 0000:ffff8881047f7e18 EFLAGS: 00010207
[ 49.527026] RAX: 0006fffff71d514d RBX: dffffc0000000000 RCX:
1ffff11020814d9e
[ 49.527898] RDX: 1ffff110214f6fbc RSI: 0000000000000004 RDI:
0037ffffb8ea8a6c
[ 49.528765] RBP: ffffffff811d098f R08: 0000000000000001 R09:
ffffed1020814d99
[ 49.529633] R10: ffff8881040a6ccb R11: 0000000000000400 R12:
0037ffffb8ea894c
[ 49.530524] R13: ffff88810a7b7dd8 R14: ffffed10214f6fbc R15:
ffff88810a7b7de0
[ 49.531405] FS: 0000000000000000(0000) GS:ffff888119d00000(0000)
knlGS:0000000000000000
[ 49.532386] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 49.533094] CR2: ffff88811948dfff CR3: 0000000005286001 CR4:
0000000000770ee0
[ 49.533971] DR0: ffffffff8fdd6ce8 DR1: ffffffff8fdd6ce9 DR2:
ffffffff8fdd6cea
[ 49.534879] DR3: ffffffff8fdd6ceb DR6: 00000000fffe0ff0 DR7:
0000000000000600
[ 49.535774] PKRU: 55555554
[ 49.536108] Kernel panic - not syncing: Fatal exception
[ 49.537673] Dumping ftrace buffer:
[ 49.538076] (ftrace buffer empty)
[ 49.538531] Kernel Offset: disabled
[ 49.538963] Rebooting in 1 seconds..

[Jinjie Ruan]

The test_cases is not freed in kunit_free_suite_set().

And the copy pointer may be moved in kunit_filter_suites().

The filtered_suite and filtered_suite->test_cases allocated in the last
kunit_filter_attr_tests() in last inner for loop may be leaked if
kunit_filter_suites() fails.

If kunit_filter_suites() succeeds, not only copy but also filtered_suite
and filtered_suite->test_cases should be freed.

Changes in v2:
- Add Reviewed-by.
- Add the memory leak backtrace for the 4th patch.
- Remove the unused func kernel test robot noticed for the 4th patch.
- Update the commit message for the 4th patch.

Jinjie Ruan (4):
kunit: Fix missed memory release in kunit_free_suite_set()
kunit: Fix the wrong kfree of copy for kunit_filter_suites()

kunit: Fix possible memory leak in kunit_filter_suites()

kunit: test: Fix the possible memory leak in executor_test

lib/kunit/executor.c | 23 ++++++++++++++++------
lib/kunit/executor_test.c | 40 ++++++++++++++++++---------------------
2 files changed, 35 insertions(+), 28 deletions(-)

--
2.34.1

[Jinjie Ruan]

Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites")

Fixes: b67abaad4d25 ("kunit: Allow kunit test modules to use test filtering")
Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Reviewed-by: Rae Moar <rm...@google.com>
---
v2:
- Add Reviewed-by.

---
lib/kunit/executor.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index a6348489d45f..a037a46fae5e 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c

@@ -137,8 +137,10 @@ void kunit_free_suite_set(struct kunit_suite_set suite_set)
{

struct kunit_suite * const *suites;

[Jinjie Ruan]

If the outer layer for loop is iterated more than once and it fails not

in the first iteration, the filtered_suite and filtered_suite->test_cases

allocated in the last kunit_filter_attr_tests() in last inner for loop

is leaked.

So add a new free_filtered_suite err label and free the filtered_suite
and filtered_suite->test_cases so far. And change kmalloc_array of copy
to kcalloc to Clear the copy to make the kfree safe.

Fixes: 5d31f71efcb6 ("kunit: add kunit.filter_glob cmdline option to filter suites")

Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")

Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Reviewed-by: Rae Moar <rm...@google.com>
---
v2:
- Add Reviewed-by.
---

lib/kunit/executor.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 9358ed2df839..1236b3cd2fbb 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c

[Jinjie Ruan]

If the outer layer for loop is iterated more than once and it fails not

in the first iteration, the copy pointer has been moved. So it should free
the original copy's backup copy_start.

Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")

Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Reviewed-by: Rae Moar <rm...@google.com>
---
v2:
- Add Reviewed-by.
---

lib/kunit/executor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index a037a46fae5e..9358ed2df839 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c

[Jinjie Ruan]

When CONFIG_KUNIT_ALL_TESTS=y, making CONFIG_DEBUG_KMEMLEAK=y and
CONFIG_DEBUG_KMEMLEAK_AUTO_SCAN=y, the below memory leak is detected.

If kunit_filter_suites() succeeds, not only copy but also filtered_suite
and filtered_suite->test_cases should be freed.

So use kunit_free_suite_set() to free the filtered_suite,
filtered_suite->test_cases and copy as kunit_module_exit() and

kunit_run_all_tests() do it. And the func kfree_at_end() is not used so
remove it. After applying this patch, the following memory leak is never
detected.

unreferenced object 0xffff8881001de400 (size 1024):
comm "kunit_try_catch", pid 1396, jiffies 4294720452 (age 932.801s)

hex dump (first 32 bytes):

73 75 69 74 65 32 00 00 00 00 00 00 00 00 00 00 suite2..........

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829e961d>] kunit_filter_suites+0x44d/0xcc0
[<ffffffff829eb69f>] filter_suites_test+0x12f/0x360
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff8881052cd388 (size 192):
comm "kunit_try_catch", pid 1396, jiffies 4294720452 (age 932.801s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff 80 cd 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829e9651>] kunit_filter_suites+0x481/0xcc0
[<ffffffff829eb69f>] filter_suites_test+0x12f/0x360
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20

unreferenced object 0xffff888100da8400 (size 1024):
comm "kunit_try_catch", pid 1398, jiffies 4294720454 (age 781.945s)

hex dump (first 32 bytes):

73 75 69 74 65 32 00 00 00 00 00 00 00 00 00 00 suite2..........

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829e961d>] kunit_filter_suites+0x44d/0xcc0
[<ffffffff829eb13f>] filter_suites_test_glob_test+0x12f/0x560
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff888105117878 (size 96):
comm "kunit_try_catch", pid 1398, jiffies 4294720454 (age 781.945s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff a0 ac 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829e9651>] kunit_filter_suites+0x481/0xcc0
[<ffffffff829eb13f>] filter_suites_test_glob_test+0x12f/0x560
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff888102c31c00 (size 1024):
comm "kunit_try_catch", pid 1404, jiffies 4294720460 (age 781.948s)

hex dump (first 32 bytes):

6e 6f 72 6d 61 6c 5f 73 75 69 74 65 00 00 00 00 normal_suite....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829ecf17>] kunit_filter_attr_tests+0xf7/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829ea975>] filter_attr_test+0x195/0x5f0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff8881052cd250 (size 192):
comm "kunit_try_catch", pid 1404, jiffies 4294720460 (age 781.948s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff 00 a9 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829ecfc1>] kunit_filter_attr_tests+0x1a1/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829ea975>] filter_attr_test+0x195/0x5f0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff888104f4e400 (size 1024):
comm "kunit_try_catch", pid 1408, jiffies 4294720464 (age 781.944s)

hex dump (first 32 bytes):

73 75 69 74 65 00 00 00 00 00 00 00 00 00 00 00 suite...........

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829ecf17>] kunit_filter_attr_tests+0xf7/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829e9fc3>] filter_attr_skip_test+0x133/0x6e0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff8881052cc620 (size 192):
comm "kunit_try_catch", pid 1408, jiffies 4294720464 (age 781.944s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff c0 a8 7c 84 ff ff ff ff ..........|.....
00 00 00 00 00 00 00 00 02 00 00 00 02 00 00 00 ................
backtrace:
[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829ecfc1>] kunit_filter_attr_tests+0x1a1/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829e9fc3>] filter_attr_skip_test+0x133/0x6e0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20

Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites")
Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>

Reported-by: kernel test robot <l...@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202309142251...@intel.com/

---
v2:
- Add the memory leak backtrace.
- Remove the unused func kfree_at_end() kernel test robot noticed.
- Update the commit message.
---
lib/kunit/executor_test.c | 40 ++++++++++++++++++---------------------
1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index b4f6f96b2844..88d26c9cdce8 100644
--- a/lib/kunit/executor_test.c
+++ b/lib/kunit/executor_test.c
@@ -9,7 +9,6 @@
#include <kunit/test.h>
#include <kunit/attributes.h>

-static void kfree_at_end(struct kunit *test, const void *to_free);
static struct kunit_suite *alloc_fake_suite(struct kunit *test,
const char *suite_name,
struct kunit_case *test_cases);
@@ -56,7 +55,6 @@ static void filter_suites_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, "suite2", NULL, NULL, &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

/* Validate we just have suite2 */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);

@@ -64,6 +62,9 @@ static void filter_suites_test(struct kunit *test)

/* Contains one element (end is 1 past end) */
KUNIT_ASSERT_EQ(test, got.end - got.start, 1);
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void filter_suites_test_glob_test(struct kunit *test)

@@ -82,7 +83,6 @@ static void filter_suites_test_glob_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, "suite2.test2", NULL, NULL, &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

/* Validate we just have suite2 */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);

@@ -93,6 +93,9 @@ static void filter_suites_test_glob_test(struct kunit *test)

KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test2");
KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void filter_suites_to_empty_test(struct kunit *test)

@@ -109,10 +112,12 @@ static void filter_suites_to_empty_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, "not_found", NULL, NULL, &err);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start); /* just in case */

KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
"should be empty to indicate no match");
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void parse_filter_attr_test(struct kunit *test)

@@ -172,7 +177,6 @@ static void filter_attr_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

/* Validate we just have normal_suite */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);

@@ -183,6 +187,9 @@ static void filter_attr_test(struct kunit *test)

KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
KUNIT_EXPECT_STREQ(test, got.start[0]->test_cases[0].name, "normal");
KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void filter_attr_empty_test(struct kunit *test)

@@ -200,10 +207,12 @@ static void filter_attr_empty_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start); /* just in case */

KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
"should be empty to indicate no match");
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static void filter_attr_skip_test(struct kunit *test)

@@ -222,7 +231,6 @@ static void filter_attr_skip_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, NULL, filter, "skip", &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

/* Validate we have both the slow and normal test */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);

@@ -233,6 +241,9 @@ static void filter_attr_skip_test(struct kunit *test)

/* Now ensure slow is skipped and normal is not */
KUNIT_EXPECT_EQ(test, got.start[0]->test_cases[0].status, KUNIT_SKIPPED);
KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].status);
+
+ if (!err)
+ kunit_free_suite_set(got);
}

static struct kunit_case executor_test_cases[] = {

@@ -255,21 +266,6 @@ static struct kunit_suite executor_test_suite = {
kunit_test_suites(&executor_test_suite);

/* Test helpers */
-
-/* Use the resource API to register a call to kfree(to_free).
- * Since we never actually use the resource, it's safe to use on const data.
- */
-static void kfree_at_end(struct kunit *test, const void *to_free)
-{
- /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */
- if (IS_ERR_OR_NULL(to_free))
- return;
-
- kunit_add_action(test,
- (kunit_action_t *)kfree,
- (void *)to_free);
-}
-
static struct kunit_suite *alloc_fake_suite(struct kunit *test,
const char *suite_name,
struct kunit_case *test_cases)
--
2.34.1

[Rae Moar]

Hello!

Thanks for sending out a new version and responding to my comments. I
do have one issue below.

Thanks!
-Rae

If filtering incorrectly outputs more than one suite, the line above
will fail. This will cause the test to exit immediately because this
is an ASSERT statement instead of an EXPECT statement.

If this happens, the suite set will never be freed. Instead we should
change this to KUNIT_EXPECT_EQ so the test will continue to the below
if statement.

We should change this for all similar lines where we still want to
free the suite if they fail.

> +
> + if (!err)
> + kunit_free_suite_set(got);

> }
>
> static void filter_suites_test_glob_test(struct kunit *test)
> @@ -82,7 +83,6 @@ static void filter_suites_test_glob_test(struct kunit *test)
> got = kunit_filter_suites(&suite_set, "suite2.test2", NULL, NULL, &err);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> KUNIT_ASSERT_EQ(test, err, 0);
> - kfree_at_end(test, got.start);
>
> /* Validate we just have suite2 */
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
> @@ -93,6 +93,9 @@ static void filter_suites_test_glob_test(struct kunit *test)
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
> KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test2");
> KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
> +
> + if (!err)
> + kunit_free_suite_set(got);

Again I recommend changing the line in this test from
"KUNIT_ASSERT_EQ(test, got.end - got.start, 1);" to an EXPECT
statement.

> }
>
> static void filter_suites_to_empty_test(struct kunit *test)
> @@ -109,10 +112,12 @@ static void filter_suites_to_empty_test(struct kunit *test)
>
> got = kunit_filter_suites(&suite_set, "not_found", NULL, NULL, &err);
> KUNIT_ASSERT_EQ(test, err, 0);
> - kfree_at_end(test, got.start); /* just in case */
>
> KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
> "should be empty to indicate no match");
> +
> + if (!err)
> + kunit_free_suite_set(got);

This test seems good.

> }
>
> static void parse_filter_attr_test(struct kunit *test)
> @@ -172,7 +177,6 @@ static void filter_attr_test(struct kunit *test)
> got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> KUNIT_ASSERT_EQ(test, err, 0);
> - kfree_at_end(test, got.start);
>
> /* Validate we just have normal_suite */
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
> @@ -183,6 +187,9 @@ static void filter_attr_test(struct kunit *test)
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
> KUNIT_EXPECT_STREQ(test, got.start[0]->test_cases[0].name, "normal");
> KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
> +
> + if (!err)
> + kunit_free_suite_set(got);

Again I recommend changing the line in this test from
"KUNIT_ASSERT_EQ(test, got.end - got.start, 1);" to an EXPECT
statement.

> }
>
> static void filter_attr_empty_test(struct kunit *test)
> @@ -200,10 +207,12 @@ static void filter_attr_empty_test(struct kunit *test)
>
> got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
> KUNIT_ASSERT_EQ(test, err, 0);
> - kfree_at_end(test, got.start); /* just in case */
>
> KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
> "should be empty to indicate no match");
> +
> + if (!err)
> + kunit_free_suite_set(got);

This test seems good.

> }
>
> static void filter_attr_skip_test(struct kunit *test)
> @@ -222,7 +231,6 @@ static void filter_attr_skip_test(struct kunit *test)
> got = kunit_filter_suites(&suite_set, NULL, filter, "skip", &err);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> KUNIT_ASSERT_EQ(test, err, 0);
> - kfree_at_end(test, got.start);
>
> /* Validate we have both the slow and normal test */
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
> @@ -233,6 +241,9 @@ static void filter_attr_skip_test(struct kunit *test)
> /* Now ensure slow is skipped and normal is not */
> KUNIT_EXPECT_EQ(test, got.start[0]->test_cases[0].status, KUNIT_SKIPPED);
> KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].status);
> +
> + if (!err)
> + kunit_free_suite_set(got);

Similarly, the line "KUNIT_ASSERT_EQ(test,
kunit_suite_num_test_cases(got.start[0]), 2)" may exit causing the
suite to not be freed. This should be changed to an EXPECT statement.
However, we may then want to check before accessing the test cases.

> }
>
> static struct kunit_case executor_test_cases[] = {
> @@ -255,21 +266,6 @@ static struct kunit_suite executor_test_suite = {
> kunit_test_suites(&executor_test_suite);
>
> /* Test helpers */
> -
> -/* Use the resource API to register a call to kfree(to_free).
> - * Since we never actually use the resource, it's safe to use on const data.
> - */
> -static void kfree_at_end(struct kunit *test, const void *to_free)
> -{
> - /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */
> - if (IS_ERR_OR_NULL(to_free))
> - return;
> -
> - kunit_add_action(test,
> - (kunit_action_t *)kfree,
> - (void *)to_free);
> -}
> -
> static struct kunit_suite *alloc_fake_suite(struct kunit *test,
> const char *suite_name,
> struct kunit_case *test_cases)
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230921014008.3887257-5-ruanjinjie%40huawei.com.

[Ruan Jinjie]

Right! It is better to update the kfree_at_end() func to solve these
problems. I'll try to do it sooner.

[Jinjie Ruan]

modprobe cpumask_kunit and rmmod cpumask_kunit, kmemleak detect
a suspected memory leak as below.

If kunit_filter_suites() in kunit_module_init() succeeds, the
suite_set.start will not be NULL and the kunit_free_suite_set() in
kunit_module_exit() should free all the memory which has not
been freed. However the test_cases in suites is left out.

unreferenced object 0xffff54ac47e83200 (size 512):
comm "modprobe", pid 592, jiffies 4294913238 (age 1367.612s)

hex dump (first 32 bytes):

84 13 1a f0 d3 b6 ff ff 30 68 1a f0 d3 b6 ff ff ........0h......

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<000000008dec63a2>] slab_post_alloc_hook+0xb8/0x368
[<00000000ec280d8e>] __kmem_cache_alloc_node+0x174/0x290
[<00000000896c7740>] __kmalloc+0x60/0x2c0
[<000000007a50fa06>] kunit_filter_suites+0x254/0x5b8
[<0000000078cc98e2>] kunit_module_notify+0xf4/0x240
[<0000000033cea952>] notifier_call_chain+0x98/0x17c
[<00000000973d05cc>] notifier_call_chain_robust+0x4c/0xa4
[<000000005f95895f>] blocking_notifier_call_chain_robust+0x4c/0x74
[<0000000048e36fa7>] load_module+0x1a2c/0x1c40
[<0000000004eb8a91>] init_module_from_file+0x94/0xcc
[<0000000037dbba28>] idempotent_init_module+0x184/0x278
[<00000000161b75cb>] __arm64_sys_finit_module+0x68/0xa8
[<000000006dc1669b>] invoke_syscall+0x44/0x100
[<00000000fa87e304>] el0_svc_common.constprop.1+0x68/0xe0
[<000000009d8ad866>] do_el0_svc+0x1c/0x28
[<000000005b83c607>] el0_svc+0x3c/0xc4

Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites")

Fixes: b67abaad4d25 ("kunit: Allow kunit test modules to use test filtering")
Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>

Reviewed-by: Rae Moar <rm...@google.com>
---
v2:
- Add Reviewed-by.
---

lib/kunit/executor.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index a6348489d45f..a037a46fae5e 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c

@@ -137,8 +137,10 @@ void kunit_free_suite_set(struct kunit_suite_set suite_set)
{

struct kunit_suite * const *suites;

[Jinjie Ruan]

If the outer layer for loop is iterated more than once and it fails not
in the first iteration, the copy pointer has been moved. So it should free
the original copy's backup copy_start.

Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")

Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Reviewed-by: Rae Moar <rm...@google.com>
---
v2:
- Add Reviewed-by.
---

lib/kunit/executor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index a037a46fae5e..9358ed2df839 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c

[Jinjie Ruan]

If the outer layer for loop is iterated more than once and it fails not

in the first iteration, the filtered_suite and filtered_suite->test_cases
allocated in the last kunit_filter_attr_tests() in last inner for loop
is leaked.

So add a new free_filtered_suite err label and free the filtered_suite
and filtered_suite->test_cases so far. And change kmalloc_array of copy
to kcalloc to Clear the copy to make the kfree safe.

Fixes: 5d31f71efcb6 ("kunit: add kunit.filter_glob cmdline option to filter suites")

Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")

Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Reviewed-by: Rae Moar <rm...@google.com>
---
v2:
- Add Reviewed-by.
---

lib/kunit/executor.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 9358ed2df839..1236b3cd2fbb 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c

[Jinjie Ruan]

When CONFIG_KUNIT_ALL_TESTS=y, making CONFIG_DEBUG_KMEMLEAK=y and
CONFIG_DEBUG_KMEMLEAK_AUTO_SCAN=y, the below memory leak is detected.

If kunit_filter_suites() succeeds, not only copy but also filtered_suite
and filtered_suite->test_cases should be freed.

So as Rae suggested, to avoid the suite set never be freed when
KUNIT_ASSERT_EQ() fails and exits after kunit_filter_suites() succeeds,
update kfree_at_end() func to free_suite_set_at_end() to use
kunit_free_suite_set() to free them as kunit_module_exit() and
kunit_run_all_tests() do it. As the second arg got of
free_suite_set_at_end() is a local variable, copy it for free to avoid
wild-memory-access. After applying this patch, the following memory leak

is never detected.

unreferenced object 0xffff8881001de400 (size 1024):
comm "kunit_try_catch", pid 1396, jiffies 4294720452 (age 932.801s)

hex dump (first 32 bytes):

73 75 69 74 65 32 00 00 00 00 00 00 00 00 00 00 suite2..........

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829e961d>] kunit_filter_suites+0x44d/0xcc0
[<ffffffff829eb69f>] filter_suites_test+0x12f/0x360
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff8881052cd388 (size 192):
comm "kunit_try_catch", pid 1396, jiffies 4294720452 (age 932.801s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff 80 cd 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829e9651>] kunit_filter_suites+0x481/0xcc0
[<ffffffff829eb69f>] filter_suites_test+0x12f/0x360
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20

unreferenced object 0xffff888100da8400 (size 1024):
comm "kunit_try_catch", pid 1398, jiffies 4294720454 (age 781.945s)

hex dump (first 32 bytes):

73 75 69 74 65 32 00 00 00 00 00 00 00 00 00 00 suite2..........

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829e961d>] kunit_filter_suites+0x44d/0xcc0
[<ffffffff829eb13f>] filter_suites_test_glob_test+0x12f/0x560
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff888105117878 (size 96):
comm "kunit_try_catch", pid 1398, jiffies 4294720454 (age 781.945s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff a0 ac 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829e9651>] kunit_filter_suites+0x481/0xcc0
[<ffffffff829eb13f>] filter_suites_test_glob_test+0x12f/0x560
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff888102c31c00 (size 1024):
comm "kunit_try_catch", pid 1404, jiffies 4294720460 (age 781.948s)

hex dump (first 32 bytes):

6e 6f 72 6d 61 6c 5f 73 75 69 74 65 00 00 00 00 normal_suite....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829ecf17>] kunit_filter_attr_tests+0xf7/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829ea975>] filter_attr_test+0x195/0x5f0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff8881052cd250 (size 192):
comm "kunit_try_catch", pid 1404, jiffies 4294720460 (age 781.948s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff 00 a9 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829ecfc1>] kunit_filter_attr_tests+0x1a1/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829ea975>] filter_attr_test+0x195/0x5f0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff888104f4e400 (size 1024):
comm "kunit_try_catch", pid 1408, jiffies 4294720464 (age 781.944s)

hex dump (first 32 bytes):

73 75 69 74 65 00 00 00 00 00 00 00 00 00 00 00 suite...........

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829ecf17>] kunit_filter_attr_tests+0xf7/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829e9fc3>] filter_attr_skip_test+0x133/0x6e0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff8881052cc620 (size 192):
comm "kunit_try_catch", pid 1408, jiffies 4294720464 (age 781.944s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff c0 a8 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 02 00 00 00 02 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829ecfc1>] kunit_filter_attr_tests+0x1a1/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829e9fc3>] filter_attr_skip_test+0x133/0x6e0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20

Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites")
Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Suggested-by: Rae Moar <rm...@google.com>

Reported-by: kernel test robot <l...@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202309142251...@intel.com/

---
v3:
- Update the kfree_at_end() to use kunit_free_suite_set() instead calling it
directly.

- Update the commit message.

- Add Suggested-by.

v2:
- Add the memory leak backtrace.
- Remove the unused func kfree_at_end() kernel test robot noticed.
- Update the commit message.
---

lib/kunit/executor_test.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index b4f6f96b2844..6b68959def9d 100644
--- a/lib/kunit/executor_test.c
+++ b/lib/kunit/executor_test.c
@@ -9,7 +9,7 @@

#include <kunit/test.h>
#include <kunit/attributes.h>

-static void kfree_at_end(struct kunit *test, const void *to_free);

+static void free_suite_set_at_end(struct kunit *test, const void *to_free);

static struct kunit_suite *alloc_fake_suite(struct kunit *test,
const char *suite_name,
struct kunit_case *test_cases);

@@ -56,7 +56,7 @@ static void filter_suites_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, "suite2", NULL, NULL, &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

+ free_suite_set_at_end(test, &got);

/* Validate we just have suite2 */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);

@@ -82,7 +82,7 @@ static void filter_suites_test_glob_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, "suite2.test2", NULL, NULL, &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

+ free_suite_set_at_end(test, &got);

/* Validate we just have suite2 */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);

@@ -109,7 +109,7 @@ static void filter_suites_to_empty_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, "not_found", NULL, NULL, &err);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start); /* just in case */

+ free_suite_set_at_end(test, &got); /* just in case */

KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
"should be empty to indicate no match");

@@ -172,7 +172,7 @@ static void filter_attr_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

+ free_suite_set_at_end(test, &got);

/* Validate we just have normal_suite */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);

@@ -200,7 +200,7 @@ static void filter_attr_empty_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start); /* just in case */

+ free_suite_set_at_end(test, &got); /* just in case */

KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
"should be empty to indicate no match");

@@ -222,7 +222,7 @@ static void filter_attr_skip_test(struct kunit *test)

got = kunit_filter_suites(&suite_set, NULL, filter, "skip", &err);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
KUNIT_ASSERT_EQ(test, err, 0);
- kfree_at_end(test, got.start);

+ free_suite_set_at_end(test, &got);

/* Validate we have both the slow and normal test */
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);

@@ -256,18 +256,27 @@ kunit_test_suites(&executor_test_suite);

/* Test helpers */

-/* Use the resource API to register a call to kfree(to_free).

+static void free_suite_set(struct kunit_suite_set *suite_set)
+{
+ kunit_free_suite_set(*suite_set);
+ kfree(suite_set);
+}
+
+/* Use the resource API to register a call to free_suite_set.

* Since we never actually use the resource, it's safe to use on const data.

*/
-static void kfree_at_end(struct kunit *test, const void *to_free)

+static void free_suite_set_at_end(struct kunit *test, const void *to_free)

{
- /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */
- if (IS_ERR_OR_NULL(to_free))

+ if (!((struct kunit_suite_set *)to_free)->start)
return;

+ struct kunit_suite_set *free = kzalloc(sizeof(struct kunit_suite_set),
+ GFP_KERNEL);
+ *free = *(struct kunit_suite_set *)to_free;
+

kunit_add_action(test,
- (kunit_action_t *)kfree,
- (void *)to_free);

+ (kunit_action_t *)free_suite_set,
+ (void *)free);

}

static struct kunit_suite *alloc_fake_suite(struct kunit *test,

--
2.34.1

[Jinjie Ruan]

The test_cases is not freed in kunit_free_suite_set().

And the copy pointer may be moved in kunit_filter_suites().

The filtered_suite and filtered_suite->test_cases allocated in the last
kunit_filter_attr_tests() in last inner for loop may be leaked if
kunit_filter_suites() fails.

If kunit_filter_suites() succeeds, not only copy but also filtered_suite
and filtered_suite->test_cases should be freed.

Changes in v3:
- Update the kfree_at_end() to use kunit_free_suite_set() for 4th patch.

- Update the commit message for the 4th patch.

Changes in v2:
- Add Reviewed-by.
- Add the memory leak backtrace for the 4th patch.
- Remove the unused func kernel test robot noticed for the 4th patch.
- Update the commit message for the 4th patch.

Jinjie Ruan (4):
kunit: Fix missed memory release in kunit_free_suite_set()
kunit: Fix the wrong kfree of copy for kunit_filter_suites()
kunit: Fix possible memory leak in kunit_filter_suites()
kunit: test: Fix the possible memory leak in executor_test

lib/kunit/executor.c | 23 +++++++++++++++++------
lib/kunit/executor_test.c | 35 ++++++++++++++++++++++-------------
2 files changed, 39 insertions(+), 19 deletions(-)

--
2.34.1

[David Gow]

On Thu, 21 Sept 2023 at 09:41, 'Jinjie Ruan' via KUnit Development

Looks good to me.

Reviewed-by: David Gow <davi...@google.com>

> lib/kunit/executor.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index a6348489d45f..a037a46fae5e 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -137,8 +137,10 @@ void kunit_free_suite_set(struct kunit_suite_set suite_set)
> {
> struct kunit_suite * const *suites;
>
> - for (suites = suite_set.start; suites < suite_set.end; suites++)
> + for (suites = suite_set.start; suites < suite_set.end; suites++) {
> + kfree((*suites)->test_cases);
> kfree(*suites);
> + }
> kfree(suite_set.start);
> }
>
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230921014008.3887257-2-ruanjinjie%40huawei.com.

[David Gow]

On Thu, 21 Sept 2023 at 09:41, 'Jinjie Ruan' via KUnit Development
<kuni...@googlegroups.com> wrote:
>

> If the outer layer for loop is iterated more than once and it fails not
> in the first iteration, the copy pointer has been moved. So it should free
> the original copy's backup copy_start.
>
> Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")
> Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
> Reviewed-by: Rae Moar <rm...@google.com>
> ---
> v2:
> - Add Reviewed-by.
> ---

Nice catch. Thanks!

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

> lib/kunit/executor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index a037a46fae5e..9358ed2df839 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -243,7 +243,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>
> free_copy:
> if (*err)
> - kfree(copy);
> + kfree(copy_start);
>
> return filtered;
> }
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230921014008.3887257-3-ruanjinjie%40huawei.com.

[David Gow]

On Thu, 21 Sept 2023 at 09:41, 'Jinjie Ruan' via KUnit Development
<kuni...@googlegroups.com> wrote:
>

> If the outer layer for loop is iterated more than once and it fails not
> in the first iteration, the filtered_suite and filtered_suite->test_cases
> allocated in the last kunit_filter_attr_tests() in last inner for loop
> is leaked.
>
> So add a new free_filtered_suite err label and free the filtered_suite
> and filtered_suite->test_cases so far. And change kmalloc_array of copy
> to kcalloc to Clear the copy to make the kfree safe.
>
> Fixes: 5d31f71efcb6 ("kunit: add kunit.filter_glob cmdline option to filter suites")
> Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
> Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
> Reviewed-by: Rae Moar <rm...@google.com>
> ---
> v2:
> - Add Reviewed-by.
> ---

This looks good to me, though I admit that this code is starting to
get a bit too complicated...

A few thoughts below, but they're more notes-to-self for a future
refactoring than something I think this patch needs.

Reviewed-by: David Gow <davi...@google.com>

-- David

Do we really need both filtered.start and copy_start, and filtered.end
/ copy? The only case where they're different would be when an error
occurs, and it should be easy to simply reset them to NULL then,
anyway.

>
> +free_filtered_suite:
> + if (*err) {
> + for (suites = copy_start; suites < copy; suites++) {
> + kfree((*suites)->test_cases);
> + kfree(*suites);
> + }

We possibly should set filtered = {NULL, NULL} here. It's not actually
possible for them to be non-NULL at this point, so it is redundant,
but it's not easy to tell (and it looks like this could be returning a
freed pointer here, even though it's not).

> + }
> +
> free_parsed_filters:
> if (filter_count)
> kfree(parsed_filters);
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230921014008.3887257-4-ruanjinjie%40huawei.com.

[David Gow]

On Thu, 21 Sept 2023 at 09:41, 'Jinjie Ruan' via KUnit Development
<kuni...@googlegroups.com> wrote:
>

We have some plans to reintroduce a similar function later as a
general kunit feature, but it's fine removing it here.

> - Update the commit message.
> ---

This mostly looks good, but as Rae pointed out, the cleanup won't get
called if some of the assertions fail.

Using something more like kfree_at_end(), such as
kunit_add_action(test, (kunit_action_t *)kunit_free_suite_set, got)
would resolve all of these issues.

(You may need to write a wrapper around kunit_free_suite_set to make
it work as an action if you go down that path.)

Cheers,
-- David

Because of the KUNIT_ASSERT_EQ(test, err, 0) call above, we know err
is nonzero here, so this conditional shouldn't be required. But it
also wouldn't be if you used a deferred action to clean up.

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230921014008.3887257-5-ruanjinjie%40huawei.com.

[Ruan Jinjie]

Right! I do it in v3 and the got is a local struct so there is a little
problem. Thank you very much!

[David Gow]

On Fri, 22 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development

Ah, I like this much more than v2, thanks!

The need to make a new struct kunit_suite_set so it stays in scope is
a bit ugly, but is probably the best we can do.

My only suggestion is that we make free_suite_set() take a void *,
which would let us avoid to kunit_action_t function pointer cast,
which will break CFI, and result in some warnings with clang 16+ and
W=1.
See:
https://lore.kernel.org/all/20230915050125.3...@google.com/

(The existing code was already broken, so I'm happy to accept this
as-is, and fix it separately if you prefer.)

Otherwise,

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

If this accepted suite_set as a void *...

> +{
> + kunit_free_suite_set(*suite_set);

(And casted it to struct kunit_suite_set * here).

> + kfree(suite_set);
> +}
> +
> +/* Use the resource API to register a call to free_suite_set.
> * Since we never actually use the resource, it's safe to use on const data.
> */
> -static void kfree_at_end(struct kunit *test, const void *to_free)
> +static void free_suite_set_at_end(struct kunit *test, const void *to_free)
> {
> - /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */
> - if (IS_ERR_OR_NULL(to_free))
> + if (!((struct kunit_suite_set *)to_free)->start)
> return;
>
> + struct kunit_suite_set *free = kzalloc(sizeof(struct kunit_suite_set),
> + GFP_KERNEL);
> + *free = *(struct kunit_suite_set *)to_free;
> +
> kunit_add_action(test,
> - (kunit_action_t *)kfree,
> - (void *)to_free);
> + (kunit_action_t *)free_suite_set,

...we could get rid of this cast.

> + (void *)free);
> }
>
> static struct kunit_suite *alloc_fake_suite(struct kunit *test,
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230922071020.2554677-5-ruanjinjie%40huawei.com.

[David Gow]

On Fri, 22 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development
<kuni...@googlegroups.com> wrote:
>

Whoops -- v3 came through while I was reviewing v2.

This looks good, thanks.

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

> lib/kunit/executor.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index a6348489d45f..a037a46fae5e 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -137,8 +137,10 @@ void kunit_free_suite_set(struct kunit_suite_set suite_set)
> {
> struct kunit_suite * const *suites;
>
> - for (suites = suite_set.start; suites < suite_set.end; suites++)
> + for (suites = suite_set.start; suites < suite_set.end; suites++) {
> + kfree((*suites)->test_cases);
> kfree(*suites);
> + }
> kfree(suite_set.start);
> }
>
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230922071020.2554677-2-ruanjinjie%40huawei.com.

[David Gow]

On Fri, 22 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development
<kuni...@googlegroups.com> wrote:
>

> If the outer layer for loop is iterated more than once and it fails not
> in the first iteration, the copy pointer has been moved. So it should free
> the original copy's backup copy_start.
>
> Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")
> Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
> Reviewed-by: Rae Moar <rm...@google.com>
> ---
> v2:
> - Add Reviewed-by.
> ---

Like v2, this looks good.

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

> lib/kunit/executor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index a037a46fae5e..9358ed2df839 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -243,7 +243,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>
> free_copy:
> if (*err)
> - kfree(copy);
> + kfree(copy_start);
>
> return filtered;
> }
> --
> 2.34.1
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230922071020.2554677-3-ruanjinjie%40huawei.com.

[David Gow]

On Fri, 22 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development
<kuni...@googlegroups.com> wrote:
>

> If the outer layer for loop is iterated more than once and it fails not
> in the first iteration, the filtered_suite and filtered_suite->test_cases
> allocated in the last kunit_filter_attr_tests() in last inner for loop
> is leaked.
>
> So add a new free_filtered_suite err label and free the filtered_suite
> and filtered_suite->test_cases so far. And change kmalloc_array of copy
> to kcalloc to Clear the copy to make the kfree safe.
>
> Fixes: 5d31f71efcb6 ("kunit: add kunit.filter_glob cmdline option to filter suites")
> Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
> Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
> Reviewed-by: Rae Moar <rm...@google.com>
> ---
> v2:
> - Add Reviewed-by.
> ---

This looks good to me. There are a couple of things in this code which
probably could be cleaned up (as I noted in v2), but that's something
we can do separately.

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230922071020.2554677-4-ruanjinjie%40huawei.com.

[kernel test robot]

Hi Jinjie,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.6-rc3 next-20230926]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Jinjie-Ruan/kunit-Fix-missed-memory-release-in-kunit_free_suite_set/20230922-151243
base: linus/master
patch link: https://lore.kernel.org/r/20230922071020.2554677-5-ruanjinjie%40huawei.com
patch subject: [PATCH v3 4/4] kunit: test: Fix the possible memory leak in executor_test
config: powerpc-allyesconfig (https://download.01.org/0day-ci/archive/20230927/202309270433...@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230927/202309270433...@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags

| Reported-by: kernel test robot <l...@intel.com>

| Closes: https://lore.kernel.org/oe-kbuild-all/202309270433...@intel.com/

All warnings (new ones prefixed by >>):

In file included from lib/kunit/executor.c:353:
>> lib/kunit/executor_test.c:278:4: warning: cast from 'void (*)(struct kunit_suite_set *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
278 | (kunit_action_t *)free_suite_set,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.


vim +278 lib/kunit/executor_test.c

264
265 /* Use the resource API to register a call to free_suite_set.
266 * Since we never actually use the resource, it's safe to use on const data.
267 */
268 static void free_suite_set_at_end(struct kunit *test, const void *to_free)
269 {
270 if (!((struct kunit_suite_set *)to_free)->start)
271 return;
272
273 struct kunit_suite_set *free = kzalloc(sizeof(struct kunit_suite_set),
274 GFP_KERNEL);
275 *free = *(struct kunit_suite_set *)to_free;
276
277 kunit_add_action(test,
> 278 (kunit_action_t *)free_suite_set,
279 (void *)free);
280 }
281

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[Jinjie Ruan]

If the outer layer for loop is iterated more than once and it fails not
in the first iteration, the filtered_suite and filtered_suite->test_cases
allocated in the last kunit_filter_attr_tests() in last inner for loop
is leaked.

So add a new free_filtered_suite err label and free the filtered_suite
and filtered_suite->test_cases so far. And change kmalloc_array of copy
to kcalloc to Clear the copy to make the kfree safe.

Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Reviewed-by: Rae Moar <rm...@google.com>

Reviewed-by: David Gow <davi...@google.com>
---
v4:
- Correct the fix tag.
- Add Reviewed-by.

v2:
- Add Reviewed-by.
---

[Jinjie Ruan]

The test_cases is not freed in kunit_free_suite_set().

And the copy pointer may be moved in kunit_filter_suites().

The filtered_suite and filtered_suite->test_cases allocated in the last
kunit_filter_attr_tests() in last inner for loop may be leaked if
kunit_filter_suites() fails.

If kunit_filter_suites() succeeds, not only copy but also filtered_suite
and filtered_suite->test_cases should be freed.

Changes in v4:
- Make free_suite_set() take a void * for the 4th patch.
- Add Suggested-by and Reviewed-by.

- Correct the fix tag.

Changes in v3:
- Update the kfree_at_end() to use kunit_free_suite_set() for 4th patch.
- Update the commit message for the 4th patch.

Changes in v2:
- Add Reviewed-by.
- Add the memory leak backtrace for the 4th patch.
- Remove the unused func kernel test robot noticed for the 4th patch.
- Update the commit message for the 4th patch.

Jinjie Ruan (4):
kunit: Fix missed memory release in kunit_free_suite_set()
kunit: Fix the wrong kfree of copy for kunit_filter_suites()

kunit: Fix possible memory leak in kunit_filter_suites()

kunit: test: Fix the possible memory leak in executor_test

lib/kunit/executor.c | 23 +++++++++++++++++------
lib/kunit/executor_test.c | 36 ++++++++++++++++++++++--------------
2 files changed, 39 insertions(+), 20 deletions(-)

--
2.34.1

[Jinjie Ruan]

When CONFIG_KUNIT_ALL_TESTS=y, making CONFIG_DEBUG_KMEMLEAK=y and
CONFIG_DEBUG_KMEMLEAK_AUTO_SCAN=y, the below memory leak is detected.

If kunit_filter_suites() succeeds, not only copy but also filtered_suite
and filtered_suite->test_cases should be freed.

So as Rae suggested, to avoid the suite set never be freed when
KUNIT_ASSERT_EQ() fails and exits after kunit_filter_suites() succeeds,
update kfree_at_end() func to free_suite_set_at_end() to use
kunit_free_suite_set() to free them as kunit_module_exit() and
kunit_run_all_tests() do it. As the second arg got of
free_suite_set_at_end() is a local variable, copy it for free to avoid
wild-memory-access. After applying this patch, the following memory leak
is never detected.

unreferenced object 0xffff8881001de400 (size 1024):
comm "kunit_try_catch", pid 1396, jiffies 4294720452 (age 932.801s)

hex dump (first 32 bytes):

73 75 69 74 65 32 00 00 00 00 00 00 00 00 00 00 suite2..........

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829e961d>] kunit_filter_suites+0x44d/0xcc0
[<ffffffff829eb69f>] filter_suites_test+0x12f/0x360
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff8881052cd388 (size 192):
comm "kunit_try_catch", pid 1396, jiffies 4294720452 (age 932.801s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff 80 cd 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829e9651>] kunit_filter_suites+0x481/0xcc0
[<ffffffff829eb69f>] filter_suites_test+0x12f/0x360
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20

unreferenced object 0xffff888100da8400 (size 1024):
comm "kunit_try_catch", pid 1398, jiffies 4294720454 (age 781.945s)

hex dump (first 32 bytes):

73 75 69 74 65 32 00 00 00 00 00 00 00 00 00 00 suite2..........

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829e961d>] kunit_filter_suites+0x44d/0xcc0
[<ffffffff829eb13f>] filter_suites_test_glob_test+0x12f/0x560
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff888105117878 (size 96):
comm "kunit_try_catch", pid 1398, jiffies 4294720454 (age 781.945s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff a0 ac 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829e9651>] kunit_filter_suites+0x481/0xcc0
[<ffffffff829eb13f>] filter_suites_test_glob_test+0x12f/0x560
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff888102c31c00 (size 1024):
comm "kunit_try_catch", pid 1404, jiffies 4294720460 (age 781.948s)

hex dump (first 32 bytes):

6e 6f 72 6d 61 6c 5f 73 75 69 74 65 00 00 00 00 normal_suite....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829ecf17>] kunit_filter_attr_tests+0xf7/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829ea975>] filter_attr_test+0x195/0x5f0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff8881052cd250 (size 192):
comm "kunit_try_catch", pid 1404, jiffies 4294720460 (age 781.948s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff 00 a9 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829ecfc1>] kunit_filter_attr_tests+0x1a1/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829ea975>] filter_attr_test+0x195/0x5f0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff888104f4e400 (size 1024):
comm "kunit_try_catch", pid 1408, jiffies 4294720464 (age 781.944s)

hex dump (first 32 bytes):

73 75 69 74 65 00 00 00 00 00 00 00 00 00 00 00 suite...........

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<ffffffff817db753>] __kmalloc_node_track_caller+0x53/0x150
[<ffffffff817bd242>] kmemdup+0x22/0x50
[<ffffffff829ecf17>] kunit_filter_attr_tests+0xf7/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829e9fc3>] filter_attr_skip_test+0x133/0x6e0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20
unreferenced object 0xffff8881052cc620 (size 192):
comm "kunit_try_catch", pid 1408, jiffies 4294720464 (age 781.944s)

hex dump (first 32 bytes):

a0 85 9e 82 ff ff ff ff c0 a8 7c 84 ff ff ff ff ..........|.....

00 00 00 00 00 00 00 00 02 00 00 00 02 00 00 00 ................
backtrace:

[<ffffffff817dbad2>] __kmalloc+0x52/0x150
[<ffffffff829ecfc1>] kunit_filter_attr_tests+0x1a1/0x860
[<ffffffff829e99ff>] kunit_filter_suites+0x82f/0xcc0
[<ffffffff829e9fc3>] filter_attr_skip_test+0x133/0x6e0
[<ffffffff829e802a>] kunit_generic_run_threadfn_adapter+0x4a/0x90
[<ffffffff81236fc6>] kthread+0x2b6/0x380
[<ffffffff81096afd>] ret_from_fork+0x2d/0x70
[<ffffffff81003511>] ret_from_fork_asm+0x11/0x20

Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites")

Fixes: 76066f93f1df ("kunit: add tests for filtering attributes")
Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Suggested-by: Rae Moar <rm...@google.com>
Reviewed-by: David Gow <davi...@google.com>
Suggested-by: David Gow <davi...@google.com>

Reported-by: kernel test robot <l...@intel.com>

Closes: https://lore.kernel.org/oe-kbuild-all/202309142251...@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202309270433...@intel.com/
---
v4:
- Make free_suite_set() take a void *.
- Update the fix tag.

- Add Suggested-by and Reviewed-by.

v3:

- Update the kfree_at_end() to use kunit_free_suite_set() instead calling it
directly.
- Update the commit message.
- Add Suggested-by.
v2:
- Add the memory leak backtrace.
- Remove the unused func kfree_at_end() kernel test robot noticed.
- Update the commit message.
---

lib/kunit/executor_test.c | 36 ++++++++++++++++++++++--------------
1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index b4f6f96b2844..22d4ee86dbed 100644

@@ -256,18 +256,26 @@ kunit_test_suites(&executor_test_suite);

/* Test helpers */

-/* Use the resource API to register a call to kfree(to_free).
+static void free_suite_set(void *suite_set)
+{
+ kunit_free_suite_set(*(struct kunit_suite_set *)suite_set);
+ kfree(suite_set);
+}
+
+/* Use the resource API to register a call to free_suite_set.

* Since we never actually use the resource, it's safe to use on const data.

*/
-static void kfree_at_end(struct kunit *test, const void *to_free)
+static void free_suite_set_at_end(struct kunit *test, const void *to_free)
{
- /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */
- if (IS_ERR_OR_NULL(to_free))

+ struct kunit_suite_set *free;
+
+ if (!((struct kunit_suite_set *)to_free)->start)
return;

- kunit_add_action(test,

- (kunit_action_t *)kfree,
- (void *)to_free);

+ free = kzalloc(sizeof(struct kunit_suite_set), GFP_KERNEL);
+ *free = *(struct kunit_suite_set *)to_free;
+
+ kunit_add_action(test, free_suite_set, (void *)free);

[Jinjie Ruan]

modprobe cpumask_kunit and rmmod cpumask_kunit, kmemleak detect
a suspected memory leak as below.

If kunit_filter_suites() in kunit_module_init() succeeds, the
suite_set.start will not be NULL and the kunit_free_suite_set() in
kunit_module_exit() should free all the memory which has not
been freed. However the test_cases in suites is left out.

unreferenced object 0xffff54ac47e83200 (size 512):
comm "modprobe", pid 592, jiffies 4294913238 (age 1367.612s)

hex dump (first 32 bytes):

84 13 1a f0 d3 b6 ff ff 30 68 1a f0 d3 b6 ff ff ........0h......

00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:

[<000000008dec63a2>] slab_post_alloc_hook+0xb8/0x368
[<00000000ec280d8e>] __kmem_cache_alloc_node+0x174/0x290
[<00000000896c7740>] __kmalloc+0x60/0x2c0
[<000000007a50fa06>] kunit_filter_suites+0x254/0x5b8
[<0000000078cc98e2>] kunit_module_notify+0xf4/0x240
[<0000000033cea952>] notifier_call_chain+0x98/0x17c
[<00000000973d05cc>] notifier_call_chain_robust+0x4c/0xa4
[<000000005f95895f>] blocking_notifier_call_chain_robust+0x4c/0x74
[<0000000048e36fa7>] load_module+0x1a2c/0x1c40
[<0000000004eb8a91>] init_module_from_file+0x94/0xcc
[<0000000037dbba28>] idempotent_init_module+0x184/0x278
[<00000000161b75cb>] __arm64_sys_finit_module+0x68/0xa8
[<000000006dc1669b>] invoke_syscall+0x44/0x100
[<00000000fa87e304>] el0_svc_common.constprop.1+0x68/0xe0
[<000000009d8ad866>] do_el0_svc+0x1c/0x28
[<000000005b83c607>] el0_svc+0x3c/0xc4

Fixes: a127b154a8f2 ("kunit: tool: allow filtering test cases via glob")
Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Reviewed-by: Rae Moar <rm...@google.com>
Reviewed-by: David Gow <davi...@google.com>
---
v4:

- Correct the fix tag.

- Add Reviewed-by.
v2:
- Add Reviewed-by.
---

lib/kunit/executor.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index a6348489d45f..a037a46fae5e 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c

@@ -137,8 +137,10 @@ void kunit_free_suite_set(struct kunit_suite_set suite_set)
{

struct kunit_suite * const *suites;

[Jinjie Ruan]

If the outer layer for loop is iterated more than once and it fails not

in the first iteration, the copy pointer has been moved. So it should free
the original copy's backup copy_start.

Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")

Signed-off-by: Jinjie Ruan <ruanj...@huawei.com>
Reviewed-by: Rae Moar <rm...@google.com>
Reviewed-by: David Gow <davi...@google.com>
---
v4:

- Add Reviewed-by.
v2:
- Add Reviewed-by.
---

lib/kunit/executor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index a037a46fae5e..9358ed2df839 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c

[David Gow]

Thanks. This is all looking good to me.

The whole series is (still):