linux-kernel - Re: [PATCH 2/2] kunit: Use the static key in kunit_fail_current

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <C8A2E3D7-E62D-43B3-AECC-3B6117E1DD0E@kernel.org>
Date:   Fri, 21 Oct 2022 07:16:46 -0700
From:   Kees Cook <kees@...nel.org>
To:     David Gow <davidgow@...gle.com>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Daniel Latypov <dlatypov@...gle.com>,
        Shuah Khan <skhan@...uxfoundation.org>
CC:     kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org, Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH 2/2] kunit: Use the static key in kunit_fail_current_test()

On October 21, 2022 12:28:55 AM PDT, David Gow <davidgow@...gle.com> wrote:
>Speed up the case where kunit_fail_current_test() is called when no test
>is running. This should make it convenient for code to call this
>unconditionally in some error paths, without fear of causing a
>performance problem.

A third patch showing these cases may help in understanding the utility. I get it, but I lack imagination on where/why they would be added. :)

>
>If CONFIG_KUNIT=n, 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. kunit_fail_current_test() does not work if KUnit
>itself is built as a module, though this is a pre-existing limitation.
>
>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.
>
>Signed-off-by: David Gow <davidgow@...gle.com>
>---
> include/kunit/test-bug.h | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
>diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
>index 5fc58081d511..ba9558a9f9c0 100644
>--- a/include/kunit/test-bug.h
>+++ b/include/kunit/test-bug.h
>@@ -9,16 +9,29 @@
> #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 */
>+
>+/* Static key if KUnit is running any tests. */
>+extern struct static_key_false kunit_running;
>+
>+#define kunit_fail_current_test(fmt, ...) do {					\
>+	if (static_branch_unlikely(&kunit_running)) {				\

This trailing { should be dropped.

>+		__kunit_fail_current_test(__FILE__, __LINE__,			\
>+					  fmt, ##__VA_ARGS__);			\

Or a closing one added here. (The {}s are unbalanced, as 0day complained about.)

>+	} while (0)
>+
>+
> extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
> 						    const char *fmt, ...);
> 
> #else
> 
>+/* 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, ...)
> {


-- 
Kees Cook