On Tue, Mar 29, 2022 at 3: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?"
> Or maybe just adding some pr_err() calls is sufficient.
I think we should either return an err ptr, or log something (maybe both).
But yeah, I agree with you Daniel, I don't like overloading NULL.
> > memcpy(copy, suite, sizeof(*copy));
> >
> > filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL);
> > + if (!filtered)
> > + return NULL;
> >
> > n = 0;
> > kunit_suite_for_each_test_case(suite, test_case) {
> > --
> > 2.25.1
> >
