* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-07 4:02 [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST Kees Cook
@ 2023-01-07 4:36 ` Guenter Roeck
2023-01-07 10:55 ` Geert Uytterhoeven
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2023-01-07 4:36 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Nathan Chancellor, linux-hardening, David Gow,
Nick Desaulniers, Josh Poimboeuf, Geert Uytterhoeven,
Miguel Ojeda, Isabella Basso, Vlastimil Babka, Dan Williams,
Rasmus Villemoes, linux-kernel
On Fri, Jan 06, 2023 at 08:02:07PM -0800, Kees Cook wrote:
> Since the long memcpy tests may stall a system for tens of seconds
> in virtualized architecture environments, split those tests off under
> CONFIG_MEMCPY_SLOW_KUNIT_TEST so they can be separately disabled.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/lkml/20221226195206.GA2626419@roeck-us.net
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Guenter, does this give you the needed flexibility to turn on the memcpy
> kunit tests again in your slower environments?
I'll be traveling this weekend. Give me until early next week to give
it a try.
Thanks,
Guenter
> ---
> lib/Kconfig.debug | 9 +++++++++
> lib/memcpy_kunit.c | 17 +++++++++++++----
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index c2c78d0e761c..b5e94807f41c 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2621,6 +2621,15 @@ config MEMCPY_KUNIT_TEST
>
> If unsure, say N.
>
> +config MEMCPY_SLOW_KUNIT_TEST
> + tristate "Include exhaustive memcpy tests" if !KUNIT_ALL_TESTS
> + depends on MEMCPY_KUNIT_TEST
> + default KUNIT_ALL_TESTS
> + help
> + Some memcpy tests are quite exhaustive in checking for overlaps
> + and bit ranges. These can be very slow, so they are split out
> + as a separate config.
> +
> config IS_SIGNED_TYPE_KUNIT_TEST
> tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 89128551448d..cc1f36335a9b 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -307,8 +307,12 @@ static void set_random_nonzero(struct kunit *test, u8 *byte)
> }
> }
>
> -static void init_large(struct kunit *test)
> +static int init_large(struct kunit *test)
> {
> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
> + return -EBUSY;
> + }
>
> /* Get many bit patterns. */
> get_random_bytes(large_src, ARRAY_SIZE(large_src));
> @@ -319,6 +323,8 @@ static void init_large(struct kunit *test)
>
> /* Explicitly zero the entire destination. */
> memset(large_dst, 0, ARRAY_SIZE(large_dst));
> +
> + return 0;
> }
>
> /*
> @@ -327,7 +333,9 @@ static void init_large(struct kunit *test)
> */
> static void copy_large_test(struct kunit *test, bool use_memmove)
> {
> - init_large(test);
> +
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of non-overlapping bytes ... */
> for (int bytes = 1; bytes <= ARRAY_SIZE(large_src); bytes++) {
> @@ -472,7 +480,8 @@ static void memmove_overlap_test(struct kunit *test)
> static const int bytes_start = 1;
> static const int bytes_end = ARRAY_SIZE(large_src) + 1;
>
> - init_large(test);
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of overlapping bytes ... */
> for (int bytes = bytes_start; bytes < bytes_end;
> @@ -549,8 +558,8 @@ static void strtomem_test(struct kunit *test)
> static struct kunit_case memcpy_test_cases[] = {
> KUNIT_CASE(memset_test),
> KUNIT_CASE(memcpy_test),
> - KUNIT_CASE(memcpy_large_test),
> KUNIT_CASE(memmove_test),
> + KUNIT_CASE(memcpy_large_test),
> KUNIT_CASE(memmove_large_test),
> KUNIT_CASE(memmove_overlap_test),
> KUNIT_CASE(strtomem_test),
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-07 4:02 [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST Kees Cook
2023-01-07 4:36 ` Guenter Roeck
@ 2023-01-07 10:55 ` Geert Uytterhoeven
2023-01-10 7:51 ` Vlastimil Babka
2023-01-09 23:49 ` Nick Desaulniers
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-01-07 10:55 UTC (permalink / raw)
To: Kees Cook
Cc: Guenter Roeck, Andrew Morton, Nathan Chancellor, linux-hardening,
David Gow, Nick Desaulniers, Josh Poimboeuf, Miguel Ojeda,
Isabella Basso, Vlastimil Babka, Dan Williams, Rasmus Villemoes,
linux-kernel
Hi Kees,
On Sat, Jan 7, 2023 at 5:02 AM Kees Cook <keescook@chromium.org> wrote:
> Since the long memcpy tests may stall a system for tens of seconds
> in virtualized architecture environments, split those tests off under
> CONFIG_MEMCPY_SLOW_KUNIT_TEST so they can be separately disabled.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/lkml/20221226195206.GA2626419@roeck-us.net
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Thanks for your patch!
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2621,6 +2621,15 @@ config MEMCPY_KUNIT_TEST
>
> If unsure, say N.
>
> +config MEMCPY_SLOW_KUNIT_TEST
> + tristate "Include exhaustive memcpy tests" if !KUNIT_ALL_TESTS
Why the tristate?
> + depends on MEMCPY_KUNIT_TEST
> + default KUNIT_ALL_TESTS
> + help
> + Some memcpy tests are quite exhaustive in checking for overlaps
> + and bit ranges. These can be very slow, so they are split out
> + as a separate config.
> +
> config IS_SIGNED_TYPE_KUNIT_TEST
> tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 89128551448d..cc1f36335a9b 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -307,8 +307,12 @@ static void set_random_nonzero(struct kunit *test, u8 *byte)
> }
> }
>
> -static void init_large(struct kunit *test)
> +static int init_large(struct kunit *test)
> {
> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
So I can't make the slower tests available for when I need them,
but not run them by default?
I guess that's why you made MEMCPY_SLOW_KUNIT_TEST tristate originally,
to have a separate module with the slow tests?
> + return -EBUSY;
> + }
>
> /* Get many bit patterns. */
> get_random_bytes(large_src, ARRAY_SIZE(large_src));
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-07 10:55 ` Geert Uytterhoeven
@ 2023-01-10 7:51 ` Vlastimil Babka
2023-01-10 20:38 ` Daniel Latypov
0 siblings, 1 reply; 12+ messages in thread
From: Vlastimil Babka @ 2023-01-10 7:51 UTC (permalink / raw)
To: Geert Uytterhoeven, Kees Cook
Cc: Guenter Roeck, Andrew Morton, Nathan Chancellor, linux-hardening,
David Gow, Nick Desaulniers, Josh Poimboeuf, Miguel Ojeda,
Isabella Basso, Dan Williams, Rasmus Villemoes, linux-kernel,
Brendan Higgins, open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development
+Cc rest of kunit from MAINTAINERS
On 1/7/23 11:55, Geert Uytterhoeven wrote:
> Hi Kees,
>
> On Sat, Jan 7, 2023 at 5:02 AM Kees Cook <keescook@chromium.org> wrote:
>> Since the long memcpy tests may stall a system for tens of seconds
>> in virtualized architecture environments, split those tests off under
>> CONFIG_MEMCPY_SLOW_KUNIT_TEST so they can be separately disabled.
>>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Link: https://lore.kernel.org/lkml/20221226195206.GA2626419@roeck-us.net
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Nathan Chancellor <nathan@kernel.org>
>> Cc: linux-hardening@vger.kernel.org
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Thanks for your patch!
>
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -2621,6 +2621,15 @@ config MEMCPY_KUNIT_TEST
>>
>> If unsure, say N.
>>
>> +config MEMCPY_SLOW_KUNIT_TEST
>> + tristate "Include exhaustive memcpy tests" if !KUNIT_ALL_TESTS
>
> Why the tristate?
>
>> + depends on MEMCPY_KUNIT_TEST
>> + default KUNIT_ALL_TESTS
>> + help
>> + Some memcpy tests are quite exhaustive in checking for overlaps
>> + and bit ranges. These can be very slow, so they are split out
>> + as a separate config.
>> +
>> config IS_SIGNED_TYPE_KUNIT_TEST
>> tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
>> depends on KUNIT
>> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
>> index 89128551448d..cc1f36335a9b 100644
>> --- a/lib/memcpy_kunit.c
>> +++ b/lib/memcpy_kunit.c
>> @@ -307,8 +307,12 @@ static void set_random_nonzero(struct kunit *test, u8 *byte)
>> }
>> }
>>
>> -static void init_large(struct kunit *test)
>> +static int init_large(struct kunit *test)
>> {
>> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
>> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
>
> So I can't make the slower tests available for when I need them,
> but not run them by default?
Indeed it seems weird to tie this to a config option without runtime override.
> I guess that's why you made MEMCPY_SLOW_KUNIT_TEST tristate originally,
> to have a separate module with the slow tests?
On the other hand I can imagine requiring a separate module for slow tests
would lead to more churn - IIUC there would need to be two files instead of
memcpy_kunit.c, possibly a duplicated boilerplate code (or another shared .c
file).
So the idea is to have a generic way to mark some tests as slow and a way to
opt-in/opt-out for those when running the tests. Maybe KUnit folks already
have such mechanism or have an idea how to implement that.
>> + return -EBUSY;
>> + }
>>
>> /* Get many bit patterns. */
>> get_random_bytes(large_src, ARRAY_SIZE(large_src));
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-10 7:51 ` Vlastimil Babka
@ 2023-01-10 20:38 ` Daniel Latypov
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Latypov @ 2023-01-10 20:38 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Geert Uytterhoeven, Kees Cook, Guenter Roeck, Andrew Morton,
Nathan Chancellor, linux-hardening, David Gow, Nick Desaulniers,
Josh Poimboeuf, Miguel Ojeda, Isabella Basso, Dan Williams,
Rasmus Villemoes, linux-kernel, Brendan Higgins,
open list:KERNEL SELFTEST FRAMEWORK, KUnit Development
On Mon, Jan 9, 2023 at 11:51 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> +Cc rest of kunit from MAINTAINERS
>
> On 1/7/23 11:55, Geert Uytterhoeven wrote:
> > Hi Kees,
> >
> > On Sat, Jan 7, 2023 at 5:02 AM Kees Cook <keescook@chromium.org> wrote:
> >> Since the long memcpy tests may stall a system for tens of seconds
> >> in virtualized architecture environments, split those tests off under
> >> CONFIG_MEMCPY_SLOW_KUNIT_TEST so they can be separately disabled.
<snip>
> >>
> >> -static void init_large(struct kunit *test)
> >> +static int init_large(struct kunit *test)
> >> {
> >> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
> >> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
> >
> > So I can't make the slower tests available for when I need them,
> > but not run them by default?
>
> Indeed it seems weird to tie this to a config option without runtime override.
>
> > I guess that's why you made MEMCPY_SLOW_KUNIT_TEST tristate originally,
> > to have a separate module with the slow tests?
>
> On the other hand I can imagine requiring a separate module for slow tests
> would lead to more churn - IIUC there would need to be two files instead of
> memcpy_kunit.c, possibly a duplicated boilerplate code (or another shared .c
> file).
>
> So the idea is to have a generic way to mark some tests as slow and a way to
> opt-in/opt-out for those when running the tests. Maybe KUnit folks already
> have such mechanism or have an idea how to implement that.
There is no mechanism atm, and we'd still need to figure it out so
it'll be a while.
So I think a patch like this makes sense in the short-term.
This is definitely something we've always thought would be useful eventually.
See this TODO which has been there since day 1 ;)
https://elixir.bootlin.com/linux/latest/source/lib/kunit/try-catch.c#L36
It just felt like it would be premature to come up with something when
basically all the tests up until now ran ~instantly.
Throwing out some rough implementation ideas:
I was thinking the granularity for these timeout annotations would be
at the suite-level.
If we go with that, then I guess the intended flow is to group slow
tests into their own suite and mark them as such.
Then maybe we'd have some runtime way of disabling/enabling "long"
tests, like a cmdline opt.
E.g. you'd pass `kunit.max_test_size=30` to exclude tests longer than
30 seconds.
Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-07 4:02 [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST Kees Cook
2023-01-07 4:36 ` Guenter Roeck
2023-01-07 10:55 ` Geert Uytterhoeven
@ 2023-01-09 23:49 ` Nick Desaulniers
2023-01-10 0:11 ` Guenter Roeck
2023-01-11 7:18 ` David Gow
4 siblings, 0 replies; 12+ messages in thread
From: Nick Desaulniers @ 2023-01-09 23:49 UTC (permalink / raw)
To: Kees Cook
Cc: Guenter Roeck, Andrew Morton, Nathan Chancellor, linux-hardening,
David Gow, Josh Poimboeuf, Geert Uytterhoeven, Miguel Ojeda,
Isabella Basso, Vlastimil Babka, Dan Williams, Rasmus Villemoes,
linux-kernel
On Fri, Jan 6, 2023 at 8:02 PM Kees Cook <keescook@chromium.org> wrote:
>
> Since the long memcpy tests may stall a system for tens of seconds
> in virtualized architecture environments, split those tests off under
> CONFIG_MEMCPY_SLOW_KUNIT_TEST so they can be separately disabled.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/lkml/20221226195206.GA2626419@roeck-us.net
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Guenter, does this give you the needed flexibility to turn on the memcpy
> kunit tests again in your slower environments?
> ---
> lib/Kconfig.debug | 9 +++++++++
> lib/memcpy_kunit.c | 17 +++++++++++++----
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index c2c78d0e761c..b5e94807f41c 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2621,6 +2621,15 @@ config MEMCPY_KUNIT_TEST
>
> If unsure, say N.
>
> +config MEMCPY_SLOW_KUNIT_TEST
> + tristate "Include exhaustive memcpy tests" if !KUNIT_ALL_TESTS
> + depends on MEMCPY_KUNIT_TEST
> + default KUNIT_ALL_TESTS
> + help
> + Some memcpy tests are quite exhaustive in checking for overlaps
> + and bit ranges. These can be very slow, so they are split out
> + as a separate config.
> +
> config IS_SIGNED_TYPE_KUNIT_TEST
> tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 89128551448d..cc1f36335a9b 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -307,8 +307,12 @@ static void set_random_nonzero(struct kunit *test, u8 *byte)
> }
> }
>
> -static void init_large(struct kunit *test)
> +static int init_large(struct kunit *test)
> {
> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
> + return -EBUSY;
> + }
>
> /* Get many bit patterns. */
> get_random_bytes(large_src, ARRAY_SIZE(large_src));
> @@ -319,6 +323,8 @@ static void init_large(struct kunit *test)
>
> /* Explicitly zero the entire destination. */
> memset(large_dst, 0, ARRAY_SIZE(large_dst));
> +
> + return 0;
> }
>
> /*
> @@ -327,7 +333,9 @@ static void init_large(struct kunit *test)
> */
> static void copy_large_test(struct kunit *test, bool use_memmove)
> {
> - init_large(test);
> +
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of non-overlapping bytes ... */
> for (int bytes = 1; bytes <= ARRAY_SIZE(large_src); bytes++) {
> @@ -472,7 +480,8 @@ static void memmove_overlap_test(struct kunit *test)
> static const int bytes_start = 1;
> static const int bytes_end = ARRAY_SIZE(large_src) + 1;
>
> - init_large(test);
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of overlapping bytes ... */
> for (int bytes = bytes_start; bytes < bytes_end;
> @@ -549,8 +558,8 @@ static void strtomem_test(struct kunit *test)
> static struct kunit_case memcpy_test_cases[] = {
> KUNIT_CASE(memset_test),
> KUNIT_CASE(memcpy_test),
> - KUNIT_CASE(memcpy_large_test),
> KUNIT_CASE(memmove_test),
> + KUNIT_CASE(memcpy_large_test),
> KUNIT_CASE(memmove_large_test),
> KUNIT_CASE(memmove_overlap_test),
> KUNIT_CASE(strtomem_test),
> --
> 2.34.1
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-07 4:02 [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST Kees Cook
` (2 preceding siblings ...)
2023-01-09 23:49 ` Nick Desaulniers
@ 2023-01-10 0:11 ` Guenter Roeck
2023-01-11 7:18 ` David Gow
4 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2023-01-10 0:11 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Nathan Chancellor, linux-hardening, David Gow,
Nick Desaulniers, Josh Poimboeuf, Geert Uytterhoeven,
Miguel Ojeda, Isabella Basso, Vlastimil Babka, Dan Williams,
Rasmus Villemoes, linux-kernel
On Fri, Jan 06, 2023 at 08:02:07PM -0800, Kees Cook wrote:
> Since the long memcpy tests may stall a system for tens of seconds
> in virtualized architecture environments, split those tests off under
> CONFIG_MEMCPY_SLOW_KUNIT_TEST so they can be separately disabled.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/lkml/20221226195206.GA2626419@roeck-us.net
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Guenter, does this give you the needed flexibility to turn on the memcpy
> kunit tests again in your slower environments?
Yes, it does, and it works.
Reviewed-and-tested-by: Guenter Roeck <linux@roeck-us.net>
> ---
> lib/Kconfig.debug | 9 +++++++++
> lib/memcpy_kunit.c | 17 +++++++++++++----
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index c2c78d0e761c..b5e94807f41c 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2621,6 +2621,15 @@ config MEMCPY_KUNIT_TEST
>
> If unsure, say N.
>
> +config MEMCPY_SLOW_KUNIT_TEST
> + tristate "Include exhaustive memcpy tests" if !KUNIT_ALL_TESTS
As Geert noticed, bool should be sufficient here since this is not a
separate test.
Thanks,
Guenter
> + depends on MEMCPY_KUNIT_TEST
> + default KUNIT_ALL_TESTS
> + help
> + Some memcpy tests are quite exhaustive in checking for overlaps
> + and bit ranges. These can be very slow, so they are split out
> + as a separate config.
> +
> config IS_SIGNED_TYPE_KUNIT_TEST
> tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 89128551448d..cc1f36335a9b 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -307,8 +307,12 @@ static void set_random_nonzero(struct kunit *test, u8 *byte)
> }
> }
>
> -static void init_large(struct kunit *test)
> +static int init_large(struct kunit *test)
> {
> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
> + return -EBUSY;
> + }
>
> /* Get many bit patterns. */
> get_random_bytes(large_src, ARRAY_SIZE(large_src));
> @@ -319,6 +323,8 @@ static void init_large(struct kunit *test)
>
> /* Explicitly zero the entire destination. */
> memset(large_dst, 0, ARRAY_SIZE(large_dst));
> +
> + return 0;
> }
>
> /*
> @@ -327,7 +333,9 @@ static void init_large(struct kunit *test)
> */
> static void copy_large_test(struct kunit *test, bool use_memmove)
> {
> - init_large(test);
> +
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of non-overlapping bytes ... */
> for (int bytes = 1; bytes <= ARRAY_SIZE(large_src); bytes++) {
> @@ -472,7 +480,8 @@ static void memmove_overlap_test(struct kunit *test)
> static const int bytes_start = 1;
> static const int bytes_end = ARRAY_SIZE(large_src) + 1;
>
> - init_large(test);
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of overlapping bytes ... */
> for (int bytes = bytes_start; bytes < bytes_end;
> @@ -549,8 +558,8 @@ static void strtomem_test(struct kunit *test)
> static struct kunit_case memcpy_test_cases[] = {
> KUNIT_CASE(memset_test),
> KUNIT_CASE(memcpy_test),
> - KUNIT_CASE(memcpy_large_test),
> KUNIT_CASE(memmove_test),
> + KUNIT_CASE(memcpy_large_test),
> KUNIT_CASE(memmove_large_test),
> KUNIT_CASE(memmove_overlap_test),
> KUNIT_CASE(strtomem_test),
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-07 4:02 [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST Kees Cook
` (3 preceding siblings ...)
2023-01-10 0:11 ` Guenter Roeck
@ 2023-01-11 7:18 ` David Gow
4 siblings, 0 replies; 12+ messages in thread
From: David Gow @ 2023-01-11 7:18 UTC (permalink / raw)
To: Kees Cook
Cc: Guenter Roeck, Andrew Morton, Nathan Chancellor, linux-hardening,
Nick Desaulniers, Josh Poimboeuf, Geert Uytterhoeven,
Miguel Ojeda, Isabella Basso, Vlastimil Babka, Dan Williams,
Rasmus Villemoes, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4957 bytes --]
On Sat, 7 Jan 2023 at 12:02, Kees Cook <keescook@chromium.org> wrote:
>
> Since the long memcpy tests may stall a system for tens of seconds
> in virtualized architecture environments, split those tests off under
> CONFIG_MEMCPY_SLOW_KUNIT_TEST so they can be separately disabled.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/lkml/20221226195206.GA2626419@roeck-us.net
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Guenter, does this give you the needed flexibility to turn on the memcpy
> kunit tests again in your slower environments?
> ---
Sorry for the delay: I've just got back from holidays.
This looks good to me (modulo the tristate/bool issue mentioned).
Having a specific way to tag tests as "slow" is something we've
considered having in KUnit for a while, but (at least initially),
there weren't any tests slow enough for it to be worthwhile. It looks
like that's changing (alongside this, there are a few DRM tests which
are quite slow, as well as some of the KCSAN tests. Even the
time64_to_tm_test_date_range test, while very quick under UML on a
modern system, is noticeably slow under qemu and times out on
something like an old 486...). Daniel's comments elsewhere in this
thread have some good ideas.
That being said, it'll probably take long enough to work out and
implement a good way of giving tests "properties" and requesting only
fast tests run that just putting tests which are slow enough to cause
problems behind a kconfig entry seems a pretty solid intermediate
solution.
So this is:
Reviewed-by: David Gow <davidgow@google.com>
(assuming the tristate goes to bool), and I'll look into getting a way
of marking tests as "slow" and enabling/disabling them at runtime.
Cheers,
-- David
> lib/Kconfig.debug | 9 +++++++++
> lib/memcpy_kunit.c | 17 +++++++++++++----
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index c2c78d0e761c..b5e94807f41c 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2621,6 +2621,15 @@ config MEMCPY_KUNIT_TEST
>
> If unsure, say N.
>
> +config MEMCPY_SLOW_KUNIT_TEST
> + tristate "Include exhaustive memcpy tests" if !KUNIT_ALL_TESTS
As mentioned, if this is just going to be a toggle, it can be a "bool".
> + depends on MEMCPY_KUNIT_TEST
> + default KUNIT_ALL_TESTS
> + help
> + Some memcpy tests are quite exhaustive in checking for overlaps
> + and bit ranges. These can be very slow, so they are split out
> + as a separate config.
> +
> config IS_SIGNED_TYPE_KUNIT_TEST
> tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 89128551448d..cc1f36335a9b 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -307,8 +307,12 @@ static void set_random_nonzero(struct kunit *test, u8 *byte)
> }
> }
>
> -static void init_large(struct kunit *test)
> +static int init_large(struct kunit *test)
> {
> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
> + return -EBUSY;
> + }
>
> /* Get many bit patterns. */
> get_random_bytes(large_src, ARRAY_SIZE(large_src));
> @@ -319,6 +323,8 @@ static void init_large(struct kunit *test)
>
> /* Explicitly zero the entire destination. */
> memset(large_dst, 0, ARRAY_SIZE(large_dst));
> +
> + return 0;
> }
>
> /*
> @@ -327,7 +333,9 @@ static void init_large(struct kunit *test)
> */
> static void copy_large_test(struct kunit *test, bool use_memmove)
> {
> - init_large(test);
> +
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of non-overlapping bytes ... */
> for (int bytes = 1; bytes <= ARRAY_SIZE(large_src); bytes++) {
> @@ -472,7 +480,8 @@ static void memmove_overlap_test(struct kunit *test)
> static const int bytes_start = 1;
> static const int bytes_end = ARRAY_SIZE(large_src) + 1;
>
> - init_large(test);
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of overlapping bytes ... */
> for (int bytes = bytes_start; bytes < bytes_end;
> @@ -549,8 +558,8 @@ static void strtomem_test(struct kunit *test)
> static struct kunit_case memcpy_test_cases[] = {
> KUNIT_CASE(memset_test),
> KUNIT_CASE(memcpy_test),
> - KUNIT_CASE(memcpy_large_test),
> KUNIT_CASE(memmove_test),
> + KUNIT_CASE(memcpy_large_test),
> KUNIT_CASE(memmove_large_test),
> KUNIT_CASE(memmove_overlap_test),
> KUNIT_CASE(strtomem_test),
> --
> 2.34.1
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-14 0:54 Kees Cook
@ 2023-01-14 0:57 ` Kees Cook
2023-01-14 1:38 ` Daniel Latypov
2023-01-14 4:31 ` David Gow
2 siblings, 0 replies; 12+ messages in thread
From: Kees Cook @ 2023-01-14 0:57 UTC (permalink / raw)
To: Guenter Roeck
Cc: Nick Desaulniers, Andrew Morton, David Gow, Nathan Chancellor,
linux-hardening, Vlastimil Babka, Daniel Latypov, Josh Poimboeuf,
Geert Uytterhoeven, Miguel Ojeda, Isabella Basso, Dan Williams,
Rasmus Villemoes, linux-kernel
gah. this is actually v2. :(
On Fri, Jan 13, 2023 at 04:54:12PM -0800, Kees Cook wrote:
> Since the long memcpy tests may stall a system for tens of seconds
> in virtualized architecture environments, split those tests off under
> CONFIG_MEMCPY_SLOW_KUNIT_TEST so they can be separately disabled.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/lkml/20221226195206.GA2626419@roeck-us.net
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Reviewed-and-tested-by: Guenter Roeck <linux@roeck-us.net>
> Reviewed-by: David Gow <davidgow@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v2: fix tristate to bool
> v1: https://lore.kernel.org/lkml/20230107040203.never.112-kees@kernel.org
> ---
> lib/Kconfig.debug | 9 +++++++++
> lib/memcpy_kunit.c | 15 ++++++++++++---
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index c2c78d0e761c..f90637171453 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2621,6 +2621,15 @@ config MEMCPY_KUNIT_TEST
>
> If unsure, say N.
>
> +config MEMCPY_SLOW_KUNIT_TEST
> + bool "Include exhaustive memcpy tests" if !KUNIT_ALL_TESTS
> + depends on MEMCPY_KUNIT_TEST
> + default KUNIT_ALL_TESTS
> + help
> + Some memcpy tests are quite exhaustive in checking for overlaps
> + and bit ranges. These can be very slow, so they are split out
> + as a separate config.
> +
> config IS_SIGNED_TYPE_KUNIT_TEST
> tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 89128551448d..5a545e1b5dbb 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -307,8 +307,12 @@ static void set_random_nonzero(struct kunit *test, u8 *byte)
> }
> }
>
> -static void init_large(struct kunit *test)
> +static int init_large(struct kunit *test)
> {
> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
> + return -EBUSY;
> + }
>
> /* Get many bit patterns. */
> get_random_bytes(large_src, ARRAY_SIZE(large_src));
> @@ -319,6 +323,8 @@ static void init_large(struct kunit *test)
>
> /* Explicitly zero the entire destination. */
> memset(large_dst, 0, ARRAY_SIZE(large_dst));
> +
> + return 0;
> }
>
> /*
> @@ -327,7 +333,9 @@ static void init_large(struct kunit *test)
> */
> static void copy_large_test(struct kunit *test, bool use_memmove)
> {
> - init_large(test);
> +
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of non-overlapping bytes ... */
> for (int bytes = 1; bytes <= ARRAY_SIZE(large_src); bytes++) {
> @@ -472,7 +480,8 @@ static void memmove_overlap_test(struct kunit *test)
> static const int bytes_start = 1;
> static const int bytes_end = ARRAY_SIZE(large_src) + 1;
>
> - init_large(test);
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of overlapping bytes ... */
> for (int bytes = bytes_start; bytes < bytes_end;
> --
> 2.34.1
>
--
Kees Cook
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-14 0:54 Kees Cook
2023-01-14 0:57 ` Kees Cook
@ 2023-01-14 1:38 ` Daniel Latypov
2023-01-14 4:31 ` David Gow
2 siblings, 0 replies; 12+ messages in thread
From: Daniel Latypov @ 2023-01-14 1:38 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Guenter Roeck, Nick Desaulniers, David Gow,
Nathan Chancellor, linux-hardening, Vlastimil Babka,
Josh Poimboeuf, Geert Uytterhoeven, Miguel Ojeda, Isabella Basso,
Dan Williams, Rasmus Villemoes, linux-kernel
On Fri, Jan 13, 2023 at 4:54 PM Kees Cook <keescook@chromium.org> wrote:
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 89128551448d..5a545e1b5dbb 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -307,8 +307,12 @@ static void set_random_nonzero(struct kunit *test, u8 *byte)
> }
> }
>
> -static void init_large(struct kunit *test)
> +static int init_large(struct kunit *test)
> {
> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
> + return -EBUSY;
Note: kunit_skip() here means you don't need explicit returns in the test cases.
kunit_skip() is basically
kunit_mark_skipped(test, "reason");
kthread_complete_and_exit(...);
So the diff in this file could be reduced down to just these 2 lines
if (!IS_ENABLED(...))
kunit_skip(test, "...")
But I can see the appeal of being more explicit about the control flow.
In that case, you could switch kunit_mark_skipped(), which just sets
the status and doesn't affect control flow at all.
Daniel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] kunit: memcpy: Split slow memcpy tests into MEMCPY_SLOW_KUNIT_TEST
2023-01-14 0:54 Kees Cook
2023-01-14 0:57 ` Kees Cook
2023-01-14 1:38 ` Daniel Latypov
@ 2023-01-14 4:31 ` David Gow
2 siblings, 0 replies; 12+ messages in thread
From: David Gow @ 2023-01-14 4:31 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Guenter Roeck, Nick Desaulniers, Nathan Chancellor,
linux-hardening, Vlastimil Babka, Daniel Latypov, Josh Poimboeuf,
Geert Uytterhoeven, Miguel Ojeda, Isabella Basso, Dan Williams,
Rasmus Villemoes, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4503 bytes --]
On Sat, 14 Jan 2023 at 08:54, Kees Cook <keescook@chromium.org> wrote:
>
> Since the long memcpy tests may stall a system for tens of seconds
> in virtualized architecture environments, split those tests off under
> CONFIG_MEMCPY_SLOW_KUNIT_TEST so they can be separately disabled.
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/lkml/20221226195206.GA2626419@roeck-us.net
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Reviewed-and-tested-by: Guenter Roeck <linux@roeck-us.net>
> Reviewed-by: David Gow <davidgow@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
Thanks: this is okay as-is, but I left a few suggestions below, having
tried it out a bit more.
Daniel's comment about kunit_skip() aborting the test is the only real
fix, but depending on how easy you'd want to make turning these tests
off, there are a couple of possible tweaks to the Kconfig option.
Cheers,
-- David
> v2: fix tristate to bool
> v1: https://lore.kernel.org/lkml/20230107040203.never.112-kees@kernel.org
> ---
> lib/Kconfig.debug | 9 +++++++++
> lib/memcpy_kunit.c | 15 ++++++++++++---
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index c2c78d0e761c..f90637171453 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2621,6 +2621,15 @@ config MEMCPY_KUNIT_TEST
>
> If unsure, say N.
>
> +config MEMCPY_SLOW_KUNIT_TEST
> + bool "Include exhaustive memcpy tests" if !KUNIT_ALL_TESTS
I think it'd be better to not include the "if !KUNIT_ALL_TESTS" here,
because it's very convenient to be able to use:
./tools/testing/kunit/kunit.py run memcpy --kconfig_add
CONFIG_MEMCPY_SLOW_KUNIT_TEST=n
to override it.
That does undermine the way KUNIT_ALL_TESTS works a bit, though it
depends if you want to consider this a new set of tests, or just an
option for the existing ones.
> + depends on MEMCPY_KUNIT_TEST
> + default KUNIT_ALL_TESTS
Does this default work for everyone? Personally, I think these tests
aren't slow enough that we'd want them disabled from an "all tests"
build by default. So I'd keep it as-is.
> + help
> + Some memcpy tests are quite exhaustive in checking for overlaps
> + and bit ranges. These can be very slow, so they are split out
> + as a separate config.
> +
> config IS_SIGNED_TYPE_KUNIT_TEST
> tristate "Test is_signed_type() macro" if !KUNIT_ALL_TESTS
> depends on KUNIT
> diff --git a/lib/memcpy_kunit.c b/lib/memcpy_kunit.c
> index 89128551448d..5a545e1b5dbb 100644
> --- a/lib/memcpy_kunit.c
> +++ b/lib/memcpy_kunit.c
> @@ -307,8 +307,12 @@ static void set_random_nonzero(struct kunit *test, u8 *byte)
> }
> }
>
> -static void init_large(struct kunit *test)
> +static int init_large(struct kunit *test)
> {
> + if (!IS_ENABLED(CONFIG_MEMCPY_SLOW_KUNIT_TEST)) {
> + kunit_skip(test, "Slow test skipped. Enable with CONFIG_MEMCPY_SLOW_KUNIT_TEST=y");
> + return -EBUSY;
As Daniel notes, it shouldn't be necessary to return here, as
kunit_skip() will abort the test. (It's a macro, so isn't marked
__noreturn itself, but kunit_try_catch_throw(), which it uses, is.)
> + }
>
> /* Get many bit patterns. */
> get_random_bytes(large_src, ARRAY_SIZE(large_src));
> @@ -319,6 +323,8 @@ static void init_large(struct kunit *test)
>
> /* Explicitly zero the entire destination. */
> memset(large_dst, 0, ARRAY_SIZE(large_dst));
> +
> + return 0;
> }
>
> /*
> @@ -327,7 +333,9 @@ static void init_large(struct kunit *test)
> */
> static void copy_large_test(struct kunit *test, bool use_memmove)
> {
> - init_large(test);
> +
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of non-overlapping bytes ... */
> for (int bytes = 1; bytes <= ARRAY_SIZE(large_src); bytes++) {
> @@ -472,7 +480,8 @@ static void memmove_overlap_test(struct kunit *test)
> static const int bytes_start = 1;
> static const int bytes_end = ARRAY_SIZE(large_src) + 1;
>
> - init_large(test);
> + if (init_large(test))
> + return;
>
> /* Copy a growing number of overlapping bytes ... */
> for (int bytes = bytes_start; bytes < bytes_end;
> --
> 2.34.1
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread