diff mbox series

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

Message ID 20230922071020.2554677-5-ruanjinjie@huawei.com (mailing list archive)
State New
Delegated to: Brendan Higgins
Headers show
Series kunit: Fix some bugs in kunit | expand

Commit Message

Jinjie Ruan Sept. 22, 2023, 7:10 a.m. UTC
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(-)

Comments

David Gow Sept. 22, 2023, 7:47 a.m. UTC | #1
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.
kernel test robot Sept. 26, 2023, 9:14 p.m. UTC | #2
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
diff mbox series

Patch

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,