Re: [PATCH 03/32] flex_array: Add Kunit tests
On Tue, May 3, 2022 at 8:47 PM Kees Cook wrote:
> +#define COMPARE_STRUCTS(STRUCT_A, STRUCT_B)do {\
> + STRUCT_A *ptr_A;\
> + STRUCT_B *ptr_B;\
> + int rc; \
> + size_t size_A, size_B; \
> + \
> + /* matching types for flex array elements and count */ \
> + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A), sizeof(*ptr_B)); \
> + KUNIT_EXPECT_TRUE(test, __same_type(*ptr_A->data, \
> + *ptr_B->__flex_array_elements));\
Leaving some minor suggestions to go along with David's comments.
Should we make these KUNIT_ASSERT_.* instead?
I assume if we have a type-mismatch, then we should bail out instead
of continuing to produce more error messages.
> + KUNIT_EXPECT_TRUE(test, __same_type(ptr_A->datalen, \
> + ptr_B->__flex_array_elements_count)); \
> + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A->data), \
> + sizeof(*ptr_B->__flex_array_elements)); \
> + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), data), \
> + offsetof(typeof(*ptr_B), \
> + __flex_array_elements)); \
> + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), datalen),\
> + offsetof(typeof(*ptr_B), \
> + __flex_array_elements_count)); \
> + \
> + /* struct_size() vs __fas_bytes() */\
> + size_A = struct_size(ptr_A, data, 13); \
> + rc = __fas_bytes(ptr_B, __flex_array_elements, \
> +__flex_array_elements_count, 13, &size_B); \
> + KUNIT_EXPECT_EQ(test, rc, 0); \
Hmm, what do you think about inlining the call/dropping rc?
i.e. something like
KUNIT_EXPECT_EQ(test, 0, __fas_bytes(ptr_B, __flex_array_elements, \
__flex_array_elements_count, 13, &size_B));
That would give a slightly clearer error message on failure.
Otherwise the user only really gets a line number to try and start to
understand what went wrong.
> +
> +#define CHECK_COPY(ptr)do {
> \
> + typeof(*(ptr)) *_cc_dst = (ptr);
> \
> + KUNIT_EXPECT_EQ(test, _cc_dst->induce_padding, 0);
> \
> + memcpy(&padding, &_cc_dst->induce_padding +
> sizeof(_cc_dst->induce_padding), \
> + sizeof(padding));
> \
> + /* Padding should be zero too. */
> \
> + KUNIT_EXPECT_EQ(test, padding, 0);
> \
> + KUNIT_EXPECT_EQ(test, src->count, _cc_dst->count);
> \
This also seems like a good place to use ASSERT instead of EXPECT.
> + KUNIT_EXPECT_EQ(test, _cc_dst->count, TEST_TARGET);
> \
> + for (i = 0; i < _cc_dst->count - 1; i++) {
> \
> + /* 'A' is 0x41, and here repeated in a u32. */
> \
> + KUNIT_EXPECT_EQ(test, _cc_dst->flex[i], 0x41414141);
> \
> + }
> \
> + /* Last item should be different. */
> \
> + KUNIT_EXPECT_EQ(test, _cc_dst->flex[_cc_dst->count - 1], 0x14141414);
> \
> +} while (0)
> +
> +/* Test copying from one flexible array struct into another. */
> +static void flex_cpy_test(struct kunit *test)
> +{
> +#define TEST_BOUNDS13
> +#define TEST_TARGET12
> +#define TEST_SMALL 10
> + struct flex_cpy_obj *src, *dst;
> + unsigned long padding;
> + int i, rc;
> +
> + /* Prepare open-coded source. */
> + src = kzalloc(struct_size(src, flex, TEST_BOUNDS), GFP_KERNEL);
Looks like we could use kunit_kzalloc() here and avoid needing the
manual call to kfree?
This also holds for the other test cases where they don't have early
calls to kfree().
Doing so would also let you use KUNIT_ASSERT's without fear of leaking
these allocations.
> + src->count = TEST_BOUNDS;
> + memset(src->flex, 'A', flex_array_size(src, flex, TEST_BOUNDS));
> + src->flex[src->count - 2] = 0x14141414;
> + src->flex[src->count - 1] = 0x24242424;
> +
> + /* Prepare open-coded destination,
Re: [PATCH 03/32] flex_array: Add Kunit tests
On Wed, May 04, 2022 at 11:00:38AM +0800, David Gow wrote:
> On Wed, May 4, 2022 at 9:47 AM Kees Cook wrote:
> >
> > Add tests for the new flexible array structure helpers. These can be run
> > with:
> >
> > make ARCH=um mrproper
> > ./tools/testing/kunit/kunit.py config
>
> Nit: it shouldn't be necessary to run kunit.py config separately:
> kunit.py run will configure the kernel if necessary.
Ah yes, I think you mentioned this before. I'll adjust the commit log.
>
> > ./tools/testing/kunit/kunit.py run flex_array
> >
> > Cc: David Gow
> > Cc: kunit-...@googlegroups.com
> > Signed-off-by: Kees Cook
> > ---
>
> This looks pretty good to me: it certainly worked on the different
> setups I tried (um, x86_64, x86_64+KASAN).
>
> A few minor nitpicks inline, mostly around minor config-y things, or
> things which weren't totally clear on my first read-through.
>
> Hopefully one day, with the various stubbing features or something
> similar, we'll be able to check against allocation failures in
> flex_dup(), too, but otherwise nothing seems too obviously missing.
>
> Reviewed-by: David Gow
Great; thanks for the review and testing!
>
> -- David
>
> > lib/Kconfig.debug | 12 +-
> > lib/Makefile | 1 +
> > lib/flex_array_kunit.c | 523 +
> > 3 files changed, 531 insertions(+), 5 deletions(-)
> > create mode 100644 lib/flex_array_kunit.c
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 9077bb38bc93..8bae6b169c50 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2551,11 +2551,6 @@ config OVERFLOW_KUNIT_TEST
> > Builds unit tests for the check_*_overflow(), size_*(),
> > allocation, and
> > related functions.
> >
> > - 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.
> > -
>
> Nit: while I'm not against removing some of this boilerplate, is it
> better suited for a separate commit?
Make sense, yes. I'll drop this for now.
>
> > config STACKINIT_KUNIT_TEST
> > tristate "Test level of stack variable initialization" if
> > !KUNIT_ALL_TESTS
> > depends on KUNIT
> > @@ -2567,6 +2562,13 @@ config STACKINIT_KUNIT_TEST
> > CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF,
> > or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
> >
> > +config FLEX_ARRAY_KUNIT_TEST
> > + tristate "Test flex_*() family of helper functions at runtime" if
> > !KUNIT_ALL_TESTS
> > + depends on KUNIT
> > + default KUNIT_ALL_TESTS
> > + help
> > + Builds unit tests for flexible array copy helper functions.
> > +
>
> Nit: checkpatch warns that the description here may be insufficient:
> WARNING: please write a help paragraph that fully describes the config symbol
Yeah, I don't know anything to put here that isn't just more
boilerplate, so I'm choosing to ignore this for now. :)
> > [...]
> > +struct normal {
> > + size_t datalen;
> > + u32 data[];
> > +};
> > +
> > +struct decl_normal {
> > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(size_t, datalen);
> > + DECLARE_FLEX_ARRAY_ELEMENTS(u32, data);
> > +};
> > +
> > +struct aligned {
> > + unsigned short datalen;
> > + chardata[] __aligned(__alignof__(u64));
> > +};
> > +
> > +struct decl_aligned {
> > + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(unsigned short, datalen);
> > + DECLARE_FLEX_ARRAY_ELEMENTS(char, data) __aligned(__alignof__(u64));
> > +};
> > +
> > +static void struct_test(struct kunit *test)
> > +{
> > + COMPARE_STRUCTS(struct normal, struct decl_normal);
> > + COMPARE_STRUCTS(struct aligned, struct decl_aligned);
> > +}
>
> If I understand it, the purpose of this is to ensure that structs both
> with and without the flexible array declaration have the same memory
> layout?
>
> If so, any chance of a comment briefly stating that's the purpose (or
> renaming this test struct_layout_test())?
Yeah, good idea; I'll improve the naming.
>
> Also, would it make sense to do the same with the struct with internal
> padding below?
Heh, yes, good point! :)
> [...]
> > +#define CHECK_COPY(ptr)do {
> > \
> > + typeof(*(ptr)) *_cc_dst = (ptr);
> > \
> > + KUNIT_EXPECT_EQ(test, _cc_dst->induce_padding, 0);
> > \
> > + memcpy(&padding, &_cc_dst->induce_padding +
> > sizeof(_cc_dst->induce_padding), \
> > + sizeof(padding));
> > \
> > + /* Padding should be zero too. */
> > \
> > + KUNIT_EXPECT_EQ(test, padding, 0);
> > \
> > + KUNIT_EXPECT_EQ(test, src->count, _cc_dst->count);
Re: [PATCH 03/32] flex_array: Add Kunit tests
On Wed, May 4, 2022 at 9:47 AM Kees Cook wrote:
>
> Add tests for the new flexible array structure helpers. These can be run
> with:
>
> make ARCH=um mrproper
> ./tools/testing/kunit/kunit.py config
Nit: it shouldn't be necessary to run kunit.py config separately:
kunit.py run will configure the kernel if necessary.
> ./tools/testing/kunit/kunit.py run flex_array
>
> Cc: David Gow
> Cc: kunit-...@googlegroups.com
> Signed-off-by: Kees Cook
> ---
This looks pretty good to me: it certainly worked on the different
setups I tried (um, x86_64, x86_64+KASAN).
A few minor nitpicks inline, mostly around minor config-y things, or
things which weren't totally clear on my first read-through.
Hopefully one day, with the various stubbing features or something
similar, we'll be able to check against allocation failures in
flex_dup(), too, but otherwise nothing seems too obviously missing.
Reviewed-by: David Gow
-- David
> lib/Kconfig.debug | 12 +-
> lib/Makefile | 1 +
> lib/flex_array_kunit.c | 523 +
> 3 files changed, 531 insertions(+), 5 deletions(-)
> create mode 100644 lib/flex_array_kunit.c
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 9077bb38bc93..8bae6b169c50 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2551,11 +2551,6 @@ config OVERFLOW_KUNIT_TEST
> Builds unit tests for the check_*_overflow(), size_*(), allocation,
> and
> related functions.
>
> - 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.
> -
Nit: while I'm not against removing some of this boilerplate, is it
better suited for a separate commit?
> config STACKINIT_KUNIT_TEST
> tristate "Test level of stack variable initialization" if
> !KUNIT_ALL_TESTS
> depends on KUNIT
> @@ -2567,6 +2562,13 @@ config STACKINIT_KUNIT_TEST
> CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF,
> or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
>
> +config FLEX_ARRAY_KUNIT_TEST
> + tristate "Test flex_*() family of helper functions at runtime" if
> !KUNIT_ALL_TESTS
> + depends on KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + Builds unit tests for flexible array copy helper functions.
> +
Nit: checkpatch warns that the description here may be insufficient:
WARNING: please write a help paragraph that fully describes the config symbol
> config TEST_UDELAY
> tristate "udelay test driver"
> help
> diff --git a/lib/Makefile b/lib/Makefile
> index 6b9ffc1bd1ee..9884318db330 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -366,6 +366,7 @@ obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o
> obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
> CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
> obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
> +obj-$(CONFIG_FLEX_ARRAY_KUNIT_TEST) += flex_array_kunit.o
>
> obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
>
> diff --git a/lib/flex_array_kunit.c b/lib/flex_array_kunit.c
> new file mode 100644
> index ..48bee88945b4
> --- /dev/null
> +++ b/lib/flex_array_kunit.c
> @@ -0,0 +1,523 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test cases for flex_*() array manipulation helpers.
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define COMPARE_STRUCTS(STRUCT_A, STRUCT_B)do {\
> + STRUCT_A *ptr_A;\
> + STRUCT_B *ptr_B;\
> + int rc; \
> + size_t size_A, size_B; \
> + \
> + /* matching types for flex array elements and count */ \
> + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A), sizeof(*ptr_B)); \
> + KUNIT_EXPECT_TRUE(test, __same_type(*ptr_A->data, \
> + *ptr_B->__flex_array_elements));\
> + KUNIT_EXPECT_TRUE(test, __same_type(ptr_A->datalen, \
> + ptr_B->__flex_array_elements_count)); \
> + KUNIT_EXPECT_EQ(test, sizeof(*ptr_A->data), \
> + sizeof(*ptr_B->__flex_array_elements)); \
> + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), data), \
> + offsetof(typeof(*ptr_B), \
> + __flex_array_elements)); \
> + KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), datalen),\
> + offsetof(typ
[PATCH 03/32] flex_array: Add Kunit tests
Add tests for the new flexible array structure helpers. These can be run
with:
make ARCH=um mrproper
./tools/testing/kunit/kunit.py config
./tools/testing/kunit/kunit.py run flex_array
Cc: David Gow
Cc: kunit-...@googlegroups.com
Signed-off-by: Kees Cook
---
lib/Kconfig.debug | 12 +-
lib/Makefile | 1 +
lib/flex_array_kunit.c | 523 +
3 files changed, 531 insertions(+), 5 deletions(-)
create mode 100644 lib/flex_array_kunit.c
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9077bb38bc93..8bae6b169c50 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2551,11 +2551,6 @@ config OVERFLOW_KUNIT_TEST
Builds unit tests for the check_*_overflow(), size_*(), allocation,
and
related functions.
- 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 STACKINIT_KUNIT_TEST
tristate "Test level of stack variable initialization" if
!KUNIT_ALL_TESTS
depends on KUNIT
@@ -2567,6 +2562,13 @@ config STACKINIT_KUNIT_TEST
CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF,
or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL.
+config FLEX_ARRAY_KUNIT_TEST
+ tristate "Test flex_*() family of helper functions at runtime" if
!KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ Builds unit tests for flexible array copy helper functions.
+
config TEST_UDELAY
tristate "udelay test driver"
help
diff --git a/lib/Makefile b/lib/Makefile
index 6b9ffc1bd1ee..9884318db330 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -366,6 +366,7 @@ obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o
obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable)
obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o
+obj-$(CONFIG_FLEX_ARRAY_KUNIT_TEST) += flex_array_kunit.o
obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
diff --git a/lib/flex_array_kunit.c b/lib/flex_array_kunit.c
new file mode 100644
index ..48bee88945b4
--- /dev/null
+++ b/lib/flex_array_kunit.c
@@ -0,0 +1,523 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test cases for flex_*() array manipulation helpers.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+
+#define COMPARE_STRUCTS(STRUCT_A, STRUCT_B)do {\
+ STRUCT_A *ptr_A;\
+ STRUCT_B *ptr_B;\
+ int rc; \
+ size_t size_A, size_B; \
+ \
+ /* matching types for flex array elements and count */ \
+ KUNIT_EXPECT_EQ(test, sizeof(*ptr_A), sizeof(*ptr_B)); \
+ KUNIT_EXPECT_TRUE(test, __same_type(*ptr_A->data, \
+ *ptr_B->__flex_array_elements));\
+ KUNIT_EXPECT_TRUE(test, __same_type(ptr_A->datalen, \
+ ptr_B->__flex_array_elements_count)); \
+ KUNIT_EXPECT_EQ(test, sizeof(*ptr_A->data), \
+ sizeof(*ptr_B->__flex_array_elements)); \
+ KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), data), \
+ offsetof(typeof(*ptr_B), \
+ __flex_array_elements)); \
+ KUNIT_EXPECT_EQ(test, offsetof(typeof(*ptr_A), datalen),\
+ offsetof(typeof(*ptr_B), \
+ __flex_array_elements_count)); \
+ \
+ /* struct_size() vs __fas_bytes() */\
+ size_A = struct_size(ptr_A, data, 13); \
+ rc = __fas_bytes(ptr_B, __flex_array_elements, \
+__flex_array_elements_count, 13, &size_B); \
+ KUNIT_EXPECT_EQ(test, rc, 0); \
+ KUNIT_EXPECT_EQ(test, size_A, size_B); \
+ \
+ /* flex_array_size() vs __fas_elements_bytes() */ \
+ size_A = flex_array_size(ptr_A, data, 13); \
+ rc = __fas_elements_bytes(ptr_B, __flex_array_elements, \
+__flex_array_elements_count, 13, &size_B); \
+ KUNIT_EXPECT_EQ(test, rc, 0); \
+ KUNIT_EXPECT_EQ(
