Message-ID: <20230117142737.246446-1-davidgow@google.com>
Date:   Tue, 17 Jan 2023 22:27:37 +0800
From:   David Gow <davidgow@...gle.com>
To:     Brendan Higgins <brendan.higgins@...ux.dev>,
        Kees Cook <keescook@...omium.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Daniel Latypov <dlatypov@...gle.com>,
        Rae Moar <rmoar@...gle.com>
Cc:     Sadiya Kazi <sadiyakazi@...gle.com>, kunit-dev@...glegroups.com,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        David Gow <davidgow@...gle.com>
Subject: [RFC PATCH] kunit: Add "hooks" to call into KUnit when it's built as
 a module

KUnit has several macros and functions intended for use from non-test
code. These hooks, currently the kunit_get_current_test() and
kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.

In order to support this case, the required functions and static data
need to be available unconditionally, even when KUnit itself is not
built-in. The new 'hooks.c' file is therefore always included, and has
both the static key required for kunit_get_current_test(), and a
function pointer to the real implementation of
__kunit_fail_current_test(), which is populated when the KUnit module is
loaded.

This can then be extended for future features which require similar
"hook" behaviour, such as static stubs:
https://lore.kernel.org/all/20221208061841.2186447-1-davidgow@google.com/

Signed-off-by: David Gow <davidgow@...gle.com>
---

This is basically a prerequisite for the stub features working when
KUnit is built as a module, and should nicely make a few other tests
work then, too.

I'm not 100% sold on the whole "fill in a table of function pointers
when kunit.ko is loaded" trick: it is basically just working around the
sensible limitations on depending on modules. I think it should be safe
here, as the functions/macros all have fallback behaviour when no test
is running, and this is just another case of that.

Similarly, I'm sure there must be a better way to compile hooks.o in
when KUNIT=y or KUNIT=m, but the trick of adding it separately as an
obj-y in the lib/ Makefile, then having an #if IS_ENABLED() check in the
file is the only one I've been able to come up with using my meagre
knowledge of Kbuild. Better suggestions welcome!
---
 Documentation/dev-tools/kunit/usage.rst | 14 ++++++--------
 include/kunit/test-bug.h                | 15 ++++++++-------
 lib/Makefile                            |  4 ++++
 lib/kunit/Makefile                      |  3 +++
 lib/kunit/hooks.c                       | 23 +++++++++++++++++++++++
 lib/kunit/test.c                        | 10 ++++------
 6 files changed, 48 insertions(+), 21 deletions(-)
 create mode 100644 lib/kunit/hooks.c

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 48f8196d5aad..6424493b93cb 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -648,10 +648,9 @@ We can do this via the ``kunit_test`` field in ``task_struct``, which we can
 access using the ``kunit_get_current_test()`` function in ``kunit/test-bug.h``.
 
 ``kunit_get_current_test()`` is safe to call even if KUnit is not enabled. If
-KUnit is not enabled, was built as a module (``CONFIG_KUNIT=m``), or no test is
-running in the current task, it will return ``NULL``. This compiles down to
-either a no-op or a static key check, so will have a negligible performance
-impact when no test is running.
+KUnit is not enabled, or if no test is running in the current task, it will
+return ``NULL``. This compiles down to either a no-op or a static key check,
+so will have a negligible performance impact when no test is running.
 
 The example below uses this to implement a "mock" implementation of a function, ``foo``:
 
@@ -726,8 +725,7 @@ structures as shown below:
 	#endif
 
 ``kunit_fail_current_test()`` is safe to call even if KUnit is not enabled. If
-KUnit is not enabled, was built as a module (``CONFIG_KUNIT=m``), or no test is
-running in the current task, it will do nothing. This compiles down to either a
-no-op or a static key check, so will have a negligible performance impact when
-no test is running.
+KUnit is not enabled, or if no test is running in the current task, it will do
+nothing. This compiles down to either a no-op or a static key check, so will
+have a negligible performance impact when no test is running.
 
diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
index c1b2e14eab64..122f50198903 100644
--- a/include/kunit/test-bug.h
+++ b/include/kunit/test-bug.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /*
- * KUnit API allowing dynamic analysis tools to interact with KUnit tests
+ * KUnit API providing hooks for non-test code to interact with tests.
  *
  * Copyright (C) 2020, Google LLC.
  * Author: Uriel Guajardo <urielguajardo@...gle.com>
@@ -9,7 +9,7 @@
 #ifndef _KUNIT_TEST_BUG_H
 #define _KUNIT_TEST_BUG_H
 
-#if IS_BUILTIN(CONFIG_KUNIT)
+#if IS_ENABLED(CONFIG_KUNIT)
 
 #include <linux/jump_label.h> /* For static branch */
 #include <linux/sched.h>
@@ -43,20 +43,21 @@ static inline struct kunit *kunit_get_current_test(void)
  * kunit_fail_current_test() - If a KUnit test is running, fail it.
  *
  * If a KUnit test is running in the current task, mark that test as failed.
- *
- * This macro will only work if KUnit is built-in (though the tests
- * themselves can be modules). Otherwise, it compiles down to nothing.
  */
 #define kunit_fail_current_test(fmt, ...) do {					\
 		if (static_branch_unlikely(&kunit_running)) {			\
+			/* Guaranteed to be non-NULL when kunit_running true*/	\
 			__kunit_fail_current_test(__FILE__, __LINE__,		\
 						  fmt, ##__VA_ARGS__);		\
 		}								\
 	} while (0)
 
 
-extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
-						    const char *fmt, ...);
+/* Function pointer defined as a hook in hooks.c, and implemented in test.c */
+typedef __printf(3, 4) void kunit_hook_fn_fail_current_test(const char *file,
+							    int line,
+							    const char *fmt, ...);
+extern kunit_hook_fn_fail_current_test *__kunit_fail_current_test;
 
 #else
 
diff --git a/lib/Makefile b/lib/Makefile
index 4d9461bfea42..9031de6ca73c 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -126,6 +126,10 @@ CFLAGS_test_fpu.o += $(FPU_CFLAGS)
 obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
 
 obj-$(CONFIG_KUNIT) += kunit/
+# Include the KUnit hooks unconditionally. They'll compile to nothing if
+# CONFIG_KUNIT=n, otherwise will be a small table of static data (static key,
+# function pointers) which need to be built-in even when KUnit is a module.
+obj-y += kunit/hooks.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 29aff6562b42..deeb46cc879b 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -11,6 +11,9 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
 kunit-objs +=				debugfs.o
 endif
 
+# KUnit 'hooks' are built-in even when KUnit is built as a module.
+lib-y +=				hooks.o
+
 obj-$(CONFIG_KUNIT_TEST) +=		kunit-test.o
 
 # string-stream-test compiles built-in only.
diff --git a/lib/kunit/hooks.c b/lib/kunit/hooks.c
new file mode 100644
index 000000000000..48189567a774
--- /dev/null
+++ b/lib/kunit/hooks.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit 'Hooks' implementation.
+ *
+ * This file contains code / structures which should be built-in even when
+ * KUnit itself is built as a module.
+ *
+ * Copyright (C) 2022, Google LLC.
+ * Author: David Gow <davidgow@...gle.com>
+ */
+
+/* This file is always built-in, so make sure it's empty if CONFIG_KUNIT=n */
+#if IS_ENABLED(CONFIG_KUNIT)
+
+#include <kunit/test-bug.h>
+
+DEFINE_STATIC_KEY_FALSE(kunit_running);
+EXPORT_SYMBOL(kunit_running);
+
+/* Function pointers for hooks. */
+kunit_hook_fn_fail_current_test *__kunit_fail_current_test;
+EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
+#endif
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c9ebf975e56b..711fdcce6de8 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -20,13 +20,10 @@
 #include "string-stream.h"
 #include "try-catch-impl.h"
 
-DEFINE_STATIC_KEY_FALSE(kunit_running);
-
-#if IS_BUILTIN(CONFIG_KUNIT)
 /*
  * Fail the current test and print an error message to the log.
  */
-void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
+void __kunit_fail_current_test_impl(const char *file, int line, const char *fmt, ...)
 {
 	va_list args;
 	int len;
@@ -53,8 +50,6 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
 	kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer);
 	kunit_kfree(current->kunit_test, buffer);
 }
-EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
-#endif
 
 /*
  * Enable KUnit tests to run.
@@ -777,6 +772,9 @@ EXPORT_SYMBOL_GPL(kunit_cleanup);
 
 static int __init kunit_init(void)
 {
+	/* Install the KUnit hook functions. */
+	__kunit_fail_current_test = __kunit_fail_current_test_impl;
+
 	kunit_debugfs_init();
 #ifdef CONFIG_MODULES
 	return register_module_notifier(&kunit_mod_nb);
-- 
2.39.0.314.g84b9a713c41-goog

Powered by blists - more mailing lists