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@xxxxxxxxxxx> > > --- > > 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 the > > debug log > > + in TAP format (http://testanything.org/). Only useful for > > kernel devs > > + running KUnit test harness and are not 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. > > + > > 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.
