Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 27 Feb 2022 09:33:46 -0800
From:   Kees Cook <keescook@chromium.org>
To:     David Gow <davidgow@google.com>
Cc:     Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Vitor Massaru Iha <vitor@massaru.org>,
        Daniel Latypov <dlatypov@google.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2] lib: overflow: Convert to Kunit
Message-ID: <202202270929.5FD6A89B@keescook>
References: <20220224054825.1853314-1-keescook@chromium.org>
 <CABVgOS=TWVh649_Vjo3wnMu9gZnq66gkV-LtGgsksAWMqc+MSA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CABVgOS=TWVh649_Vjo3wnMu9gZnq66gkV-LtGgsksAWMqc+MSA@mail.gmail.com>
Precedence: bulk

On Fri, Feb 25, 2022 at 03:57:17PM +0800, David Gow wrote:
> On Thu, Feb 24, 2022 at 1:48 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > Convert overflow unit tests to KUnit, for better integration into the
> > kernel self test framework. Includes a rename of test_overflow.c to
> > overflow_kunit.c, and CONFIG_TEST_OVERFLOW to CONFIG_OVERFLOW_KUNIT_TEST.
> >
> > $ ./tools/testing/kunit/kunit.py config
> > ...
> (This 'config' step should be unnecessary... See below)

Ah yeah. I've removed that now.

> > $ ./tools/testing/kunit/kunit.py run overflow
> > ...
> > [14:33:51] Starting KUnit Kernel (1/1)...
> > [14:33:51] ============================================================
> > [14:33:51] ================== overflow (11 subtests) ==================
> > [14:33:51] [PASSED] u8_overflow_test
> > [14:33:51] [PASSED] s8_overflow_test
> > [14:33:51] [PASSED] u16_overflow_test
> > [14:33:51] [PASSED] s16_overflow_test
> > [14:33:51] [PASSED] u32_overflow_test
> > [14:33:51] [PASSED] s32_overflow_test
> > [14:33:51] [PASSED] u64_overflow_test
> > [14:33:51] [PASSED] s64_overflow_test
> > [14:33:51] [PASSED] overflow_shift_test
> > [14:33:51] [PASSED] overflow_allocation_test
> > [14:33:51] [PASSED] overflow_size_helpers_test
> > [14:33:51] ==================== [PASSED] overflow =====================
> > [14:33:51] ============================================================
> > [14:33:51] Testing complete. Passed: 11, Failed: 0, Crashed: 0, Skipped: 0, Errors: 0
> > [14:33:51] Elapsed time: 12.525s total, 0.001s configuring, 12.402s building, 0.101s running
> >
> > Cc: David Gow <davidgow@google.com>
> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Co-developed-by: Vitor Massaru Iha <vitor@massaru.org>
> > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org>
> > Link: https://lore.kernel.org/lkml/20200720224418.200495-1-vitor@massaru.org/
> > Co-developed-by: Daniel Latypov <dlatypov@google.com>
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > Link: https://lore.kernel.org/linux-kselftest/20210503211536.1384578-1-dlatypov@google.com/
> > Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> > Link: https://lore.kernel.org/lkml/CAKwvOdm62iA1dNiC6Q11UJ-MnTqtc4kXkm-ubPaFMK824_k0nw@mail.gmail.com
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > v1: https://lore.kernel.org/lkml/20220216224153.2242451-1-keescook@chromium.org/
> > v2: Fixed up the authorship more, pulled in other prior changes (Daniel)
> 
> This looks good to me. Some of the test cases _could_ probably be done
> as parameterised tests (given they're already basically table-driven),
> but I admit that could end up being excessively verbose.

In the end there was so much churn already I decided to leave it as-is.

> Apart from my not being able to find a tree this applies cleanly on,
> the tests work well here.

Ah, sorry about that! (More below...)

> Reviewed-by: David Gow <davidgow@google.com>

Thanks!

> [...]
> > @@ -600,127 +554,118 @@ struct __test_flex_array {
> >         unsigned long data[];
> ">  };
> >
> > -static int __init test_overflow_size_helpers(void)
> > +static void overflow_size_helpers_test(struct kunit *test)
> >  {
> >         /* Make sure struct_size() can be used in a constant expression. */
> >         u8 ce_array[struct_size((struct __test_flex_array *)0, data, 55)];
> >         struct __test_flex_array *obj;
> >         int count = 0;
> > -       int err = 0;
> >         int var;
> >         volatile int unconst = 0;
> 
> Where does this variable declaration (and its use below) come from?
> It's not in the version of the "overflow: Implement size_t saturating
> arithmetic helpers" patch in linux-next:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=6312fc63aee9d2fef2f9c4d42e57e5b46828f0dc
> 
> It all works if I add it in manually, but it does make applying the
> change as-is something of a pain.

Ah, thanks for noticing that! It looks like I failed to send an update
for that patch. I'll resend this portion of my tree.

-Kees

-- 
Kees Cook