| 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: <CADQ6JjWW520V=eGhkP7Qoi7uOq7iL1r_Pk64y=R6DEB8gmfeBw@mail.gmail.com>
Date: Thu, 23 Jul 2020 18:35:45 -0300
From: Vitor Massaru Iha <vitor@...saru.org>
To: Brendan Higgins <brendanhiggins@...gle.com>
Cc: Kees Cook <keescook@...omium.org>,
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>,
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 8:47 PM 'Brendan Higgins' via KUnit
Development <kunit-dev@...glegroups.com> wrote:
>
> On Tue, Jul 21, 2020 at 12:38 PM Vitor Massaru Iha <vitor@...saru.org> wrote:
> >
> > 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.
>
> Yep, that's the pattern that pretty much all other KUnit tests follow.
> Maybe you are right and maybe we should change that convention. Still,
> this is consistent with what we do now.
>
> > >
> > > > 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.
Kees, do you prefer a more detailed result this way for example ?
# Subtest: overflow
1..10
ok 1 - test_u8_overflow
ok 2 - test_s8_overflow
ok 3 - test_u16_overflow
ok 4 - test_s16_overflow
ok 5 - test_u32_overflow
ok 6 - test_s32_overflow
ok 7 - test_u64_overflow
ok 8 - test_s64_overflow
ok 9 - overflow_shift_test
[snip]
> >
> > 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!
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAFd5g468HLqzTVX7cOeHnWWBBmTMKcKPEURSgwiDC-8Hcq_vuw%40mail.gmail.com.
Powered by blists - more mailing lists