diff mbox series

[v2,1/2] string: Convert selftest to KUnit

Message ID 20240301002650.1544829-1-keescook@chromium.org (mailing list archive)
State Superseded
Commit 257e66e5800a8a05200dd266c56c8c12725ea99d
Headers show
Series string: Convert selftests to KUnit | expand

Commit Message

Kees Cook March 1, 2024, 12:26 a.m. UTC
Convert test_string.c to KUnit so it can be easily run with everything
else.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Andy Shevchenko <andy@kernel.org>
Cc: linux-hardening@vger.kernel.org
---
 MAINTAINERS                           |   2 +-
 lib/Kconfig.debug                     |   6 +-
 lib/Makefile                          |   2 +-
 lib/{test_string.c => string_kunit.c} | 166 +++++++++-----------------
 4 files changed, 61 insertions(+), 115 deletions(-)
 rename lib/{test_string.c => string_kunit.c} (54%)

Comments

Andy Shevchenko March 1, 2024, 11:09 a.m. UTC | #1
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?

...

>         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.
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)

...

I dunno if KUnit has a fault ejection simulation. It should, in order
to be sure that test cases are fine when they fail.
Kees Cook March 1, 2024, 5:25 p.m. UTC | #2
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.
Andy Shevchenko March 1, 2024, 5:29 p.m. UTC | #3
On Fri, Mar 01, 2024 at 09:25:07AM -0800, Kees Cook wrote:
> 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:

Of course I new the answer :-)

The problem as I described is the (absence of) fault injection in KUnit itself.
When tests are passed the picture is nice, when they are not, the reports become
a disaster (if this is applied).
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index cd651c4df019..9f1f68cccd6a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8976,9 +8976,9 @@  F:	include/linux/string.h
 F:	include/linux/string_choices.h
 F:	include/linux/string_helpers.h
 F:	lib/string.c
+F:	lib/string_kunit.c
 F:	lib/string_helpers.c
 F:	lib/test-string_helpers.c
-F:	lib/test_string.c
 F:	scripts/coccinelle/api/string_choices.cocci
 
 GENERIC UIO DRIVER FOR PCI DEVICES
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4e2febe3b568..406cdf353488 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2352,8 +2352,10 @@  config ASYNC_RAID6_TEST
 config TEST_HEXDUMP
 	tristate "Test functions located in the hexdump module at runtime"
 
-config STRING_SELFTEST
-	tristate "Test string functions at runtime"
+config STRING_KUNIT_TEST
+	tristate "KUnit test string functions at runtime" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
 
 config TEST_STRING_HELPERS
 	tristate "Test functions located in the string_helpers module at runtime"
diff --git a/lib/Makefile b/lib/Makefile
index eae87c41b22b..946277c37831 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -49,7 +49,7 @@  obj-y += bcd.o sort.o parser.o debug_locks.o random32.o \
 	 percpu-refcount.o rhashtable.o base64.o \
 	 once.o refcount.o rcuref.o usercopy.o errseq.o bucket_locks.o \
 	 generic-radix-tree.o bitmap-str.o
-obj-$(CONFIG_STRING_SELFTEST) += test_string.o
+obj-$(CONFIG_STRING_KUNIT_TEST) += string_kunit.o
 obj-y += string_helpers.o
 obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
 obj-y += hexdump.o
diff --git a/lib/test_string.c b/lib/string_kunit.c
similarity index 54%
rename from lib/test_string.c
rename to lib/string_kunit.c
index c5cb92fb710e..bbb54ac11f7b 100644
--- a/lib/test_string.c
+++ b/lib/string_kunit.c
@@ -1,17 +1,23 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Test cases for string functions.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <kunit/test.h>
 #include <linux/module.h>
 #include <linux/printk.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 
-static __init int memset16_selftest(void)
+static void test_memset16(struct kunit *test)
 {
 	unsigned i, j, k;
 	u16 v, *p;
 
 	p = kmalloc(256 * 2 * 2, GFP_KERNEL);
-	if (!p)
-		return -1;
+	KUNIT_ASSERT_NOT_NULL(test, p);
 
 	for (i = 0; i < 256; i++) {
 		for (j = 0; j < 256; j++) {
@@ -20,34 +26,28 @@  static __init int memset16_selftest(void)
 			for (k = 0; k < 512; k++) {
 				v = p[k];
 				if (k < i) {
-					if (v != 0xa1a1)
-						goto fail;
+					KUNIT_EXPECT_EQ(test, v, 0xa1a1);
 				} else if (k < i + j) {
-					if (v != 0xb1b2)
-						goto fail;
+					KUNIT_EXPECT_EQ(test, v, 0xb1b2);
 				} else {
-					if (v != 0xa1a1)
-						goto fail;
+					KUNIT_EXPECT_EQ(test, v, 0xa1a1);
 				}
 			}
 		}
 	}
 
-fail:
 	kfree(p);
 	if (i < 256)
-		return (i << 24) | (j << 16) | k | 0x8000;
-	return 0;
+		KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
 }
 
-static __init int memset32_selftest(void)
+static void test_memset32(struct kunit *test)
 {
 	unsigned i, j, k;
 	u32 v, *p;
 
 	p = kmalloc(256 * 2 * 4, GFP_KERNEL);
-	if (!p)
-		return -1;
+	KUNIT_ASSERT_NOT_NULL(test, p);
 
 	for (i = 0; i < 256; i++) {
 		for (j = 0; j < 256; j++) {
@@ -56,34 +56,28 @@  static __init int memset32_selftest(void)
 			for (k = 0; k < 512; k++) {
 				v = p[k];
 				if (k < i) {
-					if (v != 0xa1a1a1a1)
-						goto fail;
+					KUNIT_EXPECT_EQ(test, v, 0xa1a1a1a1);
 				} else if (k < i + j) {
-					if (v != 0xb1b2b3b4)
-						goto fail;
+					KUNIT_EXPECT_EQ(test, v, 0xb1b2b3b4);
 				} else {
-					if (v != 0xa1a1a1a1)
-						goto fail;
+					KUNIT_EXPECT_EQ(test, v, 0xa1a1a1a1);
 				}
 			}
 		}
 	}
 
-fail:
 	kfree(p);
 	if (i < 256)
-		return (i << 24) | (j << 16) | k | 0x8000;
-	return 0;
+		KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
 }
 
-static __init int memset64_selftest(void)
+static void test_memset64(struct kunit *test)
 {
 	unsigned i, j, k;
 	u64 v, *p;
 
 	p = kmalloc(256 * 2 * 8, GFP_KERNEL);
-	if (!p)
-		return -1;
+	KUNIT_ASSERT_NOT_NULL(test, p);
 
 	for (i = 0; i < 256; i++) {
 		for (j = 0; j < 256; j++) {
@@ -92,27 +86,22 @@  static __init int memset64_selftest(void)
 			for (k = 0; k < 512; k++) {
 				v = p[k];
 				if (k < i) {
-					if (v != 0xa1a1a1a1a1a1a1a1ULL)
-						goto fail;
+					KUNIT_EXPECT_EQ(test, v, 0xa1a1a1a1a1a1a1a1ULL);
 				} else if (k < i + j) {
-					if (v != 0xb1b2b3b4b5b6b7b8ULL)
-						goto fail;
+					KUNIT_EXPECT_EQ(test, v, 0xb1b2b3b4b5b6b7b8ULL);
 				} else {
-					if (v != 0xa1a1a1a1a1a1a1a1ULL)
-						goto fail;
+					KUNIT_EXPECT_EQ(test, v, 0xa1a1a1a1a1a1a1a1ULL);
 				}
 			}
 		}
 	}
 
-fail:
 	kfree(p);
 	if (i < 256)
-		return (i << 24) | (j << 16) | k | 0x8000;
-	return 0;
+		KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);
 }
 
-static __init int strchr_selftest(void)
+static void test_strchr(struct kunit *test)
 {
 	const char *test_string = "abcdefghijkl";
 	const char *empty_string = "";
@@ -121,26 +110,20 @@  static __init int strchr_selftest(void)
 
 	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);
 	}
 
 	result = strchr(empty_string, '\0');
-	if (result != empty_string)
-		return 0x101;
+	KUNIT_ASSERT_PTR_EQ(test, result, empty_string);
 
 	result = strchr(empty_string, 'a');
-	if (result)
-		return 0x102;
+	KUNIT_ASSERT_NULL(test, result);
 
 	result = strchr(test_string, 'z');
-	if (result)
-		return 0x103;
-
-	return 0;
+	KUNIT_ASSERT_NULL(test, result);
 }
 
-static __init int strnchr_selftest(void)
+static void test_strnchr(struct kunit *test)
 {
 	const char *test_string = "abcdefghijkl";
 	const char *empty_string = "";
@@ -153,33 +136,27 @@  static __init int strnchr_selftest(void)
 			if (j <= i) {
 				if (!result)
 					continue;
-				return ((i + 'a') << 8) | j;
+				KUNIT_ASSERT_EQ(test, 0, 1);
 			}
 			if (result - test_string != i)
-				return ((i + 'a') << 8) | j;
+				KUNIT_ASSERT_EQ(test, 0, 1);
 		}
 	}
 
 	result = strnchr(empty_string, 0, '\0');
-	if (result)
-		return 0x10001;
+	KUNIT_ASSERT_NULL(test, result);
 
 	result = strnchr(empty_string, 1, '\0');
-	if (result != empty_string)
-		return 0x10002;
+	KUNIT_ASSERT_PTR_EQ(test, result, empty_string);
 
 	result = strnchr(empty_string, 1, 'a');
-	if (result)
-		return 0x10003;
+	KUNIT_ASSERT_NULL(test, result);
 
 	result = strnchr(NULL, 0, '\0');
-	if (result)
-		return 0x10004;
-
-	return 0;
+	KUNIT_ASSERT_NULL(test, result);
 }
 
-static __init int strspn_selftest(void)
+static void test_strspn(struct kunit *test)
 {
 	static const struct strspn_test {
 		const char str[16];
@@ -187,7 +164,7 @@  static __init int strspn_selftest(void)
 		const char reject[16];
 		unsigned a;
 		unsigned r;
-	} tests[] __initconst = {
+	} tests[] = {
 		{ "foobar", "", "", 0, 6 },
 		{ "abba", "abc", "ABBA", 4, 4 },
 		{ "abba", "a", "b", 1, 1 },
@@ -198,60 +175,27 @@  static __init int strspn_selftest(void)
 
 	for (i = 0; i < ARRAY_SIZE(tests); ++i, ++s) {
 		res = strspn(s->str, s->accept);
-		if (res != s->a)
-			return 0x100 + 2*i;
+		KUNIT_ASSERT_EQ(test, res, s->a);
 		res = strcspn(s->str, s->reject);
-		if (res != s->r)
-			return 0x100 + 2*i + 1;
+		KUNIT_ASSERT_EQ(test, res, s->r);
 	}
-	return 0;
-}
-
-static __exit void string_selftest_remove(void)
-{
 }
 
-static __init int string_selftest_init(void)
-{
-	int test, subtest;
-
-	test = 1;
-	subtest = memset16_selftest();
-	if (subtest)
-		goto fail;
-
-	test = 2;
-	subtest = memset32_selftest();
-	if (subtest)
-		goto fail;
-
-	test = 3;
-	subtest = memset64_selftest();
-	if (subtest)
-		goto fail;
+static struct kunit_case string_test_cases[] = {
+	KUNIT_CASE(test_memset16),
+	KUNIT_CASE(test_memset32),
+	KUNIT_CASE(test_memset64),
+	KUNIT_CASE(test_strchr),
+	KUNIT_CASE(test_strnchr),
+	KUNIT_CASE(test_strspn),
+	{}
+};
 
-	test = 4;
-	subtest = strchr_selftest();
-	if (subtest)
-		goto fail;
+static struct kunit_suite string_test_suite = {
+	.name = "string",
+	.test_cases = string_test_cases,
+};
 
-	test = 5;
-	subtest = strnchr_selftest();
-	if (subtest)
-		goto fail;
-
-	test = 6;
-	subtest = strspn_selftest();
-	if (subtest)
-		goto fail;
-
-	pr_info("String selftests succeeded\n");
-	return 0;
-fail:
-	pr_crit("String selftest failure %d.%08x\n", test, subtest);
-	return 0;
-}
+kunit_test_suites(&string_test_suite);
 
-module_init(string_selftest_init);
-module_exit(string_selftest_remove);
 MODULE_LICENSE("GPL v2");