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

[PATCH v3 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.

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

[PATCH v3 1/4] kunit: Fix missed memory release in kunit_free_suite_set()

[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 <ruanjinjie@huawei.com>
Reviewed-by: Rae Moar <rmoar@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;
 
-	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

Re: [PATCH v3 1/4] kunit: Fix missed memory release in kunit_free_suite_set()

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 3214 bytes --]

On Fri, 22 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> 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 <ruanjinjie@huawei.com>
> Reviewed-by: Rae Moar <rmoar@google.com>
> ---
> v2:
> - Add Reviewed-by.
> ---

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

This looks good, thanks.

Reviewed-by: David Gow <davidgow@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+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230922071020.2554677-2-ruanjinjie%40huawei.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH v3 2/4] kunit: Fix the wrong kfree of copy for kunit_filter_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 <ruanjinjie@huawei.com>
Reviewed-by: Rae Moar <rmoar@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
@@ -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

Re: [PATCH v3 2/4] kunit: Fix the wrong kfree of copy for kunit_filter_suites()

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Fri, 22 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development
<kunit-dev@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 <ruanjinjie@huawei.com>
> Reviewed-by: Rae Moar <rmoar@google.com>
> ---
> v2:
> - Add Reviewed-by.
> ---

Like v2, this looks good.

Reviewed-by: David Gow <davidgow@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+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230922071020.2554677-3-ruanjinjie%40huawei.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH v3 3/4] kunit: Fix possible memory leak in kunit_filter_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 <ruanjinjie@huawei.com>
Reviewed-by: Rae Moar <rmoar@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
@@ -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

Re: [PATCH v3 3/4] kunit: Fix possible memory leak in kunit_filter_suites()

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 4105 bytes --]

On Fri, 22 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development
<kunit-dev@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 <ruanjinjie@huawei.com>
> Reviewed-by: Rae Moar <rmoar@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 <davidgow@google.com>

Cheers,
-- David


>  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
>
> --
> 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+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230922071020.2554677-4-ruanjinjie%40huawei.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH v3 4/4] kunit: test: Fix the possible memory leak in executor_test

[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 <ruanjinjie@huawei.com>
Suggested-by: Rae Moar <rmoar@google.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202309142251.uJ8saAZv-lkp@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

Re: [PATCH v3 4/4] kunit: test: Fix the possible memory leak in executor_test

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 13633 bytes --]

On Fri, 22 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> 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 <ruanjinjie@huawei.com>
> Suggested-by: Rae Moar <rmoar@google.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202309142251.uJ8saAZv-lkp@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.
> ---

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.3609689-1-davidgow@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 <davidgow@google.com>

Cheers,
-- David

>  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)

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+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230922071020.2554677-5-ruanjinjie%40huawei.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v3 4/4] kunit: test: Fix the possible memory leak in executor_test

[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.wGmFRGjd-lkp@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.wGmFRGjd-lkp@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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309270433.wGmFRGjd-lkp@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