Hi,
I realized the string selftests hadn't been converted to KUnit yet. Do that.
-Kees
v2: rebase onto https://lore.kernel.org/r/[email protected]
v1: https://lore.kernel.org/linux-hardening/[email protected]/
Kees Cook (2):
string: Convert selftest to KUnit
string: Convert helpers selftest to KUnit
MAINTAINERS | 4 +-
lib/Kconfig.debug | 12 +-
lib/Makefile | 4 +-
...tring_helpers.c => string_helpers_kunit.c} | 177 ++++++++++--------
lib/{test_string.c => string_kunit.c} | 166 ++++++----------
5 files changed, 164 insertions(+), 199 deletions(-)
rename lib/{test-string_helpers.c => string_helpers_kunit.c} (78%)
rename lib/{test_string.c => string_kunit.c} (54%)
--
2.34.1
Convert test-string_helpers.c to KUnit so it can be easily run with
everything else.
Signed-off-by: Kees Cook <[email protected]>
---
Cc: Andy Shevchenko <[email protected]>
Cc: Kent Overstreet <[email protected]>
Cc: [email protected]
---
MAINTAINERS | 2 +-
lib/Kconfig.debug | 6 +-
lib/Makefile | 2 +-
...tring_helpers.c => string_helpers_kunit.c} | 177 ++++++++++--------
4 files changed, 103 insertions(+), 84 deletions(-)
rename lib/{test-string_helpers.c => string_helpers_kunit.c} (78%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 9f1f68cccd6a..f3f26d2d4ffb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8978,7 +8978,7 @@ 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/string_helpers_kunit.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 406cdf353488..5429e6f170f3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2357,8 +2357,10 @@ config STRING_KUNIT_TEST
depends on KUNIT
default KUNIT_ALL_TESTS
-config TEST_STRING_HELPERS
- tristate "Test functions located in the string_helpers module at runtime"
+config STRING_HELPERS_KUNIT_TEST
+ tristate "KUnit test string helpers at runtime" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
config TEST_KSTRTOX
tristate "Test kstrto*() family of functions at runtime"
diff --git a/lib/Makefile b/lib/Makefile
index 946277c37831..97c42e38046f 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -51,7 +51,7 @@ obj-y += bcd.o sort.o parser.o debug_locks.o random32.o \
generic-radix-tree.o bitmap-str.o
obj-$(CONFIG_STRING_KUNIT_TEST) += string_kunit.o
obj-y += string_helpers.o
-obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
+obj-$(CONFIG_STRING_HELPERS_KUNIT_TEST) += string_helpers_kunit.o
obj-y += hexdump.o
obj-$(CONFIG_TEST_HEXDUMP) += test_hexdump.o
obj-y += kstrtox.o
diff --git a/lib/test-string_helpers.c b/lib/string_helpers_kunit.c
similarity index 78%
rename from lib/test-string_helpers.c
rename to lib/string_helpers_kunit.c
index dce67698297b..1fed4fa38f97 100644
--- a/lib/test-string_helpers.c
+++ b/lib/string_helpers_kunit.c
@@ -3,8 +3,9 @@
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/array_size.h>
#include <linux/init.h>
+#include <kunit/test.h>
+#include <linux/array_size.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/module.h>
@@ -12,13 +13,21 @@
#include <linux/string.h>
#include <linux/string_helpers.h>
-static __init bool test_string_check_buf(const char *name, unsigned int flags,
- char *in, size_t p,
- char *out_real, size_t q_real,
- char *out_test, size_t q_test)
+static void test_string_check_buf(struct kunit *test,
+ const char *name, unsigned int flags,
+ char *in, size_t p,
+ char *out_real, size_t q_real,
+ char *out_test, size_t q_test)
{
- if (q_real == q_test && !memcmp(out_test, out_real, q_test))
- return true;
+ int result;
+
+ KUNIT_EXPECT_EQ(test, q_real, q_test);
+
+ result = memcmp(out_test, out_real, q_test);
+ KUNIT_EXPECT_EQ(test, 0, result);
+
+ if (q_real == q_test && result == 0)
+ return;
pr_warn("Test '%s' failed: flags = %#x\n", name, flags);
@@ -28,8 +37,6 @@ static __init bool test_string_check_buf(const char *name, unsigned int flags,
out_test, q_test, true);
print_hex_dump(KERN_WARNING, "Got: ", DUMP_PREFIX_NONE, 16, 1,
out_real, q_real, true);
-
- return false;
}
struct test_string {
@@ -38,7 +45,7 @@ struct test_string {
unsigned int flags;
};
-static const struct test_string strings[] __initconst = {
+static const struct test_string strings[] = {
{
.in = "\\f\\ \\n\\r\\t\\v",
.out = "\f\\ \n\r\t\v",
@@ -61,8 +68,9 @@ static const struct test_string strings[] __initconst = {
},
};
-static void __init test_string_unescape(const char *name, unsigned int flags,
- bool inplace)
+static void test_string_unescape(struct kunit *test,
+ const char *name, unsigned int flags,
+ bool inplace)
{
int q_real = 256;
char *in = kmalloc(q_real, GFP_KERNEL);
@@ -104,7 +112,7 @@ static void __init test_string_unescape(const char *name, unsigned int flags,
q_real = string_unescape(in, out_real, q_real, flags);
}
- test_string_check_buf(name, flags, in, p - 1, out_real, q_real,
+ test_string_check_buf(test, name, flags, in, p - 1, out_real, q_real,
out_test, q_test);
out:
kfree(out_real);
@@ -124,7 +132,7 @@ struct test_string_2 {
};
#define TEST_STRING_2_DICT_0 NULL
-static const struct test_string_2 escape0[] __initconst = {{
+static const struct test_string_2 escape0[] = {{
.in = "\f\\ \n\r\t\v",
.s1 = {{
.out = "\\f\\ \\n\\r\\t\\v",
@@ -222,7 +230,7 @@ static const struct test_string_2 escape0[] __initconst = {{
}};
#define TEST_STRING_2_DICT_1 "b\\ \t\r\xCF"
-static const struct test_string_2 escape1[] __initconst = {{
+static const struct test_string_2 escape1[] = {{
.in = "\f\\ \n\r\t\v",
.s1 = {{
.out = "\f\\134\\040\n\\015\\011\v",
@@ -359,7 +367,7 @@ static const struct test_string_2 escape1[] __initconst = {{
/* terminator */
}};
-static const struct test_string strings_upper[] __initconst = {
+static const struct test_string strings_upper[] = {
{
.in = "abcdefgh1234567890test",
.out = "ABCDEFGH1234567890TEST",
@@ -370,7 +378,7 @@ static const struct test_string strings_upper[] __initconst = {
},
};
-static const struct test_string strings_lower[] __initconst = {
+static const struct test_string strings_lower[] = {
{
.in = "ABCDEFGH1234567890TEST",
.out = "abcdefgh1234567890test",
@@ -381,8 +389,8 @@ static const struct test_string strings_lower[] __initconst = {
},
};
-static __init const char *test_string_find_match(const struct test_string_2 *s2,
- unsigned int flags)
+static const char *test_string_find_match(const struct test_string_2 *s2,
+ unsigned int flags)
{
const struct test_string_1 *s1 = s2->s1;
unsigned int i;
@@ -403,21 +411,20 @@ static __init const char *test_string_find_match(const struct test_string_2 *s2,
return NULL;
}
-static __init void
-test_string_escape_overflow(const char *in, int p, unsigned int flags, const char *esc,
+static void
+test_string_escape_overflow(struct kunit *test,
+ const char *in, int p, unsigned int flags, const char *esc,
int q_test, const char *name)
{
int q_real;
q_real = string_escape_mem(in, p, NULL, 0, flags, esc);
- if (q_real != q_test)
- pr_warn("Test '%s' failed: flags = %#x, osz = 0, expected %d, got %d\n",
- name, flags, q_test, q_real);
+ KUNIT_EXPECT_EQ(test, q_real, q_test);
}
-static __init void test_string_escape(const char *name,
- const struct test_string_2 *s2,
- unsigned int flags, const char *esc)
+static void test_string_escape(struct kunit *test, const char *name,
+ const struct test_string_2 *s2,
+ unsigned int flags, const char *esc)
{
size_t out_size = 512;
char *out_test = kmalloc(out_size, GFP_KERNEL);
@@ -463,10 +470,10 @@ static __init void test_string_escape(const char *name,
q_real = string_escape_mem(in, p, out_real, out_size, flags, esc);
- test_string_check_buf(name, flags, in, p, out_real, q_real, out_test,
+ test_string_check_buf(test, name, flags, in, p, out_real, q_real, out_test,
q_test);
- test_string_escape_overflow(in, p, flags, esc, q_test, name);
+ test_string_escape_overflow(test, in, p, flags, esc, q_test, name);
out:
kfree(in);
@@ -475,22 +482,26 @@ static __init void test_string_escape(const char *name,
}
#define string_get_size_maxbuf 16
-#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2) \
- do { \
- BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf); \
- BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf); \
- __test_string_get_size((size), (blk_size), (exp_result10), \
- (exp_result2)); \
+#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2) \
+ do { \
+ BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf); \
+ BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf); \
+ __test_string_get_size(test, (size), (blk_size), (exp_result10), \
+ (exp_result2)); \
} while (0)
-static __init void test_string_get_size_check(const char *units,
- const char *exp,
- char *res,
- const u64 size,
- const u64 blk_size)
+static void test_string_get_size_check(struct kunit *test,
+ const char *units,
+ const char *exp,
+ char *res,
+ const u64 size,
+ const u64 blk_size)
{
- if (!memcmp(res, exp, strlen(exp) + 1))
+ int result = memcmp(res, exp, strlen(exp) + 1);
+
+ KUNIT_EXPECT_EQ(test, 0, result);
+ if (!result)
return;
res[string_get_size_maxbuf - 1] = '\0';
@@ -501,7 +512,7 @@ static __init void test_string_get_size_check(const char *units,
pr_warn("expected: '%s', got '%s'\n", exp, res);
}
-static __init void __strchrcut(char *dst, const char *src, const char *cut)
+static void __strchrcut(char *dst, const char *src, const char *cut)
{
const char *from = src;
size_t len;
@@ -515,11 +526,12 @@ static __init void __strchrcut(char *dst, const char *src, const char *cut)
*dst = '\0';
}
-static __init void __test_string_get_size_one(const u64 size, const u64 blk_size,
- const char *exp_result10,
- const char *exp_result2,
- enum string_size_units units,
- const char *cut)
+static void __test_string_get_size_one(struct kunit *test,
+ const u64 size, const u64 blk_size,
+ const char *exp_result10,
+ const char *exp_result2,
+ enum string_size_units units,
+ const char *cut)
{
char buf10[string_get_size_maxbuf];
char buf2[string_get_size_maxbuf];
@@ -537,13 +549,14 @@ static __init void __test_string_get_size_one(const u64 size, const u64 blk_size
string_get_size(size, blk_size, STRING_UNITS_10 | units, buf10, sizeof(buf10));
string_get_size(size, blk_size, STRING_UNITS_2 | units, buf2, sizeof(buf2));
- test_string_get_size_check(prefix10, exp10, buf10, size, blk_size);
- test_string_get_size_check(prefix2, exp2, buf2, size, blk_size);
+ test_string_get_size_check(test, prefix10, exp10, buf10, size, blk_size);
+ test_string_get_size_check(test, prefix2, exp2, buf2, size, blk_size);
}
-static __init void __test_string_get_size(const u64 size, const u64 blk_size,
- const char *exp_result10,
- const char *exp_result2)
+static void __test_string_get_size(struct kunit *test,
+ const u64 size, const u64 blk_size,
+ const char *exp_result10,
+ const char *exp_result2)
{
struct {
enum string_size_units units;
@@ -557,12 +570,13 @@ static __init void __test_string_get_size(const u64 size, const u64 blk_size,
int i;
for (i = 0; i < ARRAY_SIZE(get_size_test_cases); i++)
- __test_string_get_size_one(size, blk_size, exp_result10, exp_result2,
+ __test_string_get_size_one(test, size, blk_size,
+ exp_result10, exp_result2,
get_size_test_cases[i].units,
get_size_test_cases[i].cut);
}
-static __init void test_string_get_size(void)
+static void test_get_size(struct kunit *test)
{
/* small values */
test_string_get_size_one(0, 512, "0 B", "0 B");
@@ -582,7 +596,7 @@ static __init void test_string_get_size(void)
test_string_get_size_one(4096, U64_MAX, "75.6 ZB", "64.0 ZiB");
}
-static void __init test_string_upper_lower(void)
+static void test_upper_lower(struct kunit *test)
{
char *dst;
int i;
@@ -590,65 +604,68 @@ static void __init test_string_upper_lower(void)
for (i = 0; i < ARRAY_SIZE(strings_upper); i++) {
const char *s = strings_upper[i].in;
int len = strlen(strings_upper[i].in) + 1;
+ int result;
dst = kmalloc(len, GFP_KERNEL);
- if (!dst)
- return;
+ KUNIT_ASSERT_NOT_NULL(test, dst);
string_upper(dst, s);
- if (memcmp(dst, strings_upper[i].out, len)) {
+ result = memcmp(dst, strings_upper[i].out, len);
+ KUNIT_EXPECT_EQ(test, 0, result);
+ if (result)
pr_warn("Test 'string_upper' failed : expected %s, got %s!\n",
strings_upper[i].out, dst);
- kfree(dst);
- return;
- }
kfree(dst);
}
for (i = 0; i < ARRAY_SIZE(strings_lower); i++) {
const char *s = strings_lower[i].in;
int len = strlen(strings_lower[i].in) + 1;
+ int result;
dst = kmalloc(len, GFP_KERNEL);
- if (!dst)
- return;
+ KUNIT_ASSERT_NOT_NULL(test, dst);
string_lower(dst, s);
- if (memcmp(dst, strings_lower[i].out, len)) {
+ result = memcmp(dst, strings_lower[i].out, len);
+ KUNIT_EXPECT_EQ(test, 0, result);
+ if (result)
pr_warn("Test 'string_lower failed : : expected %s, got %s!\n",
strings_lower[i].out, dst);
- kfree(dst);
- return;
- }
kfree(dst);
}
}
-static int __init test_string_helpers_init(void)
+static void test_unescape(struct kunit *test)
{
unsigned int i;
- pr_info("Running tests...\n");
for (i = 0; i < UNESCAPE_ALL_MASK + 1; i++)
- test_string_unescape("unescape", i, false);
- test_string_unescape("unescape inplace",
+ test_string_unescape(test, "unescape", i, false);
+ test_string_unescape(test, "unescape inplace",
get_random_u32_below(UNESCAPE_ALL_MASK + 1), true);
/* Without dictionary */
for (i = 0; i < ESCAPE_ALL_MASK + 1; i++)
- test_string_escape("escape 0", escape0, i, TEST_STRING_2_DICT_0);
+ test_string_escape(test, "escape 0", escape0, i, TEST_STRING_2_DICT_0);
/* With dictionary */
for (i = 0; i < ESCAPE_ALL_MASK + 1; i++)
- test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1);
+ test_string_escape(test, "escape 1", escape1, i, TEST_STRING_2_DICT_1);
+}
- /* Test string_get_size() */
- test_string_get_size();
+static struct kunit_case string_helpers_test_cases[] = {
+ KUNIT_CASE(test_get_size),
+ KUNIT_CASE(test_upper_lower),
+ KUNIT_CASE(test_unescape),
+ {}
+};
- /* Test string upper(), string_lower() */
- test_string_upper_lower();
+static struct kunit_suite string_helpers_test_suite = {
+ .name = "string_helpers",
+ .test_cases = string_helpers_test_cases,
+};
+
+kunit_test_suites(&string_helpers_test_suite);
- return -EINVAL;
-}
-module_init(test_string_helpers_init);
MODULE_LICENSE("Dual BSD/GPL");
--
2.34.1
Convert test_string.c to KUnit so it can be easily run with everything
else.
Signed-off-by: Kees Cook <[email protected]>
---
Cc: Andy Shevchenko <[email protected]>
Cc: [email protected]
---
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%)
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");
--
2.34.1
On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <[email protected]> wrote:
>
> Convert test-string_helpers.c to KUnit so it can be easily run with
> everything else.
..
> -#include <linux/array_size.h>
> #include <linux/init.h>
> +#include <kunit/test.h>
I know the order is broken here, but don't make it worse, please. And
stick with one schema where to put kunit/test.h always before
everything else and somewhere else (like always after linux/*).
> +#include <linux/array_size.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/module.h>
..
> +static void test_string_check_buf(struct kunit *test,
> + const char *name, unsigned int flags,
> + char *in, size_t p,
> + char *out_real, size_t q_real,
> + char *out_test, size_t q_test)
> {
> - if (q_real == q_test && !memcmp(out_test, out_real, q_test))
> - return true;
> + int result;
> +
> + KUNIT_EXPECT_EQ(test, q_real, q_test);
This needs a message.
> + result = memcmp(out_test, out_real, q_test);
> + KUNIT_EXPECT_EQ(test, 0, result);
Why do we need this assertion? We have a dump below to show what's wrong.
> + if (q_real == q_test && result == 0)
> + return;
I'm not sure this is an equivalent change. IIRC KUnit assertions do
not continue on failure. (Long time last time I run KUnit test)
>
> pr_warn("Test '%s' failed: flags = %#x\n", name, flags);
>
> @@ -28,8 +37,6 @@ static __init bool test_string_check_buf(const char *name, unsigned int flags,
> out_test, q_test, true);
> print_hex_dump(KERN_WARNING, "Got: ", DUMP_PREFIX_NONE, 16, 1,
> out_real, q_real, true);
> -
> - return false;
> }
..
> +static void
> +test_string_escape_overflow(struct kunit *test,
> + const char *in, int p, unsigned int flags, const char *esc,
> int q_test, const char *name)
> {
> int q_real;
>
> q_real = string_escape_mem(in, p, NULL, 0, flags, esc);
> - if (q_real != q_test)
> - pr_warn("Test '%s' failed: flags = %#x, osz = 0, expected %d, got %d\n",
> - name, flags, q_test, q_real);
> + KUNIT_EXPECT_EQ(test, q_real, q_test);
You killed the message, not good.
> }
..
> +#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2) \
> + do { \
> + BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf); \
> + BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf); \
No analogous assertions in KUnit?
> + __test_string_get_size(test, (size), (blk_size), (exp_result10), \
> + (exp_result2)); \
> } while (0)
..
> {
> - if (!memcmp(res, exp, strlen(exp) + 1))
> + int result = memcmp(res, exp, strlen(exp) + 1);
> + KUNIT_EXPECT_EQ(test, 0, result);
As per above, what's the added value of this assertion?
> + if (!result)
> return;
..
> @@ -590,65 +604,68 @@ static void __init test_string_upper_lower(void)
> for (i = 0; i < ARRAY_SIZE(strings_upper); i++) {
> const char *s = strings_upper[i].in;
> int len = strlen(strings_upper[i].in) + 1;
> + int result;
>
> dst = kmalloc(len, GFP_KERNEL);
> - if (!dst)
> - return;
> + KUNIT_ASSERT_NOT_NULL(test, dst);
>
> string_upper(dst, s);
> - if (memcmp(dst, strings_upper[i].out, len)) {
> + result = memcmp(dst, strings_upper[i].out, len);
> + KUNIT_EXPECT_EQ(test, 0, result);
Ditto.
> + if (result)
> pr_warn("Test 'string_upper' failed : expected %s, got %s!\n",
> strings_upper[i].out, dst);
> - kfree(dst);
> - return;
> - }
> kfree(dst);
> }
>
> for (i = 0; i < ARRAY_SIZE(strings_lower); i++) {
> const char *s = strings_lower[i].in;
> int len = strlen(strings_lower[i].in) + 1;
> + int result;
>
> dst = kmalloc(len, GFP_KERNEL);
> - if (!dst)
> - return;
> + KUNIT_ASSERT_NOT_NULL(test, dst);
>
> string_lower(dst, s);
> - if (memcmp(dst, strings_lower[i].out, len)) {
> + result = memcmp(dst, strings_lower[i].out, len);
> + KUNIT_EXPECT_EQ(test, 0, result);
Ditto.
> + if (result)
> pr_warn("Test 'string_lower failed : : expected %s, got %s!\n",
> strings_lower[i].out, dst);
> - kfree(dst);
> - return;
> - }
> kfree(dst);
> }
> }
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <[email protected]> wrote:
>
> Hi,
>
> I realized the string selftests hadn't been converted to KUnit yet. Do that.
IIRC last time somebody wanted to do that KUnit was completely sucking
in supporting the cases we need in these files. (I mean proper
messaging when we need to dump the expected and resulting data). And
please Cc Rasmus as well.
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 01, 2024 at 01:09:27PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <[email protected]> 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
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 <[email protected]> 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).
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 01, 2024 at 01:20:41PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <[email protected]> wrote:
> >
> > Convert test-string_helpers.c to KUnit so it can be easily run with
> > everything else.
>
> ...
>
> > -#include <linux/array_size.h>
> > #include <linux/init.h>
> > +#include <kunit/test.h>
>
> I know the order is broken here, but don't make it worse, please. And
> stick with one schema where to put kunit/test.h always before
> everything else and somewhere else (like always after linux/*).
Fixed.
>
> > +#include <linux/array_size.h>
> > #include <linux/kernel.h>
> > #include <linux/slab.h>
> > #include <linux/module.h>
>
> ...
>
> > +static void test_string_check_buf(struct kunit *test,
> > + const char *name, unsigned int flags,
> > + char *in, size_t p,
> > + char *out_real, size_t q_real,
> > + char *out_test, size_t q_test)
> > {
> > - if (q_real == q_test && !memcmp(out_test, out_real, q_test))
> > - return true;
> > + int result;
> > +
> > + KUNIT_EXPECT_EQ(test, q_real, q_test);
>
> This needs a message.
>
> > + result = memcmp(out_test, out_real, q_test);
>
> > + KUNIT_EXPECT_EQ(test, 0, result);
>
> Why do we need this assertion? We have a dump below to show what's wrong.
I've improved this all around with ...STREQ and ...MEMEQ calls, and
added _MSG versions where needed to retain the prior reported test
context.
> ...
>
> > +#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2) \
> > + do { \
> > + BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf); \
> > + BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf); \
>
> No analogous assertions in KUnit?
It's designed for runtime testing. This is fine as-is for compile-time
asserts.
Thanks for the reviews!
-Kees
--
Kees Cook