| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADQ6JjUm10vPcSO5Wto=BjfM7dtdQZ924d0x-C6kxQKKZJr=qQ@mail.gmail.com>
Date: Tue, 21 Jul 2020 16:37:59 -0300
From: Vitor Massaru Iha <vitor@...saru.org>
To: Kees Cook <keescook@...omium.org>
Cc: KUnit Development <kunit-dev@...glegroups.com>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Brendan Higgins <brendanhiggins@...gle.com>,
David Gow <davidgow@...gle.com>,
Shuah Khan <skhan@...uxfoundation.org>,
linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow
On Tue, Jul 21, 2020 at 3:55 PM Kees Cook <keescook@...omium.org> wrote:
>
> On Mon, Jul 20, 2020 at 07:44:18PM -0300, Vitor Massaru Iha wrote:
> > This adds the conversion of the runtime tests of check_*_overflow functions,
> > from `lib/test_overflow.c`to KUnit tests.
> >
> > The log similar to the one seen in dmesg running test_overflow.c can be
> > seen in `test.log`.
> >
> > Signed-off-by: Vitor Massaru Iha <vitor@...saru.org>
> > Tested-by: David Gow <davidgow@...gle.com>
> > ---
> > v2:
> > * moved lib/test_overflow.c to lib/overflow-test.c;
> > * back to original license;
> > * fixed style code;
> > * keeps __initconst and added _refdata on overflow_test_cases variable;
> > * keeps macros intact making asserts with the variable err;
> > * removed duplicate test_s8_overflow();
> > * fixed typos on commit message;
> >
> > v3:
> > * changed filename to overflow_kunit.c;
> > * replace _refdata by _inidata;
>
> It looks like this still needs to be _refdata (says the test bot)
I replaced it because you said `Erm, __refdata? This seems like it
should be __initdata.` in v2.
>
> > -static int __init test_ ## t ## _overflow(void) { \
> > +static int __init test_ ## t ## _overflow(struct kunit *test) { \
>
> style nit: it seems like "test" isn't a great variable name. Why not
> make this "kunit" or "context" or something more specific?
I tried to follow the pattern I saw in other KUnit tests.
>
> > int err = 0; \
> > unsigned i; \
> > \
> > @@ -256,6 +253,7 @@ static int __init test_ ## t ## _overflow(void) { \
> > ARRAY_SIZE(t ## _tests)); \
> > for (i = 0; i < ARRAY_SIZE(t ## _tests); ++i) \
> > err |= do_test_ ## t(&t ## _tests[i]); \
> > + KUNIT_EXPECT_FALSE(test, err); \
> > return err; \
> > }
>
> Also, if the caller is being made "void", probably this can be made void
> too?
>
> And if that's happening, maybe just plumb the EXPECT into the
> do_test_... call instead?
I did something similar in v1, but you said:
"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."
>
> >
> > @@ -270,25 +268,25 @@ DEFINE_TEST_FUNC(u64, "%llu");
> > DEFINE_TEST_FUNC(s64, "%lld");
> > #endif
> >
> > -static int __init test_overflow_calculation(void)
> > +static void __init 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();
> > + err |= test_u8_overflow(test);
> > + err |= test_s8_overflow(test);
> > + err |= test_u16_overflow(test);
> > + err |= test_s16_overflow(test);
> > + err |= test_u32_overflow(test);
> > + err |= test_s32_overflow(test);
> > #if BITS_PER_LONG == 64
> > - err |= test_u64_overflow();
> > - err |= test_s64_overflow();
> > + err |= test_u64_overflow(test);
> > + err |= test_s64_overflow(test);
> > #endif
> >
> > - return err;
> > + KUNIT_EXPECT_FALSE(test, err);
>
> This seems redundant (the tests were already tested)?
Yep, I just tried to do something you said in v1:
"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...)"
> > }
> >
> > -static int __init test_overflow_shift(void)
> > +static void __init overflow_shift_test(struct kunit *test)
> > {
> > int err = 0;
> >
> > @@ -313,9 +311,9 @@ static int __init test_overflow_shift(void)
> > pr_warn("got %llu\n", (u64)__d); \
> > __failed = 1; \
> > } \
> > - if (!__failed) \
> > - pr_info("ok: (%s)(%s << %s) == %s\n", #t, #a, #s, \
> > - of ? "overflow" : #expect); \
> > + KUNIT_EXPECT_FALSE_MSG(test, __failed, \
> > + "ok: (%s)(%s << %s) == %s\n", #t, #a, #s,\
> > + of ? "overflow" : #expect); \
> > __failed; \
> > })
> >
> > @@ -479,7 +477,7 @@ static int __init test_overflow_shift(void)
> > err |= TEST_ONE_SHIFT(0, 31, s32, 0, false);
> > err |= TEST_ONE_SHIFT(0, 63, s64, 0, false);
> >
> > - return err;
> > + KUNIT_EXPECT_FALSE(test, err);
> > }
> >
> > /*
> > @@ -499,7 +497,7 @@ static int __init test_overflow_shift(void)
> > #define TEST_SIZE (5 * 4096)
> >
> > #define DEFINE_TEST_ALLOC(func, free_func, want_arg, want_gfp, want_node)\
> > -static int __init test_ ## func (void *arg) \
> > +static int __init test_ ## func (struct kunit *test, void *arg) \
> > { \
> > volatile size_t a = TEST_SIZE; \
> > volatile size_t b = (SIZE_MAX / TEST_SIZE) + 1; \
> > @@ -507,19 +505,15 @@ static int __init test_ ## func (void *arg) \
> > \
> > /* Tiny allocation test. */ \
> > ptr = alloc ## want_arg ## want_gfp ## want_node (func, arg, 1);\
> > - if (!ptr) { \
> > - pr_warn(#func " failed regular allocation?!\n"); \
> > - return 1; \
> > - } \
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL_MSG(test, ptr, \
> > + #func " failed regular allocation?!\n"); \
> > free ## want_arg (free_func, arg, ptr); \
> > \
> > /* Wrapped allocation test. */ \
> > ptr = alloc ## want_arg ## want_gfp ## want_node (func, arg, \
> > a * b); \
> > - if (!ptr) { \
> > - pr_warn(#func " unexpectedly failed bad wrapping?!\n"); \
> > - return 1; \
> > - } \
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL_MSG(test, ptr, \
> > + #func " unexpectedly failed bad wrapping?!\n"); \
> > free ## want_arg (free_func, arg, ptr); \
> > \
> > /* Saturated allocation test. */ \
> > @@ -555,7 +549,7 @@ DEFINE_TEST_ALLOC(kvzalloc_node, kvfree, 0, 1, 1);
> > DEFINE_TEST_ALLOC(devm_kmalloc, devm_kfree, 1, 1, 0);
> > DEFINE_TEST_ALLOC(devm_kzalloc, devm_kfree, 1, 1, 0);
> >
> > -static int __init test_overflow_allocation(void)
> > +static void __init overflow_allocation_test(struct kunit *test)
> > {
> > const char device_name[] = "overflow-test";
> > struct device *dev;
> > @@ -563,52 +557,40 @@ static int __init test_overflow_allocation(void)
> >
> > /* Create dummy device for devm_kmalloc()-family tests. */
> > dev = root_device_register(device_name);
> > - if (IS_ERR(dev)) {
> > - pr_warn("Cannot register test device\n");
> > - return 1;
> > - }
> > -
> > - err |= test_kmalloc(NULL);
> > - err |= test_kmalloc_node(NULL);
> > - err |= test_kzalloc(NULL);
> > - err |= test_kzalloc_node(NULL);
> > - err |= test_kvmalloc(NULL);
> > - err |= test_kvmalloc_node(NULL);
> > - err |= test_kvzalloc(NULL);
> > - err |= test_kvzalloc_node(NULL);
> > - err |= test_vmalloc(NULL);
> > - err |= test_vmalloc_node(NULL);
> > - err |= test_vzalloc(NULL);
> > - err |= test_vzalloc_node(NULL);
> > - err |= test_devm_kmalloc(dev);
> > - err |= test_devm_kzalloc(dev);
> > + KUNIT_ASSERT_FALSE_MSG(test, IS_ERR(dev), "Cannot register test device\n");
> > +
> > + err |= test_kmalloc(test, NULL);
> > + err |= test_kmalloc_node(test, NULL);
> > + err |= test_kzalloc(test, NULL);
> > + err |= test_kzalloc_node(test, NULL);
> > + err |= test_kvmalloc(test, NULL);
> > + err |= test_kvmalloc_node(test, NULL);
> > + err |= test_kvzalloc(test, NULL);
> > + err |= test_kvzalloc_node(test, NULL);
> > + err |= test_vmalloc(test, NULL);
> > + err |= test_vmalloc_node(test, NULL);
> > + err |= test_vzalloc(test, NULL);
> > + err |= test_vzalloc_node(test, NULL);
> > + err |= test_devm_kmalloc(test, dev);
> > + err |= test_devm_kzalloc(test, dev);
> >
> > device_unregister(dev);
> >
> > - return err;
> > + KUNIT_EXPECT_FALSE(test, err);
> > }
> >
> > -static int __init test_module_init(void)
> > -{
> > - int err = 0;
> > -
> > - err |= test_overflow_calculation();
> > - err |= test_overflow_shift();
> > - err |= test_overflow_allocation();
> > -
> > - if (err) {
> > - pr_warn("FAIL!\n");
> > - err = -EINVAL;
> > - } else {
> > - pr_info("all tests passed\n");
> > - }
>
> The reason for older feedback on leaving "err" as it was, was to make
> sure it was easy for a human to see if everything passed or not. If
> KUnit provides a summary of all the tests at the end, then I don't need
> to preserve that here (in which case "err" can go away). However, if
> that summary does not exist for KUnit yet, then I'd like to keep the
> summary that is being removed here.
Kunit shows the result this way:
[16:24:44] ======== [PASSED] overflow ========
[16:24:44] [PASSED] overflow_calculation_test
[16:24:44] [PASSED] overflow_shift_test
[16:24:44] [PASSED] overflow_allocation_test
[16:24:44] ============================================================
>
> > +static struct kunit_case __initdata overflow_test_cases[] = {
> > + KUNIT_CASE(overflow_calculation_test),
> > + KUNIT_CASE(overflow_shift_test),
> > + KUNIT_CASE(overflow_allocation_test),
> > + {}
> > +};
> >
> > - return err;
> > -}
> > +static struct kunit_suite __initdata overflow_test_suite = {
> > + .name = "overflow",
> > + .test_cases = overflow_test_cases,
> > +};
> >
> > -static void __exit test_module_exit(void)
> > -{ }
> > +kunit_test_suites(&overflow_test_suite);
> >
> > -module_init(test_module_init);
> > -module_exit(test_module_exit);
> > MODULE_LICENSE("Dual MIT/GPL");
> >
> > base-commit: c63d2dd7e134ebddce4745c51f9572b3f0d92b26
> > --
> > 2.26.2
> >
>
> --
> Kees Cook
In this version I tried to leave as few changes as possible.
It seemed to me that it would be better to leave a smaller diff.
Thanks for the review!
Powered by blists - more mailing lists