[PATCH 0/2] kunit: add boot time parameter to enable KUnit

[Joe Fradley]

There are some use cases where the kernel binary is desired to be the same
for both production and testing. This poses a problem for users of KUnit
as built-in tests will automatically run at startup and test modules
can still be loaded leaving the kernel in an unsafe state. There is a
"test" taint flag that gets set if a test runs but nothing to prevent
the execution.

This patch adds the kunit.enable module parameter that will need to be
set to true in addition to KUNIT being enabled for KUnit tests to run.
The default value is true giving backwards compatibility. However, for
the production+testing use case the new config option
KUNIT_ENABLE_DEFAULT_DISABLED can be enabled to default kunit.enable to
false requiring the tester to opt-in by passing kunit.enable=1 to
the kernel.

Joe Fradley (2):
kunit: add kunit.enable to enable/disable KUnit test
kunit: no longer call module_info(test, "Y") for kunit modules

.../admin-guide/kernel-parameters.txt | 6 ++++++
include/kunit/test.h | 1 -
lib/kunit/Kconfig | 8 ++++++++
lib/kunit/test.c | 20 +++++++++++++++++++
4 files changed, 34 insertions(+), 1 deletion(-)

--
2.37.1.595.g718a3a8f04-goog

[Joe Fradley]

This patch adds the kunit.enable module parameter that will need to be
set to true in addition to KUNIT being enabled for KUnit tests to run.
The default value is true giving backwards compatibility. However, for
the production+testing use case the new config option
KUNIT_ENABLE_DEFAULT_DISABLED can be enabled to default kunit.enable to
false requiring the tester to opt-in by passing kunit.enable=1 to
the kernel.

Signed-off-by: Joe Fradley <joefr...@google.com>
---
.../admin-guide/kernel-parameters.txt | 6 ++++++

lib/kunit/Kconfig | 8 ++++++++
lib/kunit/test.c | 20 +++++++++++++++++++

3 files changed, 34 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index d7f30902fda0..ab4c7d133c38 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2436,6 +2436,12 @@
0: force disabled
1: force enabled

+ kunit.enable= [KUNIT] Enable executing KUnit tests. Requires
+ CONFIG_KUNIT to be set to be fully enabled. The
+ default value can be overridden to disabled via
+ KUNIT_ENABLE_DEFAULT_DISABLED.
+ Default is 1 (enabled)
+
kvm.ignore_msrs=[KVM] Ignore guest accesses to unhandled MSRs.
Default is 0 (don't ignore, but inject #GP)

diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
index 0b5dfb001bac..5d6db58dbe9b 100644
--- a/lib/kunit/Kconfig
+++ b/lib/kunit/Kconfig
@@ -59,4 +59,12 @@ config KUNIT_ALL_TESTS

If unsure, say N.

+config KUNIT_ENABLE_DEFAULT_DISABLED
+ bool "Require boot parameter to enable KUnit test execution"
+ help
+ Sets the default value of the kernel parameter kunit.enable to 0
+ (disabled). This means to fully enable kunit, config KUNIT needs
+ to be enabled along with `kunit.enable=1` passed to the kernel. This
+ allows KUnit to be opt-in at boot time.
+
endif # KUNIT
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index b73d5bb5c473..e6f98e16e8ae 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -54,6 +54,17 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
#endif

+/*
+ * Enable KUnit tests to run.
+ */
+#ifdef CONFIG_KUNIT_ENABLE_DEFAULT_DISABLED
+static bool enable_param;
+#else
+static bool enable_param = true;
+#endif
+module_param_named(enable, enable_param, bool, 0);
+MODULE_PARM_DESC(enable, "Enable KUnit tests to run");
+
/*
* KUnit statistic mode:
* 0 - disabled
@@ -590,6 +601,12 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
{
unsigned int i;

+ if (!enable_param && num_suites > 0) {
+ pr_info("kunit: deactivated, cannot load %s\n",
+ suites != NULL && suites[0] != NULL ? suites[0]->name : "NULL");
+ return -EPERM;
+ }
+
for (i = 0; i < num_suites; i++) {
kunit_init_suite(suites[i]);
kunit_run_tests(suites[i]);
@@ -607,6 +624,9 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites)
{
unsigned int i;

+ if (!enable_param)
+ return;
+
for (i = 0; i < num_suites; i++)
kunit_exit_suite(suites[i]);

--
2.37.1.595.g718a3a8f04-goog

[Joe Fradley]

Because KUnit test execution is not a guarantee with the kunit.enable
parameter we want to be careful to only taint the kernel only if an
actual test runs. Calling module_info(test, "Y") for every KUnit module
automatically causes the kernel to be tainted upon module load. Therefore,
we're removing this call and relying on the KUnit framework to taint the
kernel or not.

Signed-off-by: Joe Fradley <joefr...@google.com>
---

include/kunit/test.h | 1 -
1 file changed, 1 deletion(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index c958855681cc..f23d3954aa17 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -251,7 +251,6 @@ static inline int kunit_run_all_tests(void)
#endif /* IS_BUILTIN(CONFIG_KUNIT) */

#define __kunit_test_suites(unique_array, ...) \
- MODULE_INFO(test, "Y"); \
static struct kunit_suite *unique_array[] \
__aligned(sizeof(struct kunit_suite *)) \
__used __section(".kunit_test_suites") = { __VA_ARGS__ }
--
2.37.1.595.g718a3a8f04-goog

[David Gow]

+Nico Pache in case this could be useful with kernel/kunit packaging.

On Thu, Aug 18, 2022 at 12:49 AM 'Joe Fradley' via KUnit Development
<kuni...@googlegroups.com> wrote:
>
> This patch adds the kunit.enable module parameter that will need to be
> set to true in addition to KUNIT being enabled for KUnit tests to run.
> The default value is true giving backwards compatibility. However, for
> the production+testing use case the new config option
> KUNIT_ENABLE_DEFAULT_DISABLED can be enabled to default kunit.enable to
> false requiring the tester to opt-in by passing kunit.enable=1 to
> the kernel.
>
> Signed-off-by: Joe Fradley <joefr...@google.com>
> ---

Thanks for sending this out.

I'm generally in support of the idea, and the implementation is okay,
but there are a few small usability issues or bits of futureproofing
we could do.

On the first front, this doesn't integrate well with kunit_tool: if
built-in tests are disabled, it will print the test header and test
plan, but no results, which confuses the kunit_tool parser.
In addition, maybe it'd be nice to have kunit_tool automatically pass
kunit.enable=1 to any kernels it boots. Equally, a few minor
naming/description suggestions.

More details in comments below.

On the second topic, I think we need to work out exactly how we can
evolve this to make it as useful as possible upstream going forward.
In general, while there's nothing fundamentally wrong with having
tests disabled at runtime, it is going to be a very niche feature, as
for most users the correct solution here is to build a new kernel,
without KUnit.

My feeling is that the real distinction worth making here is that
tests can be compiled in and loaded (e.g., if tests are embedded in a
non-test module, like apparmor, or thunderbolt, or (soon) amdgpu), but
won't execute automatically. Now, at the moment there's no way to
manually trigger a test. so this distinction isn't yet important, but
we may want to add something down the line, such as the ability to
trigger a test via debugfs (this was proposed as part of the original
debugfs support packages), or the ability to change the value of this
'enable' flag at runtime, and then load a specific test.

Maybe that involves some further changes to the implementation (at its
most extreme, it could involve moving the checks out into the module
loader and the kernel_init_freeable() function, though I don't think
that's _necessary_), but for the most part, it probably just involves
describing and documenting this change as such.

Would something like that still serve Android's purposes? Or is it
critically important that the idea behind this is "if this is set to
false, there should be no way of running KUnit tests", and having a
manual way to trigger them down the line would defeat the point?

Cheers,
-- David

Hmm... would it make more sense to have this be DEFAULT_ENABLED and
have the default value of the config option be y instead?
Personally, I think that'd avoid the double-negative, and so might be clearer.

> endif # KUNIT
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index b73d5bb5c473..e6f98e16e8ae 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -54,6 +54,17 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
> EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
> #endif
>
> +/*
> + * Enable KUnit tests to run.
> + */
> +#ifdef CONFIG_KUNIT_ENABLE_DEFAULT_DISABLED
> +static bool enable_param;
> +#else
> +static bool enable_param = true;
> +#endif
> +module_param_named(enable, enable_param, bool, 0);
> +MODULE_PARM_DESC(enable, "Enable KUnit tests to run");

Maybe "Enable KUnit tests" is simpler than adding "to run", which
reads a bit awkwardly.

If we were to treat this variable as specifically enabling the "run
tests on boot" and/or "module load", then that could be worked into
the description, too.

> +
> /*
> * KUnit statistic mode:
> * 0 - disabled
> @@ -590,6 +601,12 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
> {
> unsigned int i;
>
> + if (!enable_param && num_suites > 0) {
> + pr_info("kunit: deactivated, cannot load %s\n",
> + suites != NULL && suites[0] != NULL ? suites[0]->name : "NULL");
> + return -EPERM;
> + }
> +

This mostly works, but has a few small issues:
- KUnit will still print the header and a test plan, so kunit_tool
will report a large number of "crashed" tests when no results are
forthcoming.
It should be posible to add a similar check in kunit_run_all_tests()
to handle that case:
https://elixir.bootlin.com/linux/latest/source/lib/kunit/executor.c#L246
You can test this with:
./tools/testing/kunit/kunit.py run --kconfig_add
CONFIG_KUNIT_ENABLE_DEFAULT_DISABLED=y

- The message is not ideal: it only refers to the first suite in the
module (or built into the kernel). and "cannot load" is not really
applicable to built-in tests.
Maybe the goal should be less to "not load test modules" but more to
"allow test modules to load, but don't run the tests in them".
Thoughts?

- If we were to treat this as "tests load, but don't run
automatically", then we probably don't want this to be an EPERM...

> for (i = 0; i < num_suites; i++) {
> kunit_init_suite(suites[i]);
> kunit_run_tests(suites[i]);
> @@ -607,6 +624,9 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites)
> {
> unsigned int i;
>
> + if (!enable_param)
> + return;
> +
> for (i = 0; i < num_suites; i++)
> kunit_exit_suite(suites[i]);
>
> --
> 2.37.1.595.g718a3a8f04-goog
>

> --
> 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+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220817164851.3574140-2-joefradley%40google.com.

[David Gow]

On Thu, Aug 18, 2022 at 12:49 AM Joe Fradley <joefr...@google.com> wrote:
>
> Because KUnit test execution is not a guarantee with the kunit.enable
> parameter we want to be careful to only taint the kernel only if an
> actual test runs. Calling module_info(test, "Y") for every KUnit module
> automatically causes the kernel to be tainted upon module load. Therefore,
> we're removing this call and relying on the KUnit framework to taint the
> kernel or not.
>
> Signed-off-by: Joe Fradley <joefr...@google.com>
> ---

Thanks!

This definitely fixes an assumption I'd had about KUnit-usage which
definitely doesn't hold: that all KUnit tests would be in their own
modules (or at least that those modules wouldn't need to be loaded on
otherwise production systems). Given this isn't the case for a number
of modules (thuderbolt, apparmor, probably soon amdgpu), it makles
sense to get rid of this and only taint the kernel when the test is
actually run, not just when it's loaded.

This could be considered a fix for c272612cb4a2 ("kunit: Taint the
kernel when KUnit tests are run"), as it'd already be possible to
load, e.g., thunderbolt, but prevent the tests from executing with a
filter glob which doesn't match any tests. That possibly shouldn't
taint the kernel.

Reviewed-by: David Gow <davi...@google.com>
Fixes: c272612cb4a2 ("kunit: Taint the kernel when KUnit tests are run")

Cheers,
-- David

[Joe Fradley]

On Fri, Aug 19, 2022 at 1:16 AM David Gow <davi...@google.com> wrote:
>
> +Nico Pache in case this could be useful with kernel/kunit packaging.
>
> On Thu, Aug 18, 2022 at 12:49 AM 'Joe Fradley' via KUnit Development
> <kuni...@googlegroups.com> wrote:
> >
> > This patch adds the kunit.enable module parameter that will need to be
> > set to true in addition to KUNIT being enabled for KUnit tests to run.
> > The default value is true giving backwards compatibility. However, for
> > the production+testing use case the new config option
> > KUNIT_ENABLE_DEFAULT_DISABLED can be enabled to default kunit.enable to
> > false requiring the tester to opt-in by passing kunit.enable=1 to
> > the kernel.
> >
> > Signed-off-by: Joe Fradley <joefr...@google.com>
> > ---
>
> Thanks for sending this out.
>
> I'm generally in support of the idea, and the implementation is okay,
> but there are a few small usability issues or bits of futureproofing
> we could do.
>
> On the first front, this doesn't integrate well with kunit_tool: if
> built-in tests are disabled, it will print the test header and test
> plan, but no results, which confuses the kunit_tool parser.

Thanks for pointing this out and reviewing this patch. I neglected
to test it with the wrapper script. I had only tested manually with
local VMs.

> In addition, maybe it'd be nice to have kunit_tool automatically pass
> kunit.enable=1 to any kernels it boots. Equally, a few minor
> naming/description suggestions.

Makes sense to me, I’ll add it. This would technically make the missing
TAP header and results a moot point. (But we can still safeguard that
scenario)

As of right now, we wouldn’t want someone to enable KUnit after boot.
However, because we lock out debugfs for these instances maybe that
would be an acceptable path to enable it after boot.

I’m wondering why these ‘mixed’ modules would need that. Is it that
those tests shouldn’t/can’t run at startup?

That would definitely be more clear. We just want to make sure it's backwards
compatible, which it should be if we default it in the config to y.

>
> > endif # KUNIT
> > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > index b73d5bb5c473..e6f98e16e8ae 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -54,6 +54,17 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
> > EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
> > #endif
> >
> > +/*
> > + * Enable KUnit tests to run.
> > + */
> > +#ifdef CONFIG_KUNIT_ENABLE_DEFAULT_DISABLED
> > +static bool enable_param;
> > +#else
> > +static bool enable_param = true;
> > +#endif
> > +module_param_named(enable, enable_param, bool, 0);
> > +MODULE_PARM_DESC(enable, "Enable KUnit tests to run");
>
> Maybe "Enable KUnit tests" is simpler than adding "to run", which
> reads a bit awkwardly.
>
> If we were to treat this variable as specifically enabling the "run
> tests on boot" and/or "module load", then that could be worked into
> the description, too.

Going the “Enable KUnit tests” route is definitely less awkward, thanks.

>
> > +
> > /*
> > * KUnit statistic mode:
> > * 0 - disabled
> > @@ -590,6 +601,12 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
> > {
> > unsigned int i;
> >
> > + if (!enable_param && num_suites > 0) {
> > + pr_info("kunit: deactivated, cannot load %s\n",
> > + suites != NULL && suites[0] != NULL ? suites[0]->name : "NULL");
> > + return -EPERM;
> > + }
> > +
>
> This mostly works, but has a few small issues:
> - KUnit will still print the header and a test plan, so kunit_tool
> will report a large number of "crashed" tests when no results are
> forthcoming.
> It should be posible to add a similar check in kunit_run_all_tests()
> to handle that case:
> https://elixir.bootlin.com/linux/latest/source/lib/kunit/executor.c#L246
> You can test this with:
> ./tools/testing/kunit/kunit.py run --kconfig_add
> CONFIG_KUNIT_ENABLE_DEFAULT_DISABLED=y

Thank you, I can easily repro that output now and adding a check in
`kunit_run_all_tests()` as you suggested cleans it up. However, it
still shows an error if the TAP header is missing and if I add the
header, it errors on having 0 tests. But maybe either of these are
OK, as it conveys no tests have run even though you're running
the `knuit_tool` assumably expecting test output. But we're not
falsely stating particular tests have crashed.

Also, now that we’re going to be passing in kunit.enable=1 it will be
very difficult, if not impossible, to get in this situation.

>
> - The message is not ideal: it only refers to the first suite in the
> module (or built into the kernel). and "cannot load" is not really
> applicable to built-in tests.

If I move this check up one call into `kunit_modue_init` I could use
the module name. (There won't be a built-in path through here any
more after the `kunit_run_all_tests() addition.) However,
`__kunit_test_suites_init()` is exported so theoretically someone else
could call it bypassing the check. So, I think I'll leave it where it's at
now and simplify the message to something like:
"kunit: disabled, not executing tests\n"

> Maybe the goal should be less to "not load test modules" but more to
> "allow test modules to load, but don't run the tests in them".
> Thoughts?

Originally the feeling was, if we aren't going to run the tests, why
load the test modules? However, with the “mixed” modules you
mentioned in mind we would want to load them just not run the tests.

>
> - If we were to treat this as "tests load, but don't run
> automatically", then we probably don't want this to be an EPERM...

Agreed.

[Joe Fradley]

On Fri, Aug 19, 2022 at 1:34 AM David Gow <davi...@google.com> wrote:
>
> On Thu, Aug 18, 2022 at 12:49 AM Joe Fradley <joefr...@google.com> wrote:
> >
> > Because KUnit test execution is not a guarantee with the kunit.enable
> > parameter we want to be careful to only taint the kernel only if an
> > actual test runs. Calling module_info(test, "Y") for every KUnit module
> > automatically causes the kernel to be tainted upon module load. Therefore,
> > we're removing this call and relying on the KUnit framework to taint the
> > kernel or not.
> >
> > Signed-off-by: Joe Fradley <joefr...@google.com>
> > ---
>
> Thanks!
>
> This definitely fixes an assumption I'd had about KUnit-usage which
> definitely doesn't hold: that all KUnit tests would be in their own
> modules (or at least that those modules wouldn't need to be loaded on
> otherwise production systems). Given this isn't the case for a number
> of modules (thuderbolt, apparmor, probably soon amdgpu), it makles
> sense to get rid of this and only taint the kernel when the test is
> actually run, not just when it's loaded.
>
> This could be considered a fix for c272612cb4a2 ("kunit: Taint the
> kernel when KUnit tests are run"), as it'd already be possible to
> load, e.g., thunderbolt, but prevent the tests from executing with a
> filter glob which doesn't match any tests. That possibly shouldn't
> taint the kernel.

Great, thank you for the review.

[Brendan Higgins]

On Wed, Aug 17, 2022 at 12:49 PM 'Joe Fradley' via KUnit Development
<kuni...@googlegroups.com> wrote:
>
> Because KUnit test execution is not a guarantee with the kunit.enable
> parameter we want to be careful to only taint the kernel only if an
> actual test runs. Calling module_info(test, "Y") for every KUnit module
> automatically causes the kernel to be tainted upon module load. Therefore,
> we're removing this call and relying on the KUnit framework to taint the
> kernel or not.
>
> Signed-off-by: Joe Fradley <joefr...@google.com>

Reviewed-by: Brendan Higgins <brendan...@google.com>

[Joe Fradley]

v2:
- Added enable check in executor.c to prevent wrong error output from
kunit_tool.py when run against a KUnit disabled kernel
- kunit_tool.py now passes kunit.enable=1
- Flipped around logic of new config to KUNIT_DEFAULT_ENABLED
- Now load modules containing tests but not executing them
- Various message/description text clean up

There are some use cases where the kernel binary is desired to be the same
for both production and testing. This poses a problem for users of KUnit
as built-in tests will automatically run at startup and test modules
can still be loaded leaving the kernel in an unsafe state. There is a
"test" taint flag that gets set if a test runs but nothing to prevent
the execution.

This patch adds the kunit.enable module parameter that will need to be
set to true in addition to KUNIT being enabled for KUnit tests to run.
The default value is true giving backwards compatibility. However, for

the production+testing use case the new config option KUNIT_DEFAULT_ENABLED
can be set to N requiring the tester to opt-in by passing kunit.enable=1 to
the kernel.

Joe Fradley (2):
kunit: add kunit.enable to enable/disable KUnit test
kunit: no longer call module_info(test, "Y") for kunit modules

.../admin-guide/kernel-parameters.txt | 6 +++++

include/kunit/test.h | 3 ++-
lib/kunit/Kconfig | 11 +++++++++
lib/kunit/executor.c | 4 ++++
lib/kunit/test.c | 24 +++++++++++++++++++
tools/testing/kunit/kunit_kernel.py | 1 +
6 files changed, 48 insertions(+), 1 deletion(-)

--
2.37.1.595.g718a3a8f04-goog

[Joe Fradley]

This patch adds the kunit.enable module parameter that will need to be
set to true in addition to KUNIT being enabled for KUnit tests to run.
The default value is true giving backwards compatibility. However, for
the production+testing use case the new config option
KUNIT_DEFAULT_ENABLED can be set to N requiring the tester to opt-in
by passing kunit.enable=1 to the kernel.

Signed-off-by: Joe Fradley <joefr...@google.com>
---

Changes since v1:
- Created a function to get kunit enable state
- Check kunit enable state in kunit_run_all_tests() in executor.c
- Load test module even if KUnit is disabled but still don't execute
tests
- Simplified kunit disable message and kunit.enable parameter
description
- Flipped around logic of new config to be KUNIT_DEFAULT_ENABLED
- kunit_tool.py now passes kunit.enable=1 to kernel

.../admin-guide/kernel-parameters.txt | 6 +++++
include/kunit/test.h | 2 ++

lib/kunit/Kconfig | 11 +++++++++
lib/kunit/executor.c | 4 ++++
lib/kunit/test.c | 24 +++++++++++++++++++
tools/testing/kunit/kunit_kernel.py | 1 +

6 files changed, 48 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index adfda56b2691..7aa3abd7f1c5 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2432,6 +2432,12 @@

0: force disabled
1: force enabled

+ kunit.enable= [KUNIT] Enable executing KUnit tests. Requires
+ CONFIG_KUNIT to be set to be fully enabled. The

+ default value can be overridden via
+ KUNIT_DEFAULT_ENABLED.

+ Default is 1 (enabled)
+
kvm.ignore_msrs=[KVM] Ignore guest accesses to unhandled MSRs.
Default is 0 (don't ignore, but inject #GP)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index c958855681cc..ee6bf4ecbd89 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -228,6 +228,8 @@ static inline void kunit_set_failure(struct kunit *test)
WRITE_ONCE(test->status, KUNIT_FAILURE);
}

+bool kunit_enabled(void);
+
void kunit_init_test(struct kunit *test, const char *name, char *log);

int kunit_run_tests(struct kunit_suite *suite);
diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
index 0b5dfb001bac..626719b95bad 100644
--- a/lib/kunit/Kconfig
+++ b/lib/kunit/Kconfig
@@ -59,4 +59,15 @@ config KUNIT_ALL_TESTS

If unsure, say N.

+config KUNIT_DEFAULT_ENABLED
+ bool "Default value of kunit.enable"
+ default y
+ help
+ Sets the default value of kunit.enable. If set to N then KUnit
+ tests will not execute unless kunit.enable=1 is passed to the
+ kernel command line.
+
+ In most cases this should be left as Y. Only if additional opt-in
+ behavior is needed should this be set to N.
+
endif # KUNIT
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 5e223327196a..9bbc422c284b 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -190,6 +190,10 @@ int kunit_run_all_tests(void)
{
struct suite_set suite_set = {__kunit_suites_start, __kunit_suites_end};
int err = 0;
+ if (!kunit_enabled()) {
+ pr_info("kunit: disabled\n");
+ goto out;
+ }

if (filter_glob_param) {
suite_set = kunit_filter_suites(&suite_set, filter_glob_param, &err);
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index b73d5bb5c473..1e54373309a4 100644

--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -54,6 +54,17 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
#endif

+/*
+ * Enable KUnit tests to run.
+ */

+#ifdef CONFIG_KUNIT_DEFAULT_ENABLED

+static bool enable_param = true;

+#else
+static bool enable_param;

+#endif
+module_param_named(enable, enable_param, bool, 0);

+MODULE_PARM_DESC(enable, "Enable KUnit tests");

+
/*
* KUnit statistic mode:
* 0 - disabled

@@ -586,10 +597,20 @@ static void kunit_init_suite(struct kunit_suite *suite)
suite->suite_init_err = 0;
}

+bool kunit_enabled(void)
+{
+ return enable_param;
+}
+
int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_suites)
{
unsigned int i;

+ if (!kunit_enabled() && num_suites > 0) {
+ pr_info("kunit: disabled\n");
+ return 0;
+ }
+

for (i = 0; i < num_suites; i++) {
kunit_init_suite(suites[i]);
kunit_run_tests(suites[i]);

@@ -607,6 +628,9 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites)
{
unsigned int i;

+ if (!kunit_enabled())

+ return;
+
for (i = 0; i < num_suites; i++)
kunit_exit_suite(suites[i]);

diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index f5c26ea89714..ef794da420d7 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -359,6 +359,7 @@ class LinuxSourceTree:
args = []
if filter_glob:
args.append('kunit.filter_glob='+filter_glob)
+ args.append('kunit.enable=1')

process = self._ops.start(args, build_dir)
assert process.stdout is not None # tell mypy it's set
--
2.37.1.595.g718a3a8f04-goog

[Joe Fradley]

Because KUnit test execution is not a guarantee with the kunit.enable

parameter we want to be careful to only taint the kernel when actual
tests run. Calling module_info(test, "Y") for every KUnit module

automatically causes the kernel to be tainted upon module load. Therefore,
we're removing this call and relying on the KUnit framework to taint the
kernel or not.

Signed-off-by: Joe Fradley <joefr...@google.com>

Reviewed-by: David Gow <davi...@google.com>
Reviewed-by: Brendan Higgins <brendan...@google.com>
---
Changes since v1:
- Added David's and Brendan's Reviewed-by for tags.

include/kunit/test.h | 1 -
1 file changed, 1 deletion(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index ee6bf4ecbd89..512089e5ce4e 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -253,7 +253,6 @@ static inline int kunit_run_all_tests(void)

[David Gow]

On Tue, Aug 23, 2022 at 10:25 PM 'Joe Fradley' via KUnit Development
<kuni...@googlegroups.com> wrote:
>
> This patch adds the kunit.enable module parameter that will need to be
> set to true in addition to KUNIT being enabled for KUnit tests to run.
> The default value is true giving backwards compatibility. However, for
> the production+testing use case the new config option
> KUNIT_DEFAULT_ENABLED can be set to N requiring the tester to opt-in
> by passing kunit.enable=1 to the kernel.
>
> Signed-off-by: Joe Fradley <joefr...@google.com>
> ---

Thanks very much. This looks good to me, and works on my machine.

I've put a few comments/ideas below, but none of them feel necessary to me.

Regardless, this is
Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

This probably isn't strictly necessary at this stage, given that it
just checks one variable. That being said, I don't think it hurts (and
personally, I quite like it), and find it more future-proof than
exposing the variable more widely anyway.

_Maybe_ this could be pr_info_once(), if you were worried about spam
(if a whole bunch of test modules were loaded at once). That being
said, I prefer it as-is, as I don't think there are a lot of cases
where large number of kunit test modules are loaded on a system with
KUnit disable. And I'm liable to forget that KUnit is disabled if a
system has been running for a while (and maybe one test module was
loaded a boot), and end up wondering why my test isn't running.

So, I'm all for leaving this as-is, personally.

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220823142456.3977086-2-joefradley%40google.com.

[Joe Fradley]

On Tue, Aug 23, 2022 at 9:31 PM David Gow <davi...@google.com> wrote:
>
> On Tue, Aug 23, 2022 at 10:25 PM 'Joe Fradley' via KUnit Development
> <kuni...@googlegroups.com> wrote:
> >
> > This patch adds the kunit.enable module parameter that will need to be
> > set to true in addition to KUNIT being enabled for KUnit tests to run.
> > The default value is true giving backwards compatibility. However, for
> > the production+testing use case the new config option
> > KUNIT_DEFAULT_ENABLED can be set to N requiring the tester to opt-in
> > by passing kunit.enable=1 to the kernel.
> >
> > Signed-off-by: Joe Fradley <joefr...@google.com>
> > ---
>
> Thanks very much. This looks good to me, and works on my machine.
>
> I've put a few comments/ideas below, but none of them feel necessary to me.

Thank you for the review. I need to do one follow up revision to base this
off of the appropriate `linux-kselftest/kunit` branch.

It also addressed it being a static variable.

That's the same conclusion I came to after considering the one time
message used for the test taint message.

[David Gow]

On Wed, Aug 24, 2022 at 1:04 PM Joe Fradley <joefr...@google.com> wrote:
>
> On Tue, Aug 23, 2022 at 9:31 PM David Gow <davi...@google.com> wrote:
> >
> > On Tue, Aug 23, 2022 at 10:25 PM 'Joe Fradley' via KUnit Development
> > <kuni...@googlegroups.com> wrote:
> > >
> > > This patch adds the kunit.enable module parameter that will need to be
> > > set to true in addition to KUNIT being enabled for KUnit tests to run.
> > > The default value is true giving backwards compatibility. However, for
> > > the production+testing use case the new config option
> > > KUNIT_DEFAULT_ENABLED can be set to N requiring the tester to opt-in
> > > by passing kunit.enable=1 to the kernel.
> > >
> > > Signed-off-by: Joe Fradley <joefr...@google.com>
> > > ---
> >
> > Thanks very much. This looks good to me, and works on my machine.
> >
> > I've put a few comments/ideas below, but none of them feel necessary to me.
>
> Thank you for the review. I need to do one follow up revision to base this
> off of the appropriate `linux-kselftest/kunit` branch.
>

This already applies cleanly to linux-kselftest/kunit -- it should be
fine as-is.

(It also applies fine to kselftest/kunit-fixes, for what it's worth.)

Cheers,
-- David

[Joe Fradley]

On Tue, Aug 23, 2022 at 11:33 PM David Gow <davi...@google.com> wrote:
>
> On Wed, Aug 24, 2022 at 1:04 PM Joe Fradley <joefr...@google.com> wrote:
> >
> > On Tue, Aug 23, 2022 at 9:31 PM David Gow <davi...@google.com> wrote:
> > >
> > > On Tue, Aug 23, 2022 at 10:25 PM 'Joe Fradley' via KUnit Development
> > > <kuni...@googlegroups.com> wrote:
> > > >
> > > > This patch adds the kunit.enable module parameter that will need to be
> > > > set to true in addition to KUNIT being enabled for KUnit tests to run.
> > > > The default value is true giving backwards compatibility. However, for
> > > > the production+testing use case the new config option
> > > > KUNIT_DEFAULT_ENABLED can be set to N requiring the tester to opt-in
> > > > by passing kunit.enable=1 to the kernel.
> > > >
> > > > Signed-off-by: Joe Fradley <joefr...@google.com>
> > > > ---
> > >
> > > Thanks very much. This looks good to me, and works on my machine.
> > >
> > > I've put a few comments/ideas below, but none of them feel necessary to me.
> >
> > Thank you for the review. I need to do one follow up revision to base this
> > off of the appropriate `linux-kselftest/kunit` branch.
> >
>
> This already applies cleanly to linux-kselftest/kunit -- it should be
> fine as-is.

Ok great, I'll leave as-is.

[Tales Aparecida]

Hi,


This series is
Tested-by: Tales Aparecida <tales.a...@gmail.com>

1. Tested using kunit_tool: running tests and showing output as expected.

2. Tested further by running QEMU through virtme-run with:
$ ../virtme/virtme-run --show-command --kdir ../linux/.for-amd/ --mods=auto --kopt kunit.enable=1

2.a. "KUNIT_DEFAULT_ENABLED" works as expected when "kunit.enable" is omitted
2.b. kunit.enable=0 results in printing "kunit: disabled" on boot and on loading test modules, as expected
3.c. kunit.enable=1 runs tests on boot and allows them to run when loading test modules

3. Regarding taint
3.a. /proc/sys/kernel/tainted is 0 when kunit.enable=0 and does not change when trying to load test modules.
3.b. /proc/sys/kernel/tainted is 0 when kunit.enable=1 until the first test runs, then it becomes 262144 (2^18) as expected.


On other note, there's something I would like to delve into below.

On 23/08/2022 11:24, Joe Fradley wrote:
> v2:
> - Added enable check in executor.c to prevent wrong error output from
> kunit_tool.py when run against a KUnit disabled kernel
> - kunit_tool.py now passes kunit.enable=1
> - Flipped around logic of new config to KUNIT_DEFAULT_ENABLED
> - Now load modules containing tests but not executing them
> - Various message/description text clean up
>
> There are some use cases where the kernel binary is desired to be the same
> for both production and testing. This poses a problem for users of KUnit
> as built-in tests will automatically run at startup and test modules
> can still be loaded leaving the kernel in an unsafe state. There is a
> "test" taint flag that gets set if a test runs but nothing to prevent
> the execution.

Do you have any info on whether these use cases would have something against writing tests for static functions using the documented approach of including the tests into the actual runtime code?
https://docs.kernel.org/dev-tools/kunit/usage.html#testing-static-functions

Otherwise, would them agree to export the symbols that need to be tested?

I would really like to understand better what are these use cases :)

>
> This patch adds the kunit.enable module parameter that will need to be
> set to true in addition to KUNIT being enabled for KUnit tests to run.
> The default value is true giving backwards compatibility. However, for
> the production+testing use case the new config option KUNIT_DEFAULT_ENABLED
> can be set to N requiring the tester to opt-in by passing kunit.enable=1 to
> the kernel.
>
> Joe Fradley (2):
> kunit: add kunit.enable to enable/disable KUnit test
> kunit: no longer call module_info(test, "Y") for kunit modules
>
> .../admin-guide/kernel-parameters.txt | 6 +++++
> include/kunit/test.h | 3 ++-
> lib/kunit/Kconfig | 11 +++++++++
> lib/kunit/executor.c | 4 ++++
> lib/kunit/test.c | 24 +++++++++++++++++++
> tools/testing/kunit/kunit_kernel.py | 1 +
> 6 files changed, 48 insertions(+), 1 deletion(-)
>

Great work!

Kind regards,
Tales

[Joe Fradley]

On Sat, Sep 24, 2022 at 5:42 PM Tales Aparecida
<tales.a...@gmail.com> wrote:
>
> Hi,
>
>
> This series is
> Tested-by: Tales Aparecida <tales.a...@gmail.com>
>
> 1. Tested using kunit_tool: running tests and showing output as expected.
>
> 2. Tested further by running QEMU through virtme-run with:
> $ ../virtme/virtme-run --show-command --kdir ../linux/.for-amd/ --mods=auto --kopt kunit.enable=1
>
> 2.a. "KUNIT_DEFAULT_ENABLED" works as expected when "kunit.enable" is omitted
> 2.b. kunit.enable=0 results in printing "kunit: disabled" on boot and on loading test modules, as expected
> 3.c. kunit.enable=1 runs tests on boot and allows them to run when loading test modules
>
> 3. Regarding taint
> 3.a. /proc/sys/kernel/tainted is 0 when kunit.enable=0 and does not change when trying to load test modules.
> 3.b. /proc/sys/kernel/tainted is 0 when kunit.enable=1 until the first test runs, then it becomes 262144 (2^18) as expected.

Tales, thank you for doing this testing.

>
>
> On other note, there's something I would like to delve into below.
>
>
> On 23/08/2022 11:24, Joe Fradley wrote:
> > v2:
> > - Added enable check in executor.c to prevent wrong error output from
> > kunit_tool.py when run against a KUnit disabled kernel
> > - kunit_tool.py now passes kunit.enable=1
> > - Flipped around logic of new config to KUNIT_DEFAULT_ENABLED
> > - Now load modules containing tests but not executing them
> > - Various message/description text clean up
> >
> > There are some use cases where the kernel binary is desired to be the same
> > for both production and testing. This poses a problem for users of KUnit
> > as built-in tests will automatically run at startup and test modules
> > can still be loaded leaving the kernel in an unsafe state. There is a
> > "test" taint flag that gets set if a test runs but nothing to prevent
> > the execution.
>
> Do you have any info on whether these use cases would have something against writing tests for static functions using the documented approach of including the tests into the actual runtime code?
> https://docs.kernel.org/dev-tools/kunit/usage.html#testing-static-functions
>
> Otherwise, would them agree to export the symbols that need to be tested?
>
> I would really like to understand better what are these use cases :)

I feel using the static functions as described in your link is a good
alternative to
modules with embedded KUnit tests. However, this is a different use case then
I refer to, which is the ability to have the framework embedded in the
kernel for
both production and test with the control of test execution gated on a kernel
command line parameter.

[Tales Aparecida]

I have another question regarding the compilation in this use case.

Does it have a strict requirement to minimize the kernel size?
That is, is there any requirement around built-in test modules
and/or a plan to load test modules on demand when booting with
kunit.enable=1?


I would also like to know whether the use case includes running
unit tests for AMDGPU or DRM, just so we can be aware of it when
writing more tests for static functions using the approach shown
in that link.

[Joe Fradley]

Even though kunit.enable will work as expected for built-in tests,
it's geared more towards the loading test modules scenario. It doesn't
have any requirements for kernel size. But if built-in tests are
included the cost of the extra space will need to be weighed against
the value of having the option to activate the tests via kunit.enable.

> That is, is there any requirement around built-in test modules
> and/or a plan to load test modules on demand when booting with
> kunit.enable=1?

There's no current plan to load test modules on demand that wouldn't
otherwise be loaded when kunit.enable=1.

>
>
> I would also like to know whether the use case includes running
> unit tests for AMDGPU or DRM, just so we can be aware of it when
> writing more tests for static functions using the approach shown
> in that link.

I can't speak specifically about AMDGPU or DRM at the moment
because it depends on the targets we're testing. But it's a good
question in general about tests that are embedded in modules. It's
effectively the same tradeoff I mentioned earlier in regards to built-in
tests. The pros in having the tests activated by kunit.enable would
need to outweigh the cons of the size addition. It would be
preferable for the tests to be in a separate module.