Re: [PATCH v2 1/2] kunit: add extra assertions to KUnit test cases

Le 24/08/2026 à 21:32, Malte Wechter a écrit :
> add extra assertions for each individual test case, that checks that
> both `debug_locks` and `TAINT_WARN` are intact after the test is run.
> The assertions are optional behind CONFIG_KUNIT_EXTRA_ASSERTS.
> 
> Signed-off-by: Malte Wechter <[email protected]>
> ---

Thanks for this.

I'd prefer to split this up into two separate changes, one to check for 
warnings, and one for lockdep issues.

There's already a patch series to handle lockdep failures here:
https://lore.kernel.org/all/[email protected]/

It's pretty similar to this, but may have some good ideas in it.

As for WARN, we have a new feature which allows individual WARN calls to 
be expected and ignored. This works by having the WARN() macro directly 
check if a test is failing. The difference there is that the code only 
runs if the current thread is part of the test (so it wouldn't be 
affected by a WARN() in another, non-test thread). There are advantages 
and disadvantages to this, but it's probably sensible to be consistent here.

Take a look at this for more details on how it works:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=85347718ab0dd7ede9c3e1dcff2d604c7073df05

(That being said, the current implementation does seem to work pretty 
well even with the backtrace suppression, so continuing to use this 
implementation is not necessarily a dealbreaker.)

Finally, I'd appreciate there being some more documentation of this, if 
possible. In particular, it'd be nice to extend the KUnit documentation 
in Documentation/dev-tools/kunit to mention this and provide some 
examples of how to enable it. In particular, it'd be good to have an 
example kunit.py invocation which enables lockdep, e.g.
./tools/testing/kunit/kunit.py run --kconfig_add 
CONFIG_KUNIT_EXTRA_ASSERTS=y --kconfig_add CONFIG_PROVE_LOCKING=y 
--kconfig_add CONFIG_DEBUG_KERNEL=y

Thanks again for looking into this!

Cheers,
-- David


>   lib/kunit/Kconfig     | 12 ++++++++++++
>   lib/kunit/try-catch.c | 23 ++++++++++++++++++++++-
>   2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 94ff8e4089bfb..38801f7493669 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -142,4 +142,16 @@ config KUNIT_UML_PCI
>   
>   	  If unsure, say N.
>   
> +config KUNIT_EXTRA_ASSERTS
> +	bool "Enable extra assertions in KUnit tests"
> +	depends on LOCKDEP
> +	default n
> +	help
> +		Enables all extra assertions for KUnit which includes asserting `TAINT_WARN` and
> +		`debug_locks` from lockdep. A KUnit test suite (and test case) is inserted
> +		at the start of all KUnit test suites. This makes assertions prior to running any
> +		tests, as a pre-test integrity check. Assertions are made after each test case which
> +		asserts that each test case did not trigger either `TAINT_WARN` or `debug_locks`.
> +
> +		If unsure, say N.

A few notes here:
- I'd rather have the taint/warn and lockdep assertions separate here.
- A more descriptive name than "EXTRA_ASSERTS" would be nice. Perhaps 
something like KUNIT_FAIL_TEST_ON_WARN / KUNIT_FAIL_ON_LOCKDEP or similar?

>   endif # KUNIT
> diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> index d84a879f0a789..7eea3af4c9671 100644
> --- a/lib/kunit/try-catch.c
> +++ b/lib/kunit/try-catch.c
> @@ -41,6 +41,11 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
>   	struct completion *task_done;
>   	int exit_code, time_remaining;
>   
> +	#ifdef CONFIG_KUNIT_EXTRA_ASSERTS
> +	int debug_locks_snapshot = debug_locks;
> +	int tainted_warn_snapshot = test_taint(TAINT_WARN);
> +	#endif

Nit: Let's keep the preprocessor #ifdef/#endif lines un-indented here.

> +
>   	try_catch->context = context;
>   	try_catch->try_result = 0;
>   	task_struct = kthread_create(kunit_generic_run_threadfn_adapter,
> @@ -70,7 +75,23 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
>   	put_task_struct(task_struct);
>   	exit_code = try_catch->try_result;
>   
> -	if (!exit_code)
> +	#ifdef CONFIG_KUNIT_EXTRA_ASSERTS
> +	bool extra_assert = false;

Please define all variables at the top of the function / scope.

That being said, we don't need this extra_assert variable at all if you 
move the setting of try_result above moving it to exit_code above?

> +
> +	if (debug_locks_snapshot != debug_locks && !exit_code) {
> +		extra_assert = true;
> +		try_catch->try_result = -EDEADLK;

Why are we setting this, and then never handling it?

> +		kunit_err(test, "Test triggered lockdep\n");
> +	} else if (tainted_warn_snapshot != test_taint(TAINT_WARN) && !exit_code) {
> +		extra_assert = true;
> +		try_catch->try_result = -EDEADLK;

-EDEADLK made sense for the lockdep implementation, but makes less sense 
for WARN(). If you really want to keep it, please document it.

That being said, it should be possible to do this outside of the 
try/catch scope, as both lockdep and taints are global. And if you fail 
the test using the hook mechanism (like the warning suppression does), 
you won't need a separate check afterwards.

> +		kunit_err(test, "Test tainted kernel with TAINT_WARN\n");
> +	}
> +	#else
> +	bool extra_assert = false;

If we declare this unconditionally at the top of the function, there's 
no need to have it in an #ifdef.

> +	#endif
> +
> +	if (!exit_code && !extra_assert)
>   		return;
>   
>   	if (exit_code == -EFAULT)
>