Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow
@ 2020-07-20 22:44 Vitor Massaru Iha
  2020-07-21 13:33 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Vitor Massaru Iha @ 2020-07-20 22:44 UTC (permalink / raw)
  To: kunit-dev
  Cc: linux-kselftest, linux-kernel, brendanhiggins, keescook,
	davidgow, skhan, linux-kernel-mentees

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@massaru.org>
Tested-by: David Gow <davidgow@google.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;
  * added expects/asserts on individual tests;
---
 lib/Kconfig.debug                         |  20 +++-
 lib/Makefile                              |   2 +-
 lib/{test_overflow.c => overflow_kunit.c} | 122 +++++++++-------------
 3 files changed, 70 insertions(+), 74 deletions(-)
 rename lib/{test_overflow.c => overflow_kunit.c} (91%)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9ad9210d70a1..230aaf418dc0 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1999,9 +1999,6 @@ config TEST_UUID
 config TEST_XARRAY
 	tristate "Test the XArray code at runtime"

-config TEST_OVERFLOW
-	tristate "Test check_*_overflow() functions at runtime"
-
 config TEST_RHASHTABLE
 	tristate "Perform selftest on resizable hash table"
 	help
@@ -2154,6 +2151,23 @@ config SYSCTL_KUNIT_TEST

 	  If unsure, say N.

+config OVERFLOW_KUNIT
+	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
diff --git a/lib/Makefile b/lib/Makefile
index b1c42c10073b..c3cf72ec6c52 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -75,7 +75,6 @@ obj-$(CONFIG_TEST_LIST_SORT) += test_list_sort.o
 obj-$(CONFIG_TEST_MIN_HEAP) += test_min_heap.o
 obj-$(CONFIG_TEST_LKM) += test_module.o
 obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
-obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
 obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o
 obj-$(CONFIG_TEST_SORT) += test_sort.o
 obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
@@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
 # KUnit tests
 obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
 obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
+obj-$(CONFIG_OVERFLOW_KUNIT) += overflow_kunit.o
diff --git a/lib/test_overflow.c b/lib/overflow_kunit.c
similarity index 91%
rename from lib/test_overflow.c
rename to lib/overflow_kunit.c
index 7a4b6f6c5473..475d0daeb801 100644
--- a/lib/test_overflow.c
+++ b/lib/overflow_kunit.c
@@ -4,14 +4,11 @@
  */
 #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)			\
@@ -248,7 +245,7 @@ static int __init do_test_ ## t(const struct test_ ## t *p)		\
 	return err;							\
 }									\
 									\
-static int __init test_ ## t ## _overflow(void) {			\
+static int __init test_ ## t ## _overflow(struct kunit *test) {	\
 	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;							\
 }

@@ -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);
 }

-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");
-	}
+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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow
  2020-07-20 22:44 [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow Vitor Massaru Iha
@ 2020-07-21 13:33 ` kernel test robot
  2020-07-21 18:55 ` Kees Cook
  2020-07-26 21:10 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2020-07-21 13:33 UTC (permalink / raw)
  To: Vitor Massaru Iha, kunit-dev
  Cc: kbuild-all, linux-kselftest, linux-kernel, brendanhiggins,
	keescook, davidgow, skhan, linux-kernel-mentees


[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

Hi Vitor,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on c63d2dd7e134ebddce4745c51f9572b3f0d92b26]

url:    https://github.com/0day-ci/linux/commits/Vitor-Massaru-Iha/lib-overflow_kunit-add-KUnit-test-conversion-of-check_-_overflow/20200721-064712
base:    c63d2dd7e134ebddce4745c51f9572b3f0d92b26
config: x86_64-randconfig-c002-20200719 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> WARNING: modpost: lib/overflow_kunit.o(.data+0x0): Section mismatch in reference from the variable suites to the variable .init.data:overflow_test_suite
   The variable suites references
   the variable __initdata overflow_test_suite
   If the reference is valid then annotate the
   variable with or __refdata (see linux/init.h) or name the variable:
   

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35460 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow
  2020-07-20 22:44 [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow Vitor Massaru Iha
  2020-07-21 13:33 ` kernel test robot
@ 2020-07-21 18:55 ` Kees Cook
  2020-07-21 19:37   ` Vitor Massaru Iha
  2020-07-26 21:10 ` kernel test robot
  2 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2020-07-21 18:55 UTC (permalink / raw)
  To: Vitor Massaru Iha
  Cc: kunit-dev, linux-kselftest, linux-kernel, brendanhiggins,
	davidgow, skhan, linux-kernel-mentees

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@massaru.org>
> Tested-by: David Gow <davidgow@google.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)

> -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?

>  	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?

> 
> @@ -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)?

>  }
> 
> -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.

> +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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow
  2020-07-21 18:55 ` Kees Cook
@ 2020-07-21 19:37   ` Vitor Massaru Iha
  2020-07-21 23:47     ` Brendan Higgins
  0 siblings, 1 reply; 7+ messages in thread
From: Vitor Massaru Iha @ 2020-07-21 19:37 UTC (permalink / raw)
  To: Kees Cook
  Cc: KUnit Development, open list:KERNEL SELFTEST FRAMEWORK,
	Linux Kernel Mailing List, Brendan Higgins, David Gow,
	Shuah Khan, linux-kernel-mentees

On Tue, Jul 21, 2020 at 3:55 PM Kees Cook <keescook@chromium.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@massaru.org>
> > Tested-by: David Gow <davidgow@google.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!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow
  2020-07-21 19:37   ` Vitor Massaru Iha
@ 2020-07-21 23:47     ` Brendan Higgins
  2020-07-23 21:35       ` Vitor Massaru Iha
  0 siblings, 1 reply; 7+ messages in thread
From: Brendan Higgins @ 2020-07-21 23:47 UTC (permalink / raw)
  To: Vitor Massaru Iha
  Cc: Kees Cook, KUnit Development,
	open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
	David Gow, Shuah Khan, linux-kernel-mentees

On Tue, Jul 21, 2020 at 12:38 PM Vitor Massaru Iha <vitor@massaru.org> wrote:
>
> On Tue, Jul 21, 2020 at 3:55 PM Kees Cook <keescook@chromium.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@massaru.org>
> > > Tested-by: David Gow <davidgow@google.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.
>
> 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!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow
  2020-07-21 23:47     ` Brendan Higgins
@ 2020-07-23 21:35       ` Vitor Massaru Iha
  0 siblings, 0 replies; 7+ messages in thread
From: Vitor Massaru Iha @ 2020-07-23 21:35 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: Kees Cook, KUnit Development,
	open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
	David Gow, Shuah Khan, linux-kernel-mentees

On Tue, Jul 21, 2020 at 8:47 PM 'Brendan Higgins' via KUnit
Development <kunit-dev@googlegroups.com> wrote:
>
> On Tue, Jul 21, 2020 at 12:38 PM Vitor Massaru Iha <vitor@massaru.org> wrote:
> >
> > On Tue, Jul 21, 2020 at 3:55 PM Kees Cook <keescook@chromium.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@massaru.org>
> > > > Tested-by: David Gow <davidgow@google.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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAFd5g468HLqzTVX7cOeHnWWBBmTMKcKPEURSgwiDC-8Hcq_vuw%40mail.gmail.com.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow
  2020-07-20 22:44 [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow Vitor Massaru Iha
  2020-07-21 13:33 ` kernel test robot
  2020-07-21 18:55 ` Kees Cook
@ 2020-07-26 21:10 ` kernel test robot
  2 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2020-07-26 21:10 UTC (permalink / raw)
  To: Vitor Massaru Iha, kunit-dev
  Cc: kbuild-all, linux-kselftest, linux-kernel, brendanhiggins,
	keescook, davidgow, skhan, linux-kernel-mentees


[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

Hi Vitor,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on c63d2dd7e134ebddce4745c51f9572b3f0d92b26]

url:    https://github.com/0day-ci/linux/commits/Vitor-Massaru-Iha/lib-overflow_kunit-add-KUnit-test-conversion-of-check_-_overflow/20200721-064712
base:    c63d2dd7e134ebddce4745c51f9572b3f0d92b26
config: arm-randconfig-r036-20200726 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> WARNING: modpost: vmlinux.o(.data+0x8e3e0): Section mismatch in reference from the variable suites to the (unknown reference) .init.data:(unknown)
   The variable suites references
   the (unknown reference) __initdata (unknown)
   If the reference is valid then annotate the
   variable with or __refdata (see linux/init.h) or name the variable:
   

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34541 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-20 22:44 [PATCH v3] lib: overflow_kunit: add KUnit test conversion of check_*_overflow Vitor Massaru Iha
2020-07-21 13:33 ` kernel test robot
2020-07-21 18:55 ` Kees Cook
2020-07-21 19:37   ` Vitor Massaru Iha
2020-07-21 23:47     ` Brendan Higgins
2020-07-23 21:35       ` Vitor Massaru Iha
2020-07-26 21:10 ` kernel test robot

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git