From: Daniel Latypov <[email protected]>
[ Upstream commit a02353f491622e49c7ddedc6a6dc4f1d6ed2150a ]
When filtering what tests to run (suites and/or cases) via
kunit.filter_glob (e.g. kunit.py run <glob>), we allocate copies of
suites.
These allocations can fail, and we largely don't handle that.
Note: realistically, this probably doesn't matter much.
We're not allocating much memory and this happens early in boot, so if
we can't do that, then there's likely far bigger problems.
This patch makes us immediately bail out from the top-level function
(kunit_filter_suites) with -ENOMEM if any of the underlying kmalloc()
calls return NULL.
Implementation note: we used to return NULL pointers from some functions
to indicate either that all suites/tests were filtered out or there was
an error allocating the new array.
We'll log a short error in this case and not run any tests or print a
TAP header. From a kunit.py user's perspective, they'll get a message
about missing/invalid TAP output and have to dig into the test.log to
see it. Since hitting this error seems so unlikely, it's probably fine
to not invent a way to plumb this error message more visibly.
See also: https://lore.kernel.org/linux-kselftest/[email protected]/
Signed-off-by: Daniel Latypov <[email protected]>
Reported-by: Zeal Robot <[email protected]>
Reported-by: Lv Ruyi <[email protected]>
Reviewed-by: Brendan Higgins <[email protected]>
Signed-off-by: Shuah Khan <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
lib/kunit/executor.c | 27 ++++++++++++++++++++++-----
lib/kunit/executor_test.c | 4 +++-
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 22640c9ee819..2f73a6a35a7e 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -71,9 +71,13 @@ kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob)
/* Use memcpy to workaround copy->name being const. */
copy = kmalloc(sizeof(*copy), GFP_KERNEL);
+ if (!copy)
+ return ERR_PTR(-ENOMEM);
memcpy(copy, suite, sizeof(*copy));
filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL);
+ if (!filtered)
+ return ERR_PTR(-ENOMEM);
n = 0;
kunit_suite_for_each_test_case(suite, test_case) {
@@ -106,14 +110,16 @@ kunit_filter_subsuite(struct kunit_suite * const * const subsuite,
filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL);
if (!filtered)
- return NULL;
+ return ERR_PTR(-ENOMEM);
n = 0;
for (i = 0; subsuite[i] != NULL; ++i) {
if (!glob_match(filter->suite_glob, subsuite[i]->name))
continue;
filtered_suite = kunit_filter_tests(subsuite[i], filter->test_glob);
- if (filtered_suite)
+ if (IS_ERR(filtered_suite))
+ return ERR_CAST(filtered_suite);
+ else if (filtered_suite)
filtered[n++] = filtered_suite;
}
filtered[n] = NULL;
@@ -146,7 +152,8 @@ static void kunit_free_suite_set(struct suite_set suite_set)
}
static struct suite_set kunit_filter_suites(const struct suite_set *suite_set,
- const char *filter_glob)
+ const char *filter_glob,
+ int *err)
{
int i;
struct kunit_suite * const **copy, * const *filtered_subsuite;
@@ -166,6 +173,10 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set,
for (i = 0; i < max; ++i) {
filtered_subsuite = kunit_filter_subsuite(suite_set->start[i], &filter);
+ if (IS_ERR(filtered_subsuite)) {
+ *err = PTR_ERR(filtered_subsuite);
+ return filtered;
+ }
if (filtered_subsuite)
*copy++ = filtered_subsuite;
}
@@ -236,9 +247,15 @@ int kunit_run_all_tests(void)
.start = __kunit_suites_start,
.end = __kunit_suites_end,
};
+ int err;
- if (filter_glob_param)
- suite_set = kunit_filter_suites(&suite_set, filter_glob_param);
+ if (filter_glob_param) {
+ suite_set = kunit_filter_suites(&suite_set, filter_glob_param, &err);
+ if (err) {
+ pr_err("kunit executor: error filtering suites: %d\n", err);
+ return err;
+ }
+ }
if (!action_param)
kunit_exec_run_tests(&suite_set);
diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index 4ed57fd94e42..eac6ff480273 100644
--- a/lib/kunit/executor_test.c
+++ b/lib/kunit/executor_test.c
@@ -137,14 +137,16 @@ static void filter_suites_test(struct kunit *test)
.end = suites + 2,
};
struct suite_set filtered = {.start = NULL, .end = NULL};
+ int err = 0;
/* Emulate two files, each having one suite */
subsuites[0][0] = alloc_fake_suite(test, "suite0", dummy_test_cases);
subsuites[1][0] = alloc_fake_suite(test, "suite1", dummy_test_cases);
/* Filter out suite1 */
- filtered = kunit_filter_suites(&suite_set, "suite0");
+ filtered = kunit_filter_suites(&suite_set, "suite0", &err);
kfree_subsuites_at_end(test, &filtered); /* let us use ASSERTs without leaking */
+ KUNIT_EXPECT_EQ(test, err, 0);
KUNIT_ASSERT_EQ(test, filtered.end - filtered.start, (ptrdiff_t)1);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered.start);
--
2.35.1
On Mon, May 30, 2022 at 6:36 AM Sasha Levin <[email protected]> wrote:
>
> From: Daniel Latypov <[email protected]>
>
> [ Upstream commit a02353f491622e49c7ddedc6a6dc4f1d6ed2150a ]
>
Is it possible to make sure the fix for this commit gets picked up as well [1]?
I was waiting a bit to see if it was going to get picked up
automatically, but I don't see such an email yet:
https://lore.kernel.org/stable/?q=kunit+OOM
(Perhaps the automation just hasn't gotten around to it yet?)
Alternatively, reverting just this commit would also work, if that's easier.
[1] commit 1b11063d32d7e11366e48be64215ff517ce32217
Author: Daniel Latypov <[email protected]>
Date: Fri May 13 11:37:07 2022 -0700
kunit: fix executor OOM error handling logic on non-UML
The existing logic happens to work fine on UML, but is not correct when
running on other arches.
1. We didn't initialize `int err`, and kunit_filter_suites() doesn't
explicitly set it to 0 on success. So we had false "failures".
Note: it doesn't happen on UML, causing this to get overlooked.
2. If we error out, we do not call kunit_handle_shutdown().
This makes kunit.py timeout when using a non-UML arch, since the QEMU
process doesn't ever exit.
Fixes: a02353f49162 ("kunit: bail out of test filtering logic
quicker if OOM")
Signed-off-by: Daniel Latypov <[email protected]>
Reviewed-by: Brendan Higgins <[email protected]>
Signed-off-by: Shuah Khan <[email protected]>
Without 1b11063d32d7 above, this "fix" breaks more cases than it fixes
due to my sloppiness.
Thanks,
Daniel
On Tue, May 31, 2022 at 09:15:11AM -0700, Daniel Latypov wrote:
>On Mon, May 30, 2022 at 6:36 AM Sasha Levin <[email protected]> wrote:
>>
>> From: Daniel Latypov <[email protected]>
>>
>> [ Upstream commit a02353f491622e49c7ddedc6a6dc4f1d6ed2150a ]
>>
>
>Is it possible to make sure the fix for this commit gets picked up as well [1]?
>I was waiting a bit to see if it was going to get picked up
>automatically, but I don't see such an email yet:
>https://lore.kernel.org/stable/?q=kunit+OOM
>(Perhaps the automation just hasn't gotten around to it yet?)
I'll make sure the fix added along with this commit.
--
Thanks,
Sasha
On Sun, Jun 5, 2022 at 6:26 AM Sasha Levin <[email protected]> wrote:
>
> On Tue, May 31, 2022 at 09:15:11AM -0700, Daniel Latypov wrote:
> >On Mon, May 30, 2022 at 6:36 AM Sasha Levin <[email protected]> wrote:
> >>
> >> From: Daniel Latypov <[email protected]>
> >>
> >> [ Upstream commit a02353f491622e49c7ddedc6a6dc4f1d6ed2150a ]
> >>
> >
> >Is it possible to make sure the fix for this commit gets picked up as well [1]?
> >I was waiting a bit to see if it was going to get picked up
> >automatically, but I don't see such an email yet:
> >https://lore.kernel.org/stable/?q=kunit+OOM
> >(Perhaps the automation just hasn't gotten around to it yet?)
>
> I'll make sure the fix added along with this commit.
I just saw the emails about it being added to the 5.{17,18} stable trees.
Thanks a bunch!
Daniel