Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220329103919.2376818-1-lv.ruyi@zte.com.cn> <CAGS_qxpCHgp7ToQV9UALPy-4nyHDcdpWOCCd3duz-L6EgYPpOg@mail.gmail.com>
In-Reply-To: <CAGS_qxpCHgp7ToQV9UALPy-4nyHDcdpWOCCd3duz-L6EgYPpOg@mail.gmail.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Tue, 29 Mar 2022 17:54:09 -0500
Message-ID: <CAGS_qxr1VZz+Tzxpwzcx_HtPhWxSONPYEshPNyf=diUw2X6VDg@mail.gmail.com>
Subject: Re: [PATCH] kunit: tool: add null pointer check
To:     cgel.zte@gmail.com
Cc:     brendanhiggins@google.com, linux-kselftest@vger.kernel.org,
        kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org,
        Lv Ruyi <lv.ruyi@zte.com.cn>, Zeal Robot <zealci@zte.com.cn>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Mar 29, 2022 at 2:29 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Tue, Mar 29, 2022 at 5:39 AM <cgel.zte@gmail.com> wrote:
> >
> > From: Lv Ruyi <lv.ruyi@zte.com.cn>
> >
> > 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@zte.com.cn>
> > Signed-off-by: Lv Ruyi <lv.ruyi@zte.com.cn>
> > ---
> >  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.