[v4,2/3] kunit: Use the static key when retrieving the current test

Commit Message

David Gow Nov. 25, 2022, 8:43 a.m. UTC

In order to detect if a KUnit test is running, and to access its
context, the 'kunit_test' member of the current task_struct is used.
Usually, this is accessed directly or via the kunit_fail_current_task()
function.

In order to speed up the case where no test is running, add a wrapper,
kunit_get_current_test(), which uses the static key to fail early.
Equally, Speed up kunit_fail_current_test() by using the static key.

This should make it convenient for code to call this
unconditionally in fakes or error paths, without worrying that this will
slow the code down significantly.

If CONFIG_KUNIT=n (or m), this compiles away to nothing. If
CONFIG_KUNIT=y, it will compile down to a NOP (on most architectures) if
no KUnit test is currently running.

Note that kunit_get_current_test() does not work if KUnit is built as a
module. This mirrors the existing restriction on kunit_fail_current_test().

Note that the definition of kunit_fail_current_test() still wraps an
empty, inline function if KUnit is not built-in. This is to ensure that
the printf format string __attribute__ will still work.

Also update the documentation to suggest users use the new
kunit_get_current_test() function, update the example, and to describe
the behaviour when KUnit is disabled better.

Cc: Jonathan Corbet <[e-mail]>
Cc: Sadiya Kazi <[e-mail]>
Signed-off-by: David Gow <[e-mail]>
Reviewed-by: Daniel Latypov <[e-mail]>
---

As-is, the only code which will be directly affected by this (via the
kunit_fail_current_test() change) will be UBSAN's KUnit integration.

Patches to port other tests to use kunit_get_current_test() will be sent
separately (other than the SLUB one in patch 3/3). KASAN in particular
are reworking their KUnit tests and integration, so we'll use this in a
follow up to avoid introducing a conflict.

Changes since v3:
https://lore.kernel.org/linux-kselftest/[e-mail]/
- Use DECLARE_STATIC_KEY_FALSE() -- Thanks Daniel.
- Some documentation rewording to make the behaviour a bit clearer.
  - Thanks Daniel and Sadiya

Changes since v2:
https://lore.kernel.org/all/[e-mail]/
- Only add kunit_get_current_test() when KUnit is built-in, as the
  static key isn't available otherwise.
  - I'm going to try to put together some patches to make things like
    this available when CONFIG_KUNIT=m in the future.
  - Also update the documentation to note this.

Changes since v1:
https://lore.kernel.org/linux-kselftest/[e-mail]/
- Fix a missing '}' which broke everything. Thanks Kees, kernel test
  robot.
- Add the new kunit_get_current_test() function, as most of the cases
  where we retrieve the current test (even to fail it) were accessing
  current->kunit_test directly, not using kunit_fail_current_test().
- Add some documentation comments.
- Update the documentation in usage.rst.
  - The version in tips.rst was not updated, and will be removed:
  https://lore.kernel.org/linux-kselftest/[e-mail]/

---
 Documentation/dev-tools/kunit/usage.rst | 30 +++++++++-----
 include/kunit/test-bug.h                | 53 +++++++++++++++++++++++--
 2 files changed, 71 insertions(+), 12 deletions(-)

Comments

Kees Cook Dec. 2, 2022, 12:54 a.m. UTC | #1

On Fri, Nov 25, 2022 at 04:43:05PM +0800, David Gow wrote:
> In order to detect if a KUnit test is running, and to access its
> context, the 'kunit_test' member of the current task_struct is used.
> Usually, this is accessed directly or via the kunit_fail_current_task()
> function.
> 
> In order to speed up the case where no test is running, add a wrapper,
> kunit_get_current_test(), which uses the static key to fail early.
> Equally, Speed up kunit_fail_current_test() by using the static key.
> 
> This should make it convenient for code to call this
> unconditionally in fakes or error paths, without worrying that this will
> slow the code down significantly.
> 
> If CONFIG_KUNIT=n (or m), this compiles away to nothing. If
> CONFIG_KUNIT=y, it will compile down to a NOP (on most architectures) if
> no KUnit test is currently running.
> 
> Note that kunit_get_current_test() does not work if KUnit is built as a
> module. This mirrors the existing restriction on kunit_fail_current_test().
> 
> Note that the definition of kunit_fail_current_test() still wraps an
> empty, inline function if KUnit is not built-in. This is to ensure that
> the printf format string __attribute__ will still work.
> 
> Also update the documentation to suggest users use the new
> kunit_get_current_test() function, update the example, and to describe
> the behaviour when KUnit is disabled better.
> 
> Cc: Jonathan Corbet <[e-mail]>
> Cc: Sadiya Kazi <[e-mail]>
> Signed-off-by: David Gow <[e-mail]>

Reviewed-by: Kees Cook <[e-mail]>

Sadiya Kazi Dec. 5, 2022, 4:27 a.m. UTC | #2

On Fri, Nov 25, 2022 at 2:13 PM 'David Gow' via KUnit Development
<[e-mail]> wrote:
>
> In order to detect if a KUnit test is running, and to access its
> context, the 'kunit_test' member of the current task_struct is used.
> Usually, this is accessed directly or via the kunit_fail_current_task()
> function.
>
> In order to speed up the case where no test is running, add a wrapper,
> kunit_get_current_test(), which uses the static key to fail early.
> Equally, Speed up kunit_fail_current_test() by using the static key.
>
> This should make it convenient for code to call this
> unconditionally in fakes or error paths, without worrying that this will
> slow the code down significantly.
>
> If CONFIG_KUNIT=n (or m), this compiles away to nothing. If
> CONFIG_KUNIT=y, it will compile down to a NOP (on most architectures) if
> no KUnit test is currently running.
>
> Note that kunit_get_current_test() does not work if KUnit is built as a
> module. This mirrors the existing restriction on kunit_fail_current_test().
>
> Note that the definition of kunit_fail_current_test() still wraps an
> empty, inline function if KUnit is not built-in. This is to ensure that
> the printf format string __attribute__ will still work.
>
> Also update the documentation to suggest users use the new
> kunit_get_current_test() function, update the example, and to describe
> the behaviour when KUnit is disabled better.
>
> Cc: Jonathan Corbet <[e-mail]>
> Cc: Sadiya Kazi <[e-mail]>
> Signed-off-by: David Gow <[e-mail]>
> Reviewed-by: Daniel Latypov <[e-mail]>
> ---
Thank you, David. This looks great to me.
Reviewed-by: Sadiya Kazi <[e-mail]>

Best Regards,
Sadiya Kazi
>
> As-is, the only code which will be directly affected by this (via the
> kunit_fail_current_test() change) will be UBSAN's KUnit integration.
>
> Patches to port other tests to use kunit_get_current_test() will be sent
> separately (other than the SLUB one in patch 3/3). KASAN in particular
> are reworking their KUnit tests and integration, so we'll use this in a
> follow up to avoid introducing a conflict.
>
> Changes since v3:
> https://lore.kernel.org/linux-kselftest/[e-mail]/
> - Use DECLARE_STATIC_KEY_FALSE() -- Thanks Daniel.
> - Some documentation rewording to make the behaviour a bit clearer.
>   - Thanks Daniel and Sadiya
>
> Changes since v2:
> https://lore.kernel.org/all/[e-mail]/
> - Only add kunit_get_current_test() when KUnit is built-in, as the
>   static key isn't available otherwise.
>   - I'm going to try to put together some patches to make things like
>     this available when CONFIG_KUNIT=m in the future.
>   - Also update the documentation to note this.
>
> Changes since v1:
> https://lore.kernel.org/linux-kselftest/[e-mail]/
> - Fix a missing '}' which broke everything. Thanks Kees, kernel test
>   robot.
> - Add the new kunit_get_current_test() function, as most of the cases
>   where we retrieve the current test (even to fail it) were accessing
>   current->kunit_test directly, not using kunit_fail_current_test().
> - Add some documentation comments.
> - Update the documentation in usage.rst.
>   - The version in tips.rst was not updated, and will be removed:
>   https://lore.kernel.org/linux-kselftest/[e-mail]/
>
> ---
>  Documentation/dev-tools/kunit/usage.rst | 30 +++++++++-----
>  include/kunit/test-bug.h                | 53 +++++++++++++++++++++++--
>  2 files changed, 71 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 22416ebb94ab..48f8196d5aad 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -641,17 +641,23 @@ as shown in next section: *Accessing The Current Test*.
>  Accessing The Current Test
>  --------------------------
>
> -In some cases, we need to call test-only code from outside the test file.
> -For example, see example in section *Injecting Test-Only Code* or if
> -we are providing a fake implementation of an ops struct. Using
> -``kunit_test`` field in ``task_struct``, we can access it via
> -``current->kunit_test``.
> +In some cases, we need to call test-only code from outside the test file.  This
> +is helpful, for example, when providing a fake implementation of a function, or
> +to fail any current test from within an error handler.
> +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``.
>
> -The example below includes how to implement "mocking":
> +``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.
> +
> +The example below uses this to implement a "mock" implementation of a function, ``foo``:
>
>  .. code-block:: c
>
> -       #include <linux/sched.h> /* for current */
> +       #include <kunit/test-bug.h> /* for kunit_get_current_test */
>
>         struct test_data {
>                 int foo_result;
> @@ -660,7 +666,7 @@ The example below includes how to implement "mocking":
>
>         static int fake_foo(int arg)
>         {
> -               struct kunit *test = current->kunit_test;
> +               struct kunit *test = kunit_get_current_test();
>                 struct test_data *test_data = test->priv;
>
>                 KUNIT_EXPECT_EQ(test, test_data->want_foo_called_with, arg);
> @@ -691,7 +697,7 @@ Each test can have multiple resources which have string names providing the same
>  flexibility as a ``priv`` member, but also, for example, allowing helper
>  functions to create resources without conflicting with each other. It is also
>  possible to define a clean up function for each resource, making it easy to
> -avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/test.rst.
> +avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/resource.rst.
>
>  Failing The Current Test
>  ------------------------
> @@ -719,3 +725,9 @@ structures as shown below:
>         static void my_debug_function(void) { }
>         #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.
> +
> diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
> index 5fc58081d511..c1b2e14eab64 100644
> --- a/include/kunit/test-bug.h
> +++ b/include/kunit/test-bug.h
> @@ -9,16 +9,63 @@
>  #ifndef _KUNIT_TEST_BUG_H
>  #define _KUNIT_TEST_BUG_H
>
> -#define kunit_fail_current_test(fmt, ...) \
> -       __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
> -
>  #if IS_BUILTIN(CONFIG_KUNIT)
>
> +#include <linux/jump_label.h> /* For static branch */
> +#include <linux/sched.h>
> +
> +/* Static key if KUnit is running any tests. */
> +DECLARE_STATIC_KEY_FALSE(kunit_running);
> +
> +/**
> + * kunit_get_current_test() - Return a pointer to the currently running
> + *                           KUnit test.
> + *
> + * If a KUnit test is running in the current task, returns a pointer to its
> + * associated struct kunit. This pointer can then be passed to any KUnit
> + * function or assertion. If no test is running (or a test is running in a
> + * different task), returns NULL.
> + *
> + * This function is safe to call even when KUnit is disabled. If CONFIG_KUNIT
> + * is not enabled, it will compile down to nothing and will return quickly no
> + * test is running.
> + */
> +static inline struct kunit *kunit_get_current_test(void)
> +{
> +       if (!static_branch_unlikely(&kunit_running))
> +               return NULL;
> +
> +       return current->kunit_test;
> +}
> +
> +
> +/**
> + * 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)) {                   \
> +                       __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, ...);
>
>  #else
>
> +static inline struct kunit *kunit_get_current_test(void) { return NULL; }
> +
> +/* We define this with an empty helper function so format string warnings work */
> +#define kunit_fail_current_test(fmt, ...) \
> +               __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
> +
>  static inline __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
>                                                             const char *fmt, ...)
>  {
> --
> 2.38.1.584.g0f3c55d4c2-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 [e-mail].
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20221125084306.1063074-2-davidgow%40google.com.

Patch

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 22416ebb94ab..48f8196d5aad 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -641,17 +641,23 @@  as shown in next section: *Accessing The Current Test*.
 Accessing The Current Test
 --------------------------
 
-In some cases, we need to call test-only code from outside the test file.
-For example, see example in section *Injecting Test-Only Code* or if
-we are providing a fake implementation of an ops struct. Using
-``kunit_test`` field in ``task_struct``, we can access it via
-``current->kunit_test``.
+In some cases, we need to call test-only code from outside the test file.  This
+is helpful, for example, when providing a fake implementation of a function, or
+to fail any current test from within an error handler.
+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``.
 
-The example below includes how to implement "mocking":
+``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.
+
+The example below uses this to implement a "mock" implementation of a function, ``foo``:
 
 .. code-block:: c
 
-	#include <linux/sched.h> /* for current */
+	#include <kunit/test-bug.h> /* for kunit_get_current_test */
 
 	struct test_data {
 		int foo_result;
@@ -660,7 +666,7 @@  The example below includes how to implement "mocking":
 
 	static int fake_foo(int arg)
 	{
-		struct kunit *test = current->kunit_test;
+		struct kunit *test = kunit_get_current_test();
 		struct test_data *test_data = test->priv;
 
 		KUNIT_EXPECT_EQ(test, test_data->want_foo_called_with, arg);
@@ -691,7 +697,7 @@  Each test can have multiple resources which have string names providing the same
 flexibility as a ``priv`` member, but also, for example, allowing helper
 functions to create resources without conflicting with each other. It is also
 possible to define a clean up function for each resource, making it easy to
-avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/test.rst.
+avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/resource.rst.
 
 Failing The Current Test
 ------------------------
@@ -719,3 +725,9 @@  structures as shown below:
 	static void my_debug_function(void) { }
 	#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.
+
diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
index 5fc58081d511..c1b2e14eab64 100644
--- a/include/kunit/test-bug.h
+++ b/include/kunit/test-bug.h
@@ -9,16 +9,63 @@ 
 #ifndef _KUNIT_TEST_BUG_H
 #define _KUNIT_TEST_BUG_H
 
-#define kunit_fail_current_test(fmt, ...) \
-	__kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
-
 #if IS_BUILTIN(CONFIG_KUNIT)
 
+#include <linux/jump_label.h> /* For static branch */
+#include <linux/sched.h>
+
+/* Static key if KUnit is running any tests. */
+DECLARE_STATIC_KEY_FALSE(kunit_running);
+
+/**
+ * kunit_get_current_test() - Return a pointer to the currently running
+ *			      KUnit test.
+ *
+ * If a KUnit test is running in the current task, returns a pointer to its
+ * associated struct kunit. This pointer can then be passed to any KUnit
+ * function or assertion. If no test is running (or a test is running in a
+ * different task), returns NULL.
+ *
+ * This function is safe to call even when KUnit is disabled. If CONFIG_KUNIT
+ * is not enabled, it will compile down to nothing and will return quickly no
+ * test is running.
+ */
+static inline struct kunit *kunit_get_current_test(void)
+{
+	if (!static_branch_unlikely(&kunit_running))
+		return NULL;
+
+	return current->kunit_test;
+}
+
+
+/**
+ * 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)) {			\
+			__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, ...);
 
 #else
 
+static inline struct kunit *kunit_get_current_test(void) { return NULL; }
+
+/* We define this with an empty helper function so format string warnings work */
+#define kunit_fail_current_test(fmt, ...) \
+		__kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
+
 static inline __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
 							    const char *fmt, ...)
 {