From: Daniel Latypov dlatypov@google.com
[ 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/20220329103919.2376818-1-lv.ruyi@zte...
Signed-off-by: Daniel Latypov dlatypov@google.com Reported-by: Zeal Robot zealci@zte.com.cn Reported-by: Lv Ruyi lv.ruyi@zte.com.cn Reviewed-by: Brendan Higgins brendanhiggins@google.com Signed-off-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Sasha Levin sashal@kernel.org --- 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);
On Mon, May 30, 2022 at 6:36 AM Sasha Levin sashal@kernel.org wrote:
From: Daniel Latypov dlatypov@google.com
[ 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 dlatypov@google.com 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 dlatypov@google.com Reviewed-by: Brendan Higgins brendanhiggins@google.com Signed-off-by: Shuah Khan skhan@linuxfoundation.org
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 sashal@kernel.org wrote:
From: Daniel Latypov dlatypov@google.com
[ 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.
On Sun, Jun 5, 2022 at 6:26 AM Sasha Levin sashal@kernel.org 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 sashal@kernel.org wrote:
From: Daniel Latypov dlatypov@google.com
[ 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
linux-kselftest-mirror@lists.linaro.org