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?
>
> 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...)
> --- 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.
> +/*
> + * This code is the conversion of the overflow test in runtime to KUnit tests.
> + */
> +
This can be left off.
> /*
> * 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?
> 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...)
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. :)
--
Kees Cook
