| Message ID | 20220406232240.1825048-1-dlatypov@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kunit: bail out of test filtering logic quicker if OOM | expand |
On Wed, Apr 6, 2022 at 7:22 PM Daniel Latypov <dlatypov@google.com> wrote: > > 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.com.cn/ > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > Reported-by: Zeal Robot <zealci@zte.com.cn> > Signed-off-by: Lv Ruyi <lv.ruyi@zte.com.cn> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
On Wed, Apr 6, 2022 at 4:22 PM Daniel Latypov <dlatypov@google.com> wrote: > > 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.com.cn/ > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > Reported-by: Zeal Robot <zealci@zte.com.cn> > Signed-off-by: Lv Ruyi <lv.ruyi@zte.com.cn> Oh, I just realized the typo here. This was meant to be a second "Reported-by:" Let me sound out a v2 to fix that.
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);