On Fri, 2020-06-12 at 15:36 -0700, Kees Cook wrote:
On Thu, Jun 11, 2020 at 06:55:01PM -0300, Vitor Massaru Iha wrote:
This adds the convertion of the runtime tests of check_*_overflow fuctions, from `lib/test_overflow.c`to KUnit tests.
The log similar to the one seen in dmesg running test_overflow can be seen in `test.log`.
Signed-off-by: Vitor Massaru Iha vitor@massaru.org
lib/Kconfig.debug | 17 ++ lib/Makefile | 1 + lib/kunit_overflow_test.c | 590 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 608 insertions(+) create mode 100644 lib/kunit_overflow_test.c
What tree is this based on? I can't apply it to v5.7, linux-next, nor Linus's latest. I've fixed it up to apply to linux-next for now. :)
Looking at linux-next, though, I am reminded of my agony over naming:
obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += kunit_overflow_test.o
*-test test_* *_test
This has to get fixed now, and the naming convention needs to be documented. For old tests, the preferred naming was test_*. For kunit, I think it should be kunit_* (and no trailing _test; that's redundant).
For for this bikeshed, I think it should be kunit_overflow.c
For the CONFIG name, it seems to be suggested in docs to be *_KUNIT_TEST:
... menuconfig). From there, you can enable any KUnit tests you want: they usually have config options ending in ``_KUNIT_TEST``. ...
I think much stronger language needs to be added to "Writing your first test" (which actually recommends the wrong thing: "config MISC_EXAMPLE_TEST"). And then doesn't specify a module file name, though it hints at one:
... obj-$(CONFIG_MISC_EXAMPLE_TEST) += example-test.o ...
So, please, let's get this documented: we really really need a single naming convention for these.
For Kconfig in the tree, I see:
drivers/base/Kconfig:config PM_QOS_KUNIT_TEST drivers/base/test/Kconfig:config KUNIT_DRIVER_PE_TEST fs/ext4/Kconfig:config EXT4_KUNIT_TESTS lib/Kconfig.debug:config SYSCTL_KUNIT_TEST lib/Kconfig.debug:config OVERFLOW_KUNIT_TEST lib/Kconfig.debug:config LIST_KUNIT_TEST lib/Kconfig.debug:config LINEAR_RANGES_TEST lib/kunit/Kconfig:menuconfig KUNIT lib/kunit/Kconfig:config KUNIT_DEBUGFS lib/kunit/Kconfig:config KUNIT_TEST lib/kunit/Kconfig:config KUNIT_EXAMPLE_TEST lib/kunit/Kconfig:config KUNIT_ALL_TESTS
Which is:
*_KUNIT_TEST KUNIT_*_TEST KUNIT_*_TESTS *_TEST
Nooooo. ;)
If it should all be *_KUNIT_TEST, let's do that. I think just *_KUNIT would be sufficient (again, adding the word "test" to "kunit" is redundant). And it absolutely should not be a prefix, otherwise it'll get sorted away from the thing it's named after. So my preference is here would be CONFIG_OVERFLOW_KUNIT. (Yes the old convention was CONFIG_TEST_*, but those things tended to be regression tests, not unit tests.)
Please please, can we fix this before we add anything more?
Sure, I'll rewrite with _KUNIT_TEST, as David Gow suggested in the next emails.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 1f4ab7a2bdee..72fcfe1f24a4 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2075,6 +2075,23 @@ config SYSCTL_KUNIT_TEST
If unsure, say N.+config OVERFLOW_KUNIT_TEST
- tristate "KUnit test for overflow" if !KUNIT_ALL_TESTS
- depends on KUNIT
- default KUNIT_ALL_TESTS
- help
This builds the overflow KUnit tests.KUnit tests run during boot and output the results to thedebug log
in TAP format (http://testanything.org/). Only useful forkernel devs
running KUnit test harness and are not for inclusion into aproduction
build.For more information on KUnit and unit tests in generalplease refer
to the KUnit documentation in Documentation/dev-tools/kunit/.If unsure, say N.config LIST_KUNIT_TEST tristate "KUnit Test for Kernel Linked-list structures" if !KUNIT_ALL_TESTS depends on KUNIT
Regarding output:
[ 36.611358] TAP version 14 [ 36.611953] # Subtest: overflow [ 36.611954] 1..3 ... [ 36.622914] # overflow_calculation_test: s64: 21 arithmetic tests [ 36.624020] ok 1 - overflow_calculation_test ... [ 36.731096] # overflow_shift_test: ok: (s64)(0 << 63) == 0 [ 36.731840] ok 2 - overflow_shift_test ... [ 36.750294] kunit_try_catch: vmalloc: allocation failure: 18446744073709551615 bytes, mode:0xcc0(GFP_KERNEL), nodemask=(null),cpuset=/,mems_allowed=0 ... [ 36.805350] # overflow_allocation_test: devm_kzalloc detected saturation [ 36.807763] ok 3 - overflow_allocation_test [ 36.807765] ok 1 - overflow
A few things here....
- On the outer test report, there is no "plan" line (I was expecting "1..1"). Technically it's optional, but it seems like the
information is available. :)
- The subtest should have its own "TAP version 14" line, and it
should be using the diagnostic line prefix for the top-level test (this is what kselftest is doing).
There is no way to distinguish top-level TAP output from kernel log lines. I think we should stick with the existing marker, which is "# ", so that kernel output has no way to be interpreted as TAP details -- unless it intentionally starts adding "#"s. ;)
There is no summary line (to help humans). For example, the
kselftest API produces a final pass/fail report.
Taken together, I was expecting the output to be:
[ 36.611358] # TAP version 14 [ 36.611953] # 1..1 [ 36.611958] # # TAP version 14 [ 36.611954] # # 1..3 ... [ 36.622914] # # # overflow_calculation_test: s64: 21 arithmetic tests [ 36.624020] # # ok 1 - overflow_calculation_test ... [ 36.731096] # # # overflow_shift_test: ok: (s64)(0 << 63) == 0 [ 36.731840] # # ok 2 - overflow_shift_test ... [ 36.750294] kunit_try_catch: vmalloc: allocation failure: 18446744073709551615 bytes, mode:0xcc0(GFP_KERNEL), nodemask=(null),cpuset=/,mems_allowed=0 ... [ 36.805350] # # # overflow_allocation_test: devm_kzalloc detected saturation [ 36.807763] # # ok 3 - overflow_allocation_test [ 36.807763] # # # overflow: PASS [ 36.807765] # ok 1 - overflow [ 36.807767] # # kunit: PASS
But I assume there are threads on this that I've not read... :)
Now, speaking to actual behavior, I love it. :) All the tests are there (and then some -- noted below).
diff --git a/lib/Makefile b/lib/Makefile index 685aee60de1d..a3290adc0019 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -309,3 +309,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
# KUnit tests obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o +obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += kunit_overflow_test.o diff --git a/lib/kunit_overflow_test.c b/lib/kunit_overflow_test.c new file mode 100644 index 000000000000..c3eb8f0d3d50 --- /dev/null +++ b/lib/kunit_overflow_test.c
A lot of this file is unchanged, so I would suggest doing this as a "git mv lib/test_overflow.c lib/kunit_overflow.c" and then put the changes into the file. Then it should be easier to track git history, etc.
Without this, it's a lot harder to review this patch since I'm just looking at a 590 new lines. ;) Really, it's a diff (which I'll paste here for the code review...)
Sure, I'll do it. I didn't know if the runtime tests were going to stay.
--- a/lib/test_overflow.c 2020-06-12 14:07:11.161999209 -0700 +++ b/lib/kunit_overflow_test.c 2020-06-12 14:07:27.950183116 -0700 @@ -1,17 +1,18 @@ -// SPDX-License-Identifier: GPL-2.0 OR MIT +// SPDX-License-Identifier: GPL-2.0
Please don't change the license.
Sure I'll fix it.
+/*
- This code is the conversion of the overflow test in runtime to
KUnit tests.
- */
This can be left off.
Sure I'll fix it.
/*
- Test cases for arithmetic overflow checks.
*/ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt +#include <kunit/test.h> #include <linux/device.h> #include <linux/init.h> -#include <linux/kernel.h> #include <linux/mm.h> -#include <linux/module.h> #include <linux/overflow.h> -#include <linux/slab.h> -#include <linux/types.h> #include <linux/vmalloc.h> #define DEFINE_TEST_ARRAY(t) \ @@ -19,7 +20,7 @@ t a, b; \ t sum, diff, prod; \ bool s_of, d_of, p_of; \
- } t ## _tests[] __initconst
- } t ## _tests[]
Why drop the __initconst?
I removed __initconst because of these warnings below, as it is used for the kernel during the module initialization, and I do not use the module initialization in this tests. Does this have any side effects in these tests?
WARNING: modpost: vmlinux.o(.text.unlikely+0x131b7): Section mismatch in reference from the function test_s8_overflow() to the variable .init.rodata:s8_tests The function test_s8_overflow() references the variable __initconst s8_tests. This is often because test_s8_overflow lacks a __initconst annotation or the annotation of s8_tests is wrong.
DEFINE_TEST_ARRAY(u8) = { {0, 0, 0, 0, 0, false, false, false}, @@ -44,6 +45,7 @@ {128, 128, 0, 0, 0, true, false, true}, {123, 234, 101, 145, 110, true, true, true}, };
Style nit: I'd like to avoid the blank lines here.
DEFINE_TEST_ARRAY(u16) = { {0, 0, 0, 0, 0, false, false, false}, {1, 1, 2, 0, 1, false, false, false}, @@ -66,6 +68,7 @@ {123, 234, 357, 65425, 28782, false, true, false}, {1234, 2345, 3579, 64425, 10146, false, true, true}, };
DEFINE_TEST_ARRAY(u32) = { {0, 0, 0, 0, 0, false, false, false}, {1, 1, 2, 0, 1, false, false, false}, @@ -163,6 +166,7 @@ {S16_MIN, S16_MIN, 0, 0, 0, true, false, true}, {S16_MAX, S16_MAX, -2, 0, 1, true, false, true}, };
DEFINE_TEST_ARRAY(s32) = { {0, 0, 0, 0, 0, false, false, false}, @@ -186,6 +190,7 @@ {S32_MIN, S32_MIN, 0, 0, 0, true, false, true}, {S32_MAX, S32_MAX, -2, 0, 1, true, false, true}, };
DEFINE_TEST_ARRAY(s64) = { {0, 0, 0, 0, 0, false, false, false}, @@ -215,254 +220,243 @@ {0, -S64_MAX, -S64_MAX, S64_MAX, 0, false, false, false}, }; -#define check_one_op(t, fmt, op, sym, a, b, r, of) do { \
- t _r; \
- bool _of; \
\- _of = check_ ## op ## _overflow(a, b, &_r); \
- if (_of != of) { \
pr_warn("expected "fmt" "sym" "fmt \" to%s overflow (type %s)\n", \a, b, of ? "" : " not", #t); \err = 1; \- } \
- if (_r != r) { \
pr_warn("expected "fmt" "sym" "fmt" == " \fmt", got "fmt" (type %s)\n", \a, b, r, _r, #t); \err = 1; \- } \
+#define check_one_op(test, t, fmt, op, sym, a, b, r, of) do { \
- t _r;
\
- bool _of; \
\
- _of = check_ ## op ## _overflow(a, b, &_r); \
- if (_of != of) { \
KUNIT_FAIL(test, "Expected "fmt" "sym" "fmt \" to%s overflow (type %s)\n",\
a, b, of ? "" : " not", #t);\
- } \
- if (_r != r) {
\
KUNIT_FAIL(test, "Expected "fmt" "sym" "fmt" == " \fmt", got "fmt" (type %s)\n",\
a, b, r, _r, #t); \- } \
} while (0)
The trailing \ makes this more awkward to diff, but one thing I'm not quite seeing is why "test" needs to be added. It's not a variable in these macros. i.e. it is used literally:
#define DEFINE_TEST_FUNC(test, t, fmt) \ static void do_test_ ## t(struct kunit *test, const struct test_ ## t *p) \ { \ check_one_op(test, t, fmt, add, "+", p->a, p->b, p->sum, p-
s_of); \
...
Only callers of the do_test_*() would need to be changed. I think all of these macros just need the pr_warn/KUNIT_FAIL changes, and the function prototypes updated to include struct kunit *test.
-#define DEFINE_TEST_FUNC(t, fmt) \ -static int __init do_test_ ## t(const struct test_ ## t *p) \ -{ \
- int err = 0;
\
\
- check_one_op(t, fmt, add, "+", p->a, p->b, p->sum, p->s_of);
\
- check_one_op(t, fmt, add, "+", p->b, p->a, p->sum, p->s_of);
\
- check_one_op(t, fmt, sub, "-", p->a, p->b, p->diff, p->d_of);
\
- check_one_op(t, fmt, mul, "*", p->a, p->b, p->prod, p->p_of);
\
- check_one_op(t, fmt, mul, "*", p->b, p->a, p->prod, p->p_of);
\
\
- return err; \
-} \
\ -static int __init test_ ## t ## _overflow(void) { \
- int err = 0;
\
- unsigned i; \
\
- pr_info("%-3s: %zu arithmetic tests\n", #t, \
ARRAY_SIZE(t ## _tests)); \- for (i = 0; i < ARRAY_SIZE(t ## _tests); ++i)
\
err |= do_test_ ## t(&t ## _tests[i]);\
- return err; \
+#define DEFINE_TEST_FUNC(test, t, fmt) \ +static void do_test_ ## t(struct kunit *test, const struct test_ ## t *p) \ +{ \
- check_one_op(test, t, fmt, add, "+", p->a, p->b, p->sum, p-
s_of); \
- check_one_op(test, t, fmt, add, "+", p->b, p->a, p->sum, p-
s_of); \
- check_one_op(test, t, fmt, sub, "-", p->a, p->b, p->diff, p-
d_of); \
- check_one_op(test, t, fmt, mul, "*", p->a, p->b, p->prod, p-
p_of); \
- check_one_op(test, t, fmt, mul, "*", p->b, p->a, p->prod, p-
p_of); \
+} \
Then these all only need the prototype on the actual function changed.
+static void test_ ## t ## _overflow(struct kunit *test) \ +{ \
- unsigned i;
\
- kunit_warn(test, "%-3s: %zu arithmetic tests\n", #t, \
ARRAY_SIZE(t ## _tests));\
- for (i = 0; i < ARRAY_SIZE(t ## _tests); ++i) \
do_test_ ## t(test, &t ## _tests[i]);} -DEFINE_TEST_FUNC(u8, "%d"); -DEFINE_TEST_FUNC(s8, "%d"); -DEFINE_TEST_FUNC(u16, "%d"); -DEFINE_TEST_FUNC(s16, "%d"); -DEFINE_TEST_FUNC(u32, "%u"); -DEFINE_TEST_FUNC(s32, "%d"); +DEFINE_TEST_FUNC(test, u8, "%d"); +DEFINE_TEST_FUNC(test, s8, "%d"); +DEFINE_TEST_FUNC(test, u16, "%d"); +DEFINE_TEST_FUNC(test, s16, "%d"); +DEFINE_TEST_FUNC(test, u32, "%u"); +DEFINE_TEST_FUNC(test, s32, "%d"); #if BITS_PER_LONG == 64 -DEFINE_TEST_FUNC(u64, "%llu"); -DEFINE_TEST_FUNC(s64, "%lld"); +DEFINE_TEST_FUNC(test, u64, "%llu"); +DEFINE_TEST_FUNC(test, s64, "%lld"); #endif
And all the actual uses of the macros can be left unchanged.
-static int __init test_overflow_calculation(void) +static void overflow_calculation_test(struct kunit *test) {
- int err = 0;
- err |= test_u8_overflow();
- err |= test_s8_overflow();
- err |= test_u16_overflow();
- err |= test_s16_overflow();
- err |= test_u32_overflow();
- err |= test_s32_overflow();
- test_u8_overflow(test);
- test_s8_overflow(test);
- test_s8_overflow(test);
The s8 test got added twice here accidentally.
- test_u16_overflow(test);
- test_s16_overflow(test);
- test_u32_overflow(test);
- test_s32_overflow(test);
#if BITS_PER_LONG == 64
- err |= test_u64_overflow();
- err |= test_s64_overflow();
- test_u64_overflow(test);
- test_s64_overflow(test);
#endif
- return err;
}
I think it might be nice to keep the "err" vars around for a final report line (maybe per test)? (It would keep the diff churn way lower, too...)
I will correct these other suggestions.
So, yes! I like it. :) Most of my comments here have nothing to do with specifically this patch (sorry)! But I'd love to see a v2.
Thanks for doing this! I'm glad to see more TAP output. :)
Thanks Kees, I'm learning a lot from you, and as I said privately with Brendan, I've never seen so much macro in a code. I learned a lot from it.