Received-SPF: pass (google.com: domain of linux-kernel+bounces-88820-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 1 Mar 2024 09:25:07 -0800
From: Kees Cook <keescook@chromium.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Andy Shevchenko <andy@kernel.org>, linux-hardening@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] string: Convert selftest to KUnit
Message-ID: <202403010921.65C845411@keescook>
References: <20240301002416.it.092-kees@kernel.org>
 <20240301002650.1544829-1-keescook@chromium.org>
 <CAHp75VdhY6-OmNYtbpnb+dv1Du=LTnignsTRSU9+8yqpbFqPsQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHp75VdhY6-OmNYtbpnb+dv1Du=LTnignsTRSU9+8yqpbFqPsQ@mail.gmail.com>

On Fri, Mar 01, 2024 at 01:09:27PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > Convert test_string.c to KUnit so it can be easily run with everything
> > else.
> 
> Have you run it?

Yes:

$ ./tools/testing/kunit/kunit.py run string

[09:21:32] Starting KUnit Kernel (1/1)...
[09:21:32] ============================================================
[09:21:32] =================== string (6 subtests) ====================
[09:21:32] [PASSED] test_memset16
[09:21:32] [PASSED] test_memset32
[09:21:32] [PASSED] test_memset64
[09:21:32] [PASSED] test_strchr
[09:21:32] [PASSED] test_strnchr
[09:21:32] [PASSED] test_strspn
[09:21:32] ===================== [PASSED] string ======================
[09:21:32] ============================================================
[09:21:32] Testing complete. Ran 6 tests: passed: 6
[09:21:32] Elapsed time: 11.545s total, 0.001s configuring, 11.327s building, 0.183s running


> ...
> 
> >         if (i < 256)
> > -               return (i << 24) | (j << 16) | k | 0x8000;
> > -       return 0;
> > +               KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
> 
> First of all, this special value encodes the problematic patterns, so
> you missed proper messaging.

Yeah, I see now this isn't a test but rather an encoded report. Since
the failures are caught earlier, I can improve those messages instead of
doing an encoded version.

> Second, the returned value has a constant, how do you expect 0 to be
> equal to something (guaranteed not to be 0)?
> 
> This needs a good rethink of what you should do in the KUnit approach.
> 
> ...
> 
> > +               KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
> 
> Ditto.
> 
> ...
> 
> > +               KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
> 
> Ditto.
> 
> ...
> 
> >         for (i = 0; i < strlen(test_string) + 1; i++) {
> >                 result = strchr(test_string, test_string[i]);
> > -               if (result - test_string != i)
> > -                       return i + 'a';
> > +               KUNIT_ASSERT_EQ(test, result - test_string, i);
> 
> In a similar way, all returned values are *special*, you really need
> to think about them before converting to a simple (and sometimes
> wrong) checks)

This encoding is trying to report "i", so I've adjusted the error
reporting in v3.

> I dunno if KUnit has a fault ejection simulation. It should, in order
> to be sure that test cases are fine when they fail.

Yeah, bumping offsets and such produce expected failures.

-- 
Kees Cook