On Tue, Mar 29, 2022 at 2:29 PM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
>
> On Tue, Mar 29, 2022 at 5:39 AM <cgel.zte@xxxxxxxxx> wrote:
> >
> > From: Lv Ruyi <lv.ruyi@xxxxxxxxxx>
> >
> > kmalloc and kcalloc is a memory allocation function which can return NULL
> > when some internal memory errors happen. Add null pointer check to avoid
> > dereferencing null pointer.
> >
> > Reported-by: Zeal Robot <zealci@xxxxxxxxxx>
> > Signed-off-by: Lv Ruyi <lv.ruyi@xxxxxxxxxx>
> > ---
> > lib/kunit/executor.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 22640c9ee819..be21d0451367 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 NULL;
>
> While this is technically correct to check, in this context it's less clear.
> If we can't allocate this memory, we likely can't run any subsequent
> tests, either because the test cases will want to allocate some memory
> and/or KUnit will need to allocate some for internal bookkeeping.
>
> The existing code (and by extension this patch) "handles" OOM
> situations by silently dropping test suites/cases.
> So I sort of intentionally figured we should let it crash early in
> this case since that's probably more debuggable.
>
> This code does check for NULL returns earlier on in the call chain, i.e.
>
> first in kunit_filter_suites()
> 158 copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL);
> 159 filtered.start = copy;
> 160 if (!copy) { /* won't be able to run anything, return
> an empty set */
> 161 filtered.end = copy;
> 162 return filtered;
> 163 }
>
> and second in kunit_filter_subsuite()
> 107 filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL);
> 108 if (!filtered)
> 109 return NULL;
>
> The first kmalloc_array() is our first allocation in this file.
> If we can't handle that, then things are really going wrong, and I
> assumed there'd be plenty of debug messages in dmesg, so silently
> returning is probably fine.
> The second one also felt similar.
>
> So I think that
> * it's highly unlikely that we pass those checks and fail on these new
> ones (we're not allocating much)
> * if we do fail, this is now harder to debug since it's partially
> running tests, partially not
>
> Should we instead rework the code to more clearly signal allocation
> errors instead of overloading NULL to mean "no matches or error?"
More concretely, I'm thinking something like this:
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 22640c9ee819..a5c29a32a33a 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;
@@ -166,6 +172,8 @@ 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))
+ return filtered; // TODO: how do we signal this?
if (filtered_subsuite)
*copy++ = filtered_subsuite;
}
> Or maybe just adding some pr_err() calls is sufficient.
