| Message ID | 20230921014008.3887257-1-ruanjinjie@huawei.com |
|---|---|
| Headers | show |
| Series | kunit: Fix some bugs in kunit | expand |
On 2023/9/22 3:50, Rae Moar wrote: > On Wed, Sep 20, 2023 at 9:41 PM '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 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 <ruanjinjie@huawei.com> >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: https://lore.kernel.org/oe-kbuild-all/202309142251.uJ8saAZv-lkp@intel.com/ > > Hello! > > Thanks for sending out a new version and responding to my comments. I > do have one issue below. > > Thanks! > -Rae > >> --- >> 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 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. Right! It is better to update the kfree_at_end() func to solve these problems. I'll try to do it sooner. > >> + >> + 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+unsubscribe@googlegroups.com. >> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230921014008.3887257-5-ruanjinjie%40huawei.com. >
On Thu, 21 Sept 2023 at 09:41, '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 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 <ruanjinjie@huawei.com> > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202309142251.uJ8saAZv-lkp@intel.com/ > --- > v2: > - Add the memory leak backtrace. > - Remove the unused func kfree_at_end() kernel test robot noticed. 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 > 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) 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. > + 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 > > -- > 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/20230921014008.3887257-5-ruanjinjie%40huawei.com.