| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJ-ks9=SEBCZiuq2YE3Uj5wJ4Pv+78W-VBTeV7CSzLYJZqsR8Q@mail.gmail.com>
Date: Mon, 10 Feb 2025 09:36:41 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Arpitha Raghunandan <98.arpi@...il.com>, David Gow <davidgow@...gle.com>,
Petr Mladek <pmladek@...e.com>, Steven Rostedt <rostedt@...dmis.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Sergey Senozhatsky <senozhatsky@...omium.org>, Andrew Morton <akpm@...ux-foundation.org>,
Shuah Khan <shuah@...nel.org>, Brendan Higgins <brendan.higgins@...ux.dev>,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [PATCH v2 1/2] printf: convert self-test to KUnit
On Mon, Feb 10, 2025 at 8:01 AM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
>
> On Fri, Feb 07 2025, Tamir Duberstein <tamird@...il.com> wrote:
>
> [...]
>
> If/when you do re-roll a v3, can you split the defconfig changes off to
> a separate patch? It's a little annoying to scroll through all those
> mechanical one-liner diffs to get to the actual changes.
Yep. I'll split them into one for m68k and another for powerpc. Geert,
I'll move your Acked-by to the m68k patch.
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 1af972a92d06..4834cac1eb8f 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2427,6 +2427,23 @@ config ASYNC_RAID6_TEST
> > config TEST_HEXDUMP
> > tristate "Test functions located in the hexdump module at runtime"
> >
> > +config PRINTF_KUNIT_TEST
> > + tristate "KUnit test printf() family of functions at runtime" if !KUNIT_ALL_TESTS
> > + depends on KUNIT
> > + default KUNIT_ALL_TESTS
> > + help
> > + Enable this option to test the printf functions at boot.
> > +
>
> Well, not necessarily at boot unless built-in?
>
> > + KUnit tests run during boot and output the results to the debug log
> > + in TAP format (http://testanything.org/). Only useful for kernel devs
> > + running the KUnit test harness, and not intended for inclusion into a
> > + production build.
> > +
> > + For more information on KUnit and unit tests in general please refer
> > + to the KUnit documentation in Documentation/dev-tools/kunit/.
> > +
> > + If unsure, say N.
> > +
>
> I see that this is copy-pasted from other kunit config items, but not
> all of them have all this boilerplate, and I don't think it's useful
> (and, the mentions of "at boot" and "during boot" are actively
> misleading). Other kunit config items are shorter and more to the point,
> e.g.
>
> Enable this to turn on 'list_sort()' function test. This test is
> executed only once during system boot (so affects only boot time),
> or at module load time.
>
> If unsure, say N.
>
Very good point. Truthfully this boilerplate is a result of
checkpatch.pl nagging about this paragraph being a certain length.
I'll drop it and just write "Enable this option to test the printf
functions at runtime.".
> >
> > obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
> > diff --git a/lib/test_printf.c b/lib/printf_kunit.c
> > similarity index 89%
> > rename from lib/test_printf.c
> > rename to lib/printf_kunit.c
> > index 59dbe4f9a4cb..fe6f4df92dd5 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/printf_kunit.c
> > @@ -5,7 +5,7 @@
> >
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > -#include <linux/init.h>
> > +#include <kunit/test.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/printk.h>
> > @@ -25,8 +25,6 @@
> >
> > #include <linux/property.h>
> >
> > -#include "../tools/testing/selftests/kselftest_module.h"
> > -
> > #define BUF_SIZE 256
> > #define PAD_SIZE 16
> > #define FILL_CHAR '$'
> > @@ -37,72 +35,71 @@
> > block \
> > __diag_pop();
> >
> > -KSTM_MODULE_GLOBALS();
> > +static char *test_buffer;
> > +static char *alloced_buffer;
> > +
> > +static struct kunit *kunittest;
> >
> > -static char *test_buffer __initdata;
> > -static char *alloced_buffer __initdata;
> > +#define tc_fail(fmt, ...) \
> > + KUNIT_FAIL(kunittest, fmt, ##__VA_ARGS__)
> >
> > -static int __printf(4, 0) __init
> > +static void __printf(4, 0)
> > do_test(int bufsize, const char *expect, int elen,
> > const char *fmt, va_list ap)
> > {
> > va_list aq;
> > int ret, written;
> >
> > - total_tests++;
> > -
> > memset(alloced_buffer, FILL_CHAR, BUF_SIZE + 2*PAD_SIZE);
> > va_copy(aq, ap);
> > ret = vsnprintf(test_buffer, bufsize, fmt, aq);
> > va_end(aq);
> >
> > if (ret != elen) {
> > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) returned %d, expected %d\n",
> > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) returned %d, expected %d\n",
> > bufsize, fmt, ret, elen);
> > - return 1;
> > + return;
> > }
> >
> > if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) {
> > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote before buffer\n", bufsize, fmt);
> > - return 1;
> > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) wrote before buffer\n", bufsize, fmt);
> > + return;
> > }
> >
> > if (!bufsize) {
> > if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE + PAD_SIZE)) {
> > - pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to buffer\n",
> > + tc_fail("vsnprintf(buf, 0, \"%s\", ...) wrote to buffer\n",
> > fmt);
> > - return 1;
> > }
> > - return 0;
> > + return;
> > }
> >
> > written = min(bufsize-1, elen);
> > if (test_buffer[written]) {
> > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not nul-terminate buffer\n",
> > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) did not nul-terminate buffer\n",
> > bufsize, fmt);
> > - return 1;
> > + return;
> > }
> >
> > if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) {
> > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
> > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
> > bufsize, fmt);
> > - return 1;
> > + return;
> > }
> >
> > if (memchr_inv(test_buffer + bufsize, FILL_CHAR, BUF_SIZE + PAD_SIZE - bufsize)) {
> > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond buffer\n", bufsize, fmt);
> > - return 1;
> > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) wrote beyond buffer\n", bufsize, fmt);
> > + return;
> > }
> >
> > if (memcmp(test_buffer, expect, written)) {
> > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
> > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
> > bufsize, fmt, test_buffer, written, expect);
> > - return 1;
> > + return;
> > }
> > - return 0;
> > }
> >
> > -static void __printf(3, 4) __init
> > +static void __printf(3, 4)
> > __test(const char *expect, int elen, const char *fmt, ...)
> > {
> > va_list ap;
> > @@ -110,9 +107,8 @@ __test(const char *expect, int elen, const char *fmt, ...)
> > char *p;
> >
> > if (elen >= BUF_SIZE) {
> > - pr_err("error in test suite: expected output length %d too long. Format was '%s'.\n",
> > - elen, fmt);
> > - failed_tests++;
> > + tc_fail("error in test suite: expected output length %d too long. Format was '%s'.\n",
> > + elen, fmt);
> > return;
> > }
> >
> > @@ -124,19 +120,17 @@ __test(const char *expect, int elen, const char *fmt, ...)
> > * enough and 0), and then we also test that kvasprintf would
> > * be able to print it as expected.
> > */
> > - failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap);
> > + do_test(BUF_SIZE, expect, elen, fmt, ap);
> > rand = get_random_u32_inclusive(1, elen + 1);
> > /* Since elen < BUF_SIZE, we have 1 <= rand <= BUF_SIZE. */
> > - failed_tests += do_test(rand, expect, elen, fmt, ap);
> > - failed_tests += do_test(0, expect, elen, fmt, ap);
> > + do_test(rand, expect, elen, fmt, ap);
> > + do_test(0, expect, elen, fmt, ap);
> >
> > p = kvasprintf(GFP_KERNEL, fmt, ap);
> > if (p) {
> > - total_tests++;
> > if (memcmp(p, expect, elen+1)) {
> > - pr_warn("kvasprintf(..., \"%s\", ...) returned '%s', expected '%s'\n",
> > + tc_fail("kvasprintf(..., \"%s\", ...) returned '%s', expected '%s'\n",
> > fmt, p, expect);
> > - failed_tests++;
> > }
> > kfree(p);
> > }
>
> So reading through this, is there really any reason to drop those
> existing total_tests and failed_tests tallies? Since you do need to keep
> the control flow the same, leaving the return types and "return
> 1"/"return 0" and their uses in updating failed_tests would also reduce
> the patch size quite significantly.
>
> So they no longer come from some KSTM_MODULE_GLOBALS(), and thus need to
> be explicitly declared in this module, but that's exactly how it was
> originally until someone decided to lift that from the the printf suite
> to kstm.
>
> Yes, they'd need to be explicitly initialized to 0 during kunit_init
> since they are no longer use-once __initdata variable, and some
> kunit_exit logic would need to "manually" report them.
If I understand your concern correctly, you only really care about
`total_tests`, right? I think it's slightly unidiomatic to count tests
outside the framework this way but it's not a hill I'll die on.
Just to elaborate on why I think this unidiomatic: the KUnit test
runner script that produces human-readable output doesn't emit logs
unless something fails. In the success case, you'd get (before the
next patch that breaks this into many tests):
[09:34:15] ==================== printf (1 subtest)
====================
[09:34:15] [PASSED] printf_test
[09:34:15] ===================== [PASSED] printf
======================
[09:34:15] ============================================================
[09:34:15] Testing complete. Ran 1 tests: passed: 1
but the raw output does contain the tally:
KTAP version 1
1..1
KTAP version 1
# Subtest: printf
# module: printf_kunit
1..1
ok 1 printf_test
# printf: ran 448 tests
ok 1 printf
So assuming you're OK with this compromise, I'll go ahead and spin v3.
Thanks for the review!
Powered by blists - more mailing lists