[v3,4/7] init: main: add KUnit to kernel init

Commit Message

Brendan Higgins Feb. 28, 2020, 1:20 a.m. UTC

Remove KUnit from init calls entirely, instead call directly from
kernel_init().

Co-developed-by: Alan Maguire <[e-mail]>
Signed-off-by: Alan Maguire <[e-mail]>
Signed-off-by: Brendan Higgins <[e-mail]>
Reviewed-by: Stephen Boyd <[e-mail]>
---
 include/kunit/test.h | 9 +++++++++
 init/main.c          | 4 ++++
 lib/kunit/executor.c | 4 +---
 3 files changed, 14 insertions(+), 3 deletions(-)

Comments

Frank Rowand March 2, 2020, 7:13 p.m. UTC | #1

On 2/27/20 7:20 PM, Brendan Higgins wrote:
> Remove KUnit from init calls entirely, instead call directly from
> kernel_init().
> 
> Co-developed-by: Alan Maguire <[e-mail]>
> Signed-off-by: Alan Maguire <[e-mail]>
> Signed-off-by: Brendan Higgins <[e-mail]>
> Reviewed-by: Stephen Boyd <[e-mail]>
> ---
>  include/kunit/test.h | 9 +++++++++
>  init/main.c          | 4 ++++
>  lib/kunit/executor.c | 4 +---
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 8a02f93a6b505..8689dd1459844 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -197,6 +197,15 @@ void kunit_init_test(struct kunit *test, const char *name);
>  
>  int kunit_run_tests(struct kunit_suite *suite);
>  
> +#if IS_BUILTIN(CONFIG_KUNIT)

I suspected this would not work if a unittest was builtin but CONFIG_KUNIT
was set to module.

So I decided to experiment a bit to verify my assumptions (before applying
this patch series).  I tried to set CONFIG_KUNIT to module, then set
CONFIG_KUNIT_EXAMPLE_TEST to built in.  Kconfig does not let me do this
because KUNIT_EXAMPLE_TEST is inside a 'if KUNIT' in lib/kunit/Kconfig,
but instead switches KUNIT_EXAMPLE_TEST to a module, and warns that it
has done so.  This was a bit of a surprise, but seems reasonable.

So my next assumption is that the architecture of KUnit expects
each individual unit test config option to depend upon CONFIG_KUNIT.
If this is the case, please clearly document that requirement in
the KUnit documentation.


> +int kunit_run_all_tests(void);
> +#else
> +static inline int kunit_run_all_tests(void)
> +{
> +	return 0;
> +}
> +#endif /* IS_BUILTIN(CONFIG_KUNIT) */
> +
>  /*
>   * If a test suite is built-in, module_init() gets translated into
>   * an initcall which we don't want as the idea is that for builtins
> diff --git a/init/main.c b/init/main.c
> index ee4947af823f3..7875a5c486dc4 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -104,6 +104,8 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/initcall.h>
>  
> +#include <kunit/test.h>
> +
>  static int kernel_init(void *);
>  
>  extern void init_IRQ(void);
> @@ -1444,6 +1446,8 @@ static noinline void __init kernel_init_freeable(void)
>  
>  	do_basic_setup();
>  
> +	kunit_run_all_tests();
> +
>  	console_on_rootfs();
>  
>  	/*
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 6429927d598a5..b75a46c560847 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -11,7 +11,7 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
>  
>  #if IS_BUILTIN(CONFIG_KUNIT)
>  
> -static int kunit_run_all_tests(void)
> +int kunit_run_all_tests(void)
>  {
>  	struct kunit_suite * const * const *suites, * const *subsuite;
>  	bool has_test_failed = false;
> @@ -31,6 +31,4 @@ static int kunit_run_all_tests(void)
>  	return 0;
>  }
>  
> -late_initcall(kunit_run_all_tests);
> -
>  #endif /* IS_BUILTIN(CONFIG_KUNIT) */
>

Kees Cook March 2, 2020, 10:45 p.m. UTC | #2

On 2/27/20 7:20 PM, Brendan Higgins wrote:
> Remove KUnit from init calls entirely, instead call directly from
> kernel_init().
> 
> Co-developed-by: Alan Maguire <[e-mail]>
> Signed-off-by: Alan Maguire <[e-mail]>
> Signed-off-by: Brendan Higgins <[e-mail]>
> Reviewed-by: Stephen Boyd <[e-mail]>
> [...]
> diff --git a/init/main.c b/init/main.c
> index ee4947af823f3..7875a5c486dc4 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -104,6 +104,8 @@
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/initcall.h>
>  
> +#include <kunit/test.h>
> +
>  static int kernel_init(void *);
>  
>  extern void init_IRQ(void);
> @@ -1444,6 +1446,8 @@ static noinline void __init kernel_init_freeable(void)
>  
>  	do_basic_setup();
>  
> +	kunit_run_all_tests();
> +
>  	console_on_rootfs();
>  
>  	/*

I'm nervous about this happening before two key pieces of the kernel
setup, which might lead to weird timing-sensitive bugs or false
positives:
	async_synchronize_full()
	mark_readonly()

Now, I realize kunit tests _should_ be self-contained, but this seems
like a possible robustness problem. Is there any reason this can't be
moved after rcu_end_inkernel_boot() in kernel_init() instead?

Brendan Higgins June 24, 2020, 8:15 p.m. UTC | #3

On Mon, Mar 2, 2020 at 11:13 AM Frank Rowand <[e-mail]> wrote:

Sorry it took so long to respond. I am reviving this patchset now,
about to send out a new revision and I just saw this comment.

> On 2/27/20 7:20 PM, Brendan Higgins wrote:
> > Remove KUnit from init calls entirely, instead call directly from
> > kernel_init().
> >
> > Co-developed-by: Alan Maguire <[e-mail]>
> > Signed-off-by: Alan Maguire <[e-mail]>
> > Signed-off-by: Brendan Higgins <[e-mail]>
> > Reviewed-by: Stephen Boyd <[e-mail]>
> > ---
> >  include/kunit/test.h | 9 +++++++++
> >  init/main.c          | 4 ++++
> >  lib/kunit/executor.c | 4 +---
> >  3 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 8a02f93a6b505..8689dd1459844 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -197,6 +197,15 @@ void kunit_init_test(struct kunit *test, const char *name);
> >
> >  int kunit_run_tests(struct kunit_suite *suite);
> >
> > +#if IS_BUILTIN(CONFIG_KUNIT)
>
> I suspected this would not work if a unittest was builtin but CONFIG_KUNIT
> was set to module.
>
> So I decided to experiment a bit to verify my assumptions (before applying
> this patch series).  I tried to set CONFIG_KUNIT to module, then set
> CONFIG_KUNIT_EXAMPLE_TEST to built in.  Kconfig does not let me do this
> because KUNIT_EXAMPLE_TEST is inside a 'if KUNIT' in lib/kunit/Kconfig,
> but instead switches KUNIT_EXAMPLE_TEST to a module, and warns that it
> has done so.  This was a bit of a surprise, but seems reasonable.
>
> So my next assumption is that the architecture of KUnit expects
> each individual unit test config option to depend upon CONFIG_KUNIT.
> If this is the case, please clearly document that requirement in
> the KUnit documentation.

Your assumption is correct. I will fix this in the Kconfig
documentation in a separate patch.

> > +int kunit_run_all_tests(void);
> > +#else
> > +static inline int kunit_run_all_tests(void)
> > +{
> > +     return 0;
> > +}
> > +#endif /* IS_BUILTIN(CONFIG_KUNIT) */
> > +
> >  /*
> >   * If a test suite is built-in, module_init() gets translated into
> >   * an initcall which we don't want as the idea is that for builtins
> > diff --git a/init/main.c b/init/main.c
> > index ee4947af823f3..7875a5c486dc4 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -104,6 +104,8 @@
> >  #define CREATE_TRACE_POINTS
> >  #include <trace/events/initcall.h>
> >
> > +#include <kunit/test.h>
> > +
> >  static int kernel_init(void *);
> >
> >  extern void init_IRQ(void);
> > @@ -1444,6 +1446,8 @@ static noinline void __init kernel_init_freeable(void)
> >
> >       do_basic_setup();
> >
> > +     kunit_run_all_tests();
> > +
> >       console_on_rootfs();
> >
> >       /*
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 6429927d598a5..b75a46c560847 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -11,7 +11,7 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
> >
> >  #if IS_BUILTIN(CONFIG_KUNIT)
> >
> > -static int kunit_run_all_tests(void)
> > +int kunit_run_all_tests(void)
> >  {
> >       struct kunit_suite * const * const *suites, * const *subsuite;
> >       bool has_test_failed = false;
> > @@ -31,6 +31,4 @@ static int kunit_run_all_tests(void)
> >       return 0;
> >  }
> >
> > -late_initcall(kunit_run_all_tests);
> > -
> >  #endif /* IS_BUILTIN(CONFIG_KUNIT) */
> >
>

Brendan Higgins June 24, 2020, 8:20 p.m. UTC | #4

On Mon, Mar 2, 2020 at 2:45 PM Kees Cook <[e-mail]> wrote:

Sorry it took so long to respond. I am reviving this patchset now,
about to send out a new revision and I just saw this comment.

> On 2/27/20 7:20 PM, Brendan Higgins wrote:
> > Remove KUnit from init calls entirely, instead call directly from
> > kernel_init().
> >
> > Co-developed-by: Alan Maguire <[e-mail]>
> > Signed-off-by: Alan Maguire <[e-mail]>
> > Signed-off-by: Brendan Higgins <[e-mail]>
> > Reviewed-by: Stephen Boyd <[e-mail]>
> > [...]
> > diff --git a/init/main.c b/init/main.c
> > index ee4947af823f3..7875a5c486dc4 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -104,6 +104,8 @@
> >  #define CREATE_TRACE_POINTS
> >  #include <trace/events/initcall.h>
> >
> > +#include <kunit/test.h>
> > +
> >  static int kernel_init(void *);
> >
> >  extern void init_IRQ(void);
> > @@ -1444,6 +1446,8 @@ static noinline void __init kernel_init_freeable(void)
> >
> >       do_basic_setup();
> >
> > +     kunit_run_all_tests();
> > +
> >       console_on_rootfs();
> >
> >       /*
>
> I'm nervous about this happening before two key pieces of the kernel
> setup, which might lead to weird timing-sensitive bugs or false
> positives:
>         async_synchronize_full()
>         mark_readonly()
>
> Now, I realize kunit tests _should_ be self-contained, but this seems
> like a possible robustness problem. Is there any reason this can't be
> moved after rcu_end_inkernel_boot() in kernel_init() instead?

I tried that, but it doesn't work without an initramfs. We could add
an initramfs for KUnit at some point if highly desired, but I think
that is outside the scope of this patchset. Additionally, this patch
actually moves running tests to later in the init process, which is
still an improvement over the way KUnit works today.

There are some other reasons I wouldn't want to make that change right
now, which will become apparent in a patch that I will send out in
short order.

Cheers

Kees Cook June 24, 2020, 8:48 p.m. UTC | #5

On Wed, Jun 24, 2020 at 01:20:35PM -0700, Brendan Higgins wrote:
> On Mon, Mar 2, 2020 at 2:45 PM Kees Cook <[e-mail]> wrote:
> > Now, I realize kunit tests _should_ be self-contained, but this seems
> > like a possible robustness problem. Is there any reason this can't be
> > moved after rcu_end_inkernel_boot() in kernel_init() instead?
> 
> I tried that, but it doesn't work without an initramfs. We could add

I'm curious to know what happened. To me it looks like it would be
possible to do it in here:

        system_state = SYSTEM_RUNNING;
        numa_default_policy();

        rcu_end_inkernel_boot();

        do_sysctl_args();

	put it here?

        if (ramdisk_execute_command) {
                ret = run_init_process(ramdisk_execute_command);

That should be before anything happens with an initramfs. (i.e. boot the
kernel without an initrd and it won't be required...)

> an initramfs for KUnit at some point if highly desired, but I think
> that is outside the scope of this patchset. Additionally, this patch
> actually moves running tests to later in the init process, which is
> still an improvement over the way KUnit works today.

Later is better! :)

> There are some other reasons I wouldn't want to make that change right
> now, which will become apparent in a patch that I will send out in
> short order.

Cool; I'll look for it.

Patch

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 8a02f93a6b505..8689dd1459844 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -197,6 +197,15 @@  void kunit_init_test(struct kunit *test, const char *name);
 
 int kunit_run_tests(struct kunit_suite *suite);
 
+#if IS_BUILTIN(CONFIG_KUNIT)
+int kunit_run_all_tests(void);
+#else
+static inline int kunit_run_all_tests(void)
+{
+	return 0;
+}
+#endif /* IS_BUILTIN(CONFIG_KUNIT) */
+
 /*
  * If a test suite is built-in, module_init() gets translated into
  * an initcall which we don't want as the idea is that for builtins
diff --git a/init/main.c b/init/main.c
index ee4947af823f3..7875a5c486dc4 100644
--- a/init/main.c
+++ b/init/main.c
@@ -104,6 +104,8 @@ 
 #define CREATE_TRACE_POINTS
 #include <trace/events/initcall.h>
 
+#include <kunit/test.h>
+
 static int kernel_init(void *);
 
 extern void init_IRQ(void);
@@ -1444,6 +1446,8 @@  static noinline void __init kernel_init_freeable(void)
 
 	do_basic_setup();
 
+	kunit_run_all_tests();
+
 	console_on_rootfs();
 
 	/*
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 6429927d598a5..b75a46c560847 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -11,7 +11,7 @@  extern struct kunit_suite * const * const __kunit_suites_end[];
 
 #if IS_BUILTIN(CONFIG_KUNIT)
 
-static int kunit_run_all_tests(void)
+int kunit_run_all_tests(void)
 {
 	struct kunit_suite * const * const *suites, * const *subsuite;
 	bool has_test_failed = false;
@@ -31,6 +31,4 @@  static int kunit_run_all_tests(void)
 	return 0;
 }
 
-late_initcall(kunit_run_all_tests);
-
 #endif /* IS_BUILTIN(CONFIG_KUNIT) */