Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons: - They can result in overlooked real problems. - A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance.
One option to address the problem would be to add messages such as "expected warning backtraces start/end here" to the kernel log. However, that would again require filter scripts, might result in missing real problematic warning backtraces triggered while the test is running, and the irrelevant backtrace(s) would still clog the kernel log.
Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum.
Overview: Patch#1 Introduces the suppression infrastructure. Patch#2 Mitigate the impact at WARN*() sites. Patch#3 Adds selftests to validate the functionality. Patch#4 Demonstrates real-world usage in the DRM subsystem. Patch#5 Documents the new API and usage guidelines.
Design Notes: The objective is to suppress unwanted WARN*() generated messages.
Although most major architectures share common bug handling via `lib/bug.c` and `report_bug()`, some minor or legacy architectures still rely on their own platform-specific handling. This divergence must be considered in any such feature. Additionally, a key challenge in implementing this feature is the fragmentation of `WARN*()` messages emission: specific part in the macro, common with BUG*() part in the exception handler. As a result, any intervention to suppress the message must occur before the illegal instruction.
Lessons from the Previous Attempt In earlier iterations, suppression logic was added inside the `__report_bug()` function to intercept WARN*() messages not producing messages in the macro. To implement the check in the check in the bug handler code, two strategies were considered:
* Strategy #1: Use `kallsyms` to infer the originating functionid, namely a pointer to the function. Since in any case, the user interface relies on function names, they must be translated in addresses at suppression- time or at check-time. Assuming to translate at suppression-time, the `kallsyms` subsystem needs to be used to determine the symbol address from the name, and again to produce the functionid from `bugaddr`. This approach proved unreliable due to compiler-induced transformations such as inlining, cloning, and code fragmentation. Attempts to preventing them is also unconvenient because several `WARN()` sites are in functions intentionally declared as `__always_inline`.
* Strategy #2: Store function name `__func__` in `struct bug_entry` in the `__bug_table`. This implementation was used in the previous version. However, `__func__` is a compiler-generated symbol, which complicates relocation and linking in position-independent code. Workarounds such as storing offsets from `.rodata` or embedding string literals directly into the table would have significantly either increased complexity or increase the __bug_table size. Additionally, architectures not using the unified `BUG()` path would still require ad-hoc handling. Because current WARN*() message production strategy, a few WARN*() macros still need a check to suppress the part of the message produced in the macro itself.
Current Proposal: Check Directly in the `WARN()` Macros. This avoids the need for function symbol resolution or ELF section modification. Suppression is implemented directly in the `WARN*()` macros.
A helper function, `__kunit_is_suppressed_warning()`, is used to determine whether suppression applies. It is marked as `noinstr`, since some `WARN*()` sites reside in non-instrumentable sections. As it uses `strcmp`, a `noinstr` version of `strcmp` was introduced. The implementation is deliberately simple and avoids architecture-specific optimizations to preserve portability. Since this mechanism compares function names and is intended for test usage only, performance is not a primary concern.
This series is based on the RFC patch and subsequent discussion at https://patchwork.kernel.org/project/linux-kselftest/patch/02546e59-1afe-4b0... and offers a more comprehensive solution of the problem discussed there.
Changes since RFC: - Introduced CONFIG_KUNIT_SUPPRESS_BACKTRACE - Minor cleanups and bug fixes - Added support for all affected architectures - Added support for counting suppressed warnings - Added unit tests using those counters - Added patch to suppress warning backtraces in dev_addr_lists tests
Changes since v1: - Rebased to v6.9-rc1 - Added Tested-by:, Acked-by:, and Reviewed-by: tags [I retained those tags since there have been no functional changes] - Introduced KUNIT_SUPPRESS_BACKTRACE configuration option, enabled by default.
Changes since v2: - Rebased to v6.9-rc2 - Added comments to drm warning suppression explaining why it is needed. - Added patch to move conditional code in arch/sh/include/asm/bug.h to avoid kerneldoc warning - Added architecture maintainers to Cc: for architecture specific patches - No functional changes
Changes since v3: - Rebased to v6.14-rc6 - Dropped net: "kunit: Suppress lock warning noise at end of dev_addr_lists tests" since 3db3b62955cd6d73afde05a17d7e8e106695c3b9 - Added __kunit_ and KUNIT_ prefixes. - Tested on interessed architectures.
Changes since v4: - Rebased to v6.15-rc7 - Dropped all code in __report_bug() - Moved all checks in WARN*() macros. - Dropped all architecture specific code. - Made __kunit_is_suppressed_warning nice to noinstr functions.
Alessandro Carminati (2): bug/kunit: Core support for suppressing warning backtraces bug/kunit: Suppressing warning backtraces reduced impact on WARN*() sites
Guenter Roeck (3): Add unit tests to verify that warning backtrace suppression works. drm: Suppress intentional warning backtraces in scaling unit tests kunit: Add documentation for warning backtrace suppression API
Documentation/dev-tools/kunit/usage.rst | 30 ++++++- drivers/gpu/drm/tests/drm_rect_test.c | 16 ++++ include/asm-generic/bug.h | 48 +++++++---- include/kunit/bug.h | 62 ++++++++++++++ include/kunit/test.h | 1 + lib/kunit/Kconfig | 9 ++ lib/kunit/Makefile | 9 +- lib/kunit/backtrace-suppression-test.c | 105 ++++++++++++++++++++++++ lib/kunit/bug.c | 54 ++++++++++++ 9 files changed, 316 insertions(+), 18 deletions(-) create mode 100644 include/kunit/bug.h create mode 100644 lib/kunit/backtrace-suppression-test.c create mode 100644 lib/kunit/bug.c
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons: - They can result in overlooked real problems. - A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance.
Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum.
Implementation details: Check suppression directly in the `WARN()` Macros. This avoids the need for function symbol resolution or ELF section modification. Suppression is implemented directly in the `WARN*()` macros.
A helper function, `__kunit_is_suppressed_warning()`, is used to determine whether suppression applies. It is marked as `noinstr`, since some `WARN*()` sites reside in non-instrumentable sections. As it uses `strcmp`, a `noinstr` version of `strcmp` was introduced. The implementation is deliberately simple and avoids architecture-specific optimizations to preserve portability. Since this mechanism compares function names and is intended for test usage only, performance is not a primary concern.
Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- include/asm-generic/bug.h | 41 ++++++++++++++++---------- include/kunit/bug.h | 61 +++++++++++++++++++++++++++++++++++++++ include/kunit/test.h | 1 + lib/kunit/Kconfig | 9 ++++++ lib/kunit/Makefile | 6 ++-- lib/kunit/bug.c | 50 ++++++++++++++++++++++++++++++++ 6 files changed, 151 insertions(+), 17 deletions(-) create mode 100644 include/kunit/bug.h create mode 100644 lib/kunit/bug.c
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 387720933973..3cc8cb100ccd 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -18,6 +18,7 @@ #endif
#ifndef __ASSEMBLY__ +#include <kunit/bug.h> #include <linux/panic.h> #include <linux/printk.h>
@@ -61,9 +62,12 @@ struct bug_entry { */ #ifndef HAVE_ARCH_BUG #define BUG() do { \ - printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \ - barrier_before_unreachable(); \ - panic("BUG!"); \ + if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ + printk("BUG: failure at %s:%d/%s()!\n", __FILE__, \ + __LINE__, __func__); \ + barrier_before_unreachable(); \ + panic("BUG!"); \ + } } while (0) #endif
@@ -95,21 +99,26 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #ifndef __WARN_FLAGS #define __WARN() __WARN_printf(TAINT_WARN, NULL) #define __WARN_printf(taint, arg...) do { \ - instrumentation_begin(); \ - warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \ - instrumentation_end(); \ + if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ + instrumentation_begin(); \ + warn_slowpath_fmt(__FILE__, __LINE__, taint, arg);\ + instrumentation_end(); \ + } } while (0) #else #define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) #define __WARN_printf(taint, arg...) do { \ - instrumentation_begin(); \ - __warn_printk(arg); \ - __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ - instrumentation_end(); \ + if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ + instrumentation_begin(); \ + __warn_printk(arg); \ + __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | \ + BUGFLAG_TAINT(taint)); \ + instrumentation_end(); \ + } \ } while (0) #define WARN_ON_ONCE(condition) ({ \ int __ret_warn_on = !!(condition); \ - if (unlikely(__ret_warn_on)) \ + if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN_FLAGS(BUGFLAG_ONCE | \ BUGFLAG_TAINT(TAINT_WARN)); \ unlikely(__ret_warn_on); \ @@ -121,7 +130,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #ifndef WARN_ON #define WARN_ON(condition) ({ \ int __ret_warn_on = !!(condition); \ - if (unlikely(__ret_warn_on)) \ + if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN(); \ unlikely(__ret_warn_on); \ }) @@ -138,7 +147,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
#define WARN_TAINT(condition, taint, format...) ({ \ int __ret_warn_on = !!(condition); \ - if (unlikely(__ret_warn_on)) \ + if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN_printf(taint, format); \ unlikely(__ret_warn_on); \ }) @@ -157,8 +166,10 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #else /* !CONFIG_BUG */ #ifndef HAVE_ARCH_BUG #define BUG() do { \ - do {} while (1); \ - unreachable(); \ + if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ + do {} while (1); \ + unreachable(); \ + } \ } while (0) #endif
diff --git a/include/kunit/bug.h b/include/kunit/bug.h new file mode 100644 index 000000000000..9a4eff2897e9 --- /dev/null +++ b/include/kunit/bug.h @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit helpers for backtrace suppression + * + * Copyright (C) 2025 Alessandro Carminati acarmina@redhat.com + * Copyright (C) 2024 Guenter Roeck linux@roeck-us.net + */ + +#ifndef _KUNIT_BUG_H +#define _KUNIT_BUG_H + +#ifndef __ASSEMBLY__ + +#include <linux/kconfig.h> + +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE + +#include <linux/stringify.h> +#include <linux/types.h> + +struct __suppressed_warning { + struct list_head node; + const char *function; + int counter; +}; + +void __kunit_start_suppress_warning(struct __suppressed_warning *warning); +void __kunit_end_suppress_warning(struct __suppressed_warning *warning); +bool __kunit_is_suppressed_warning(const char *function); + +#define KUNIT_DEFINE_SUPPRESSED_WARNING(func) \ + struct __suppressed_warning __kunit_suppress_##func = \ + { .function = __stringify(func), .counter = 0 } + +#define KUNIT_START_SUPPRESSED_WARNING(func) \ + __kunit_start_suppress_warning(&__kunit_suppress_##func) + +#define KUNIT_END_SUPPRESSED_WARNING(func) \ + __kunit_end_suppress_warning(&__kunit_suppress_##func) + +#define KUNIT_IS_SUPPRESSED_WARNING(func) \ + __kunit_is_suppressed_warning(func) + +#define KUNIT_SUPPRESSED_WARNING_COUNT(func) \ + (__kunit_suppress_##func.counter) + +#define KUNIT_SUPPRESSED_WARNING_COUNT_RESET(func) \ + __kunit_suppress_##func.counter = 0 + +#else /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ + +#define KUNIT_DEFINE_SUPPRESSED_WARNING(func) +#define KUNIT_START_SUPPRESSED_WARNING(func) +#define KUNIT_END_SUPPRESSED_WARNING(func) +#define KUNIT_IS_SUPPRESSED_WARNING(func) (false) +#define KUNIT_SUPPRESSED_WARNING_COUNT(func) (0) +#define KUNIT_SUPPRESSED_WARNING_COUNT_RESET(func) + +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */ +#endif /* __ASSEMBLY__ */ +#endif /* _KUNIT_BUG_H */ diff --git a/include/kunit/test.h b/include/kunit/test.h index 39c768f87dc9..bd810ea2f869 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -10,6 +10,7 @@ #define _KUNIT_TEST_H
#include <kunit/assert.h> +#include <kunit/bug.h> #include <kunit/try-catch.h>
#include <linux/args.h> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig index a97897edd964..201402f0ab49 100644 --- a/lib/kunit/Kconfig +++ b/lib/kunit/Kconfig @@ -15,6 +15,15 @@ menuconfig KUNIT
if KUNIT
+config KUNIT_SUPPRESS_BACKTRACE + bool "KUnit - Enable backtrace suppression" + default y + help + Enable backtrace suppression for KUnit. If enabled, backtraces + generated intentionally by KUnit tests are suppressed. Disable + to reduce kernel image size if image size is more important than + suppression of backtraces generated by KUnit tests. + config KUNIT_DEBUGFS bool "KUnit - Enable /sys/kernel/debug/kunit debugfs representation" if !KUNIT_ALL_TESTS default KUNIT_ALL_TESTS diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index 5aa51978e456..3195e861d63c 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -16,8 +16,10 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y) kunit-objs += debugfs.o endif
-# KUnit 'hooks' are built-in even when KUnit is built as a module. -obj-y += hooks.o +# KUnit 'hooks' and bug handling are built-in even when KUnit is built +# as a module. +obj-y += hooks.o \ + bug.o
obj-$(CONFIG_KUNIT_TEST) += kunit-test.o obj-$(CONFIG_KUNIT_TEST) += platform-test.o diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c new file mode 100644 index 000000000000..4da9ae478f25 --- /dev/null +++ b/lib/kunit/bug.c @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit helpers for backtrace suppression + * + * Copyright (C) 2025 Alessandro Carminati acarmina@redhat.com + * Copyright (C) 2024 Guenter Roeck linux@roeck-us.net + */ + +#include <kunit/bug.h> +#include <linux/export.h> +#include <linux/list.h> + +static LIST_HEAD(suppressed_warnings); + +void __kunit_start_suppress_warning(struct __suppressed_warning *warning) +{ + list_add(&warning->node, &suppressed_warnings); +} +EXPORT_SYMBOL_GPL(__kunit_start_suppress_warning); + +void __kunit_end_suppress_warning(struct __suppressed_warning *warning) +{ + list_del(&warning->node); +} +EXPORT_SYMBOL_GPL(__kunit_end_suppress_warning); + +static noinstr int kunit_strcmp(const char *s1, const char *s2) { + while (*s1 != '\0' && *s1 == *s2) { + s1++; + s2++; + } + return *(const unsigned char*)s1 - *(const unsigned char*)s2; +} + +noinstr bool __kunit_is_suppressed_warning(const char *function) +{ + struct __suppressed_warning *warning; + + if (!function) + return false; + + list_for_each_entry(warning, &suppressed_warnings, node) { + if (!kunit_strcmp(function, warning->function)) { + warning->counter++; + return true; + } + } + return false; +} +EXPORT_SYMBOL_GPL(__kunit_is_suppressed_warning);
On Mon, May 26, 2025 at 01:27:51PM +0000, Alessandro Carminati wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons:
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance.
Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum.
Implementation details: Check suppression directly in the `WARN()` Macros. This avoids the need for function symbol resolution or ELF section modification. Suppression is implemented directly in the `WARN*()` macros.
A helper function, `__kunit_is_suppressed_warning()`, is used to determine whether suppression applies. It is marked as `noinstr`, since some `WARN*()` sites reside in non-instrumentable sections. As it uses `strcmp`, a `noinstr` version of `strcmp` was introduced. The implementation is deliberately simple and avoids architecture-specific optimizations to preserve portability. Since this mechanism compares function names and is intended for test usage only, performance is not a primary concern.
Signed-off-by: Guenter Roeck linux@roeck-us.net
I like this -- it's very simple, it doesn't need to be fast-path, so a linear list walker with strcmp is fine. Nice!
Reviewed-by: Kees Cook kees@kernel.org
On Wed, May 28, 2025 at 03:47:42PM -0700, Kees Cook wrote:
On Mon, May 26, 2025 at 01:27:51PM +0000, Alessandro Carminati wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons:
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance.
Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum.
Implementation details: Check suppression directly in the `WARN()` Macros. This avoids the need for function symbol resolution or ELF section modification. Suppression is implemented directly in the `WARN*()` macros.
A helper function, `__kunit_is_suppressed_warning()`, is used to determine whether suppression applies. It is marked as `noinstr`, since some `WARN*()` sites reside in non-instrumentable sections. As it uses `strcmp`, a `noinstr` version of `strcmp` was introduced. The implementation is deliberately simple and avoids architecture-specific optimizations to preserve portability. Since this mechanism compares function names and is intended for test usage only, performance is not a primary concern.
Signed-off-by: Guenter Roeck linux@roeck-us.net
I like this -- it's very simple, it doesn't need to be fast-path, so a linear list walker with strcmp is fine. Nice!
But it is on the fast path! This is still bloating every UD2 site instead of doing it on the other end.
On Thu, May 29, 2025 at 11:02:19AM +0200, Peter Zijlstra wrote:
On Wed, May 28, 2025 at 03:47:42PM -0700, Kees Cook wrote:
On Mon, May 26, 2025 at 01:27:51PM +0000, Alessandro Carminati wrote:
Some unit tests intentionally trigger warning backtraces by passing bad parameters to kernel API functions. Such unit tests typically check the return value from such calls, not the existence of the warning backtrace.
Such intentionally generated warning backtraces are neither desirable nor useful for a number of reasons:
- They can result in overlooked real problems.
- A warning that suddenly starts to show up in unit tests needs to be investigated and has to be marked to be ignored, for example by adjusting filter scripts. Such filters are ad hoc because there is no real standard format for warnings. On top of that, such filter scripts would require constant maintenance.
Solve the problem by providing a means to identify and suppress specific warning backtraces while executing test code. Support suppressing multiple backtraces while at the same time limiting changes to generic code to the absolute minimum.
Implementation details: Check suppression directly in the `WARN()` Macros. This avoids the need for function symbol resolution or ELF section modification. Suppression is implemented directly in the `WARN*()` macros.
A helper function, `__kunit_is_suppressed_warning()`, is used to determine whether suppression applies. It is marked as `noinstr`, since some `WARN*()` sites reside in non-instrumentable sections. As it uses `strcmp`, a `noinstr` version of `strcmp` was introduced. The implementation is deliberately simple and avoids architecture-specific optimizations to preserve portability. Since this mechanism compares function names and is intended for test usage only, performance is not a primary concern.
Signed-off-by: Guenter Roeck linux@roeck-us.net
I like this -- it's very simple, it doesn't need to be fast-path, so a linear list walker with strcmp is fine. Nice!
But it is on the fast path! This is still bloating every UD2 site instead of doing it on the other end.
Doing it on the other end doesn't look great (see the other reply). I was suggesting it's not on fast path because the added code is a dependant conditional following an "unlikley" conditional. But if you wanted to push it totally out of line, we'd likely need to pass __func__ into warn_slowpath_fmt() and __warn_printk(), and then have __warn_printk() return bool to make the call to __WARN_FLAGS() conditional. e.g.:
- warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \ + warn_slowpath_fmt(__FILE__, __LINE__, __func__, taint, arg); \
and:
- __warn_printk(arg); \ - __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ + if (__warn_printk(__func__, arg)) \ + __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\
But it still leaves bare __WARN unhandled...
On Thu, May 29, 2025 at 10:46:15AM -0700, Kees Cook wrote:
Doing it on the other end doesn't look great (see the other reply). I was suggesting it's not on fast path because the added code is a dependant conditional following an "unlikley" conditional. But if you wanted to push it totally out of line, we'd likely need to pass __func__ into warn_slowpath_fmt() and __warn_printk(), and then have __warn_printk()
warn_slowpath_fmt() already uses buildin_return_address(0), and then it can use kallsyms to find the symbol name. No need to pass __func__ as a string.
return bool to make the call to __WARN_FLAGS() conditional. e.g.:
warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \
warn_slowpath_fmt(__FILE__, __LINE__, __func__, taint, arg); \and:
__warn_printk(arg); \__WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\
if (__warn_printk(__func__, arg)) \__WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\But it still leaves bare __WARN unhandled...
Nah, don't propagate, just eat the __WARN and handle things in __report_bug(), which is where they all end up.
But the real purpose here seems to be to supress printk output, so why not use 'suppress_printk' ?
On Mon, May 26, 2025 at 01:27:51PM +0000, Alessandro Carminati wrote:
#define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) #define __WARN_printf(taint, arg...) do { \
instrumentation_begin(); \__warn_printk(arg); \__WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\instrumentation_end(); \
if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \instrumentation_begin(); \__warn_printk(arg); \__WARN_FLAGS(BUGFLAG_NO_CUT_HERE | \BUGFLAG_TAINT(taint)); \instrumentation_end(); \} \#define WARN_ON_ONCE(condition) ({ \ int __ret_warn_on = !!(condition); \
- if (unlikely(__ret_warn_on)) \
- if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN_FLAGS(BUGFLAG_ONCE | \ BUGFLAG_TAINT(TAINT_WARN)); \ unlikely(__ret_warn_on); \
@@ -121,7 +130,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #ifndef WARN_ON #define WARN_ON(condition) ({ \ int __ret_warn_on = !!(condition); \
- if (unlikely(__ret_warn_on)) \
- if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN(); \ unlikely(__ret_warn_on); \
}) @@ -138,7 +147,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #define WARN_TAINT(condition, taint, format...) ({ \ int __ret_warn_on = !!(condition); \
- if (unlikely(__ret_warn_on)) \
- if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN_printf(taint, format); \ unlikely(__ret_warn_on); \
}) @@ -157,8 +166,10 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #else /* !CONFIG_BUG */ #ifndef HAVE_ARCH_BUG #define BUG() do { \
- do {} while (1); \
- unreachable(); \
- if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \
do {} while (1); \unreachable(); \- } \
} while (0) #endif
NAK
This is again doing it wrong -- this will bloat every frigging bug/warn site for no reason.
Like I said before; you need to do this on the report_bug() size of things.
Hi Peter,
Thank you for your follow-up and for reiterating your point.
On Thu, May 29, 2025 at 11:01 AM Peter Zijlstra peterz@infradead.org wrote:
On Mon, May 26, 2025 at 01:27:51PM +0000, Alessandro Carminati wrote:
#define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) #define __WARN_printf(taint, arg...) do { \
instrumentation_begin(); \__warn_printk(arg); \__WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\instrumentation_end(); \
if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \instrumentation_begin(); \__warn_printk(arg); \__WARN_FLAGS(BUGFLAG_NO_CUT_HERE | \BUGFLAG_TAINT(taint)); \instrumentation_end(); \} \ } while (0)#define WARN_ON_ONCE(condition) ({ \ int __ret_warn_on = !!(condition); \
if (unlikely(__ret_warn_on)) \
if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN_FLAGS(BUGFLAG_ONCE | \ BUGFLAG_TAINT(TAINT_WARN)); \ unlikely(__ret_warn_on); \@@ -121,7 +130,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #ifndef WARN_ON #define WARN_ON(condition) ({ \ int __ret_warn_on = !!(condition); \
if (unlikely(__ret_warn_on)) \
if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN(); \ unlikely(__ret_warn_on); \}) @@ -138,7 +147,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
#define WARN_TAINT(condition, taint, format...) ({ \ int __ret_warn_on = !!(condition); \
if (unlikely(__ret_warn_on)) \
if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN_printf(taint, format); \ unlikely(__ret_warn_on); \}) @@ -157,8 +166,10 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #else /* !CONFIG_BUG */ #ifndef HAVE_ARCH_BUG #define BUG() do { \
do {} while (1); \unreachable(); \
if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \do {} while (1); \unreachable(); \} \} while (0) #endif
NAK
This is again doing it wrong -- this will bloat every frigging bug/warn site for no reason.
Like I said before; you need to do this on the report_bug() size of things.
I fully understand your concerns, and I truly appreciate both yours and Josh’s feedback on this matter. Please rest assured that I took your suggestions seriously and carefully evaluated the possibility of consolidating all related logic within the exception handler. After a thorough investigation, however, I encountered several limitations that led me to maintain the check in the macro. I’d like to share the rationale behind this decision: * In the case of WARN() messages, part of the output, the user-specified content, is emitted directly by the macro, prior to reaching the exception handler [1]. Moving the check solely to the exception handler would not prevent this early output. * Unless we change the user-facing interface that allows suppression based on function names, we still need to work with those names at runtime. * This leaves us with two main strategies: converting function names to pointers (e.g., via kallsyms) or continuing to work with names. The former requires name resolution at suppression time and pointer comparison in the handler, but function names are often altered by the compiler due to inlining or other optimizations[2]. Some WARN() sites are even marked __always_inline[3], making it difficult to prevent inlining altogether. * An alternative is to embed function names in the __bug_table. While potentially workable, this increases the size of the table and requires attention to handle position-independent builds (-fPIC/-fPIE), such as using offsets relative to __start_rodata.
However, the central challenge remains: any logic that aims to suppress WARN() output must either move the entire message emission into the exception handler or accept that user-specified parts of the message will still be printed. As a secondary point, there are also less common architectures where it's unclear whether suppressing these warnings is a priority, which might influence how broadly the effort is applied. I hoped to have addressed the concern of having faster runtime, by exposing a counter that could skip the logic. Kess suggested using static branching that would make things even better. Could Kess' suggestion mitigate your concern on this strategy? I’m absolutely open to any further thoughts or suggestions you may have, and I appreciate your continued guidance.
[1]. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/incl... [2]. https://godbolt.org/z/d8aja1Wfz Compiler here emits inlined function and stand alone function to allow pointer usage. [3]. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/incl... this is one example, others exist.
+Mark because he loves a hack :-)
On Thu, May 29, 2025 at 12:36:55PM +0200, Alessandro Carminati wrote:
Like I said before; you need to do this on the report_bug() size of things.
I fully understand your concerns, and I truly appreciate both yours and Josh’s feedback on this matter. Please rest assured that I took your suggestions seriously and carefully evaluated the possibility of consolidating all related logic within the exception handler. After a thorough investigation, however, I encountered several limitations that led me to maintain the check in the macro. I’d like to share the rationale behind this decision:
- In the case of WARN() messages, part of the output, the
user-specified content, is emitted directly by the macro, prior to reaching the exception handler [1]. Moving the check solely to the exception handler would not prevent this early output.
Yeah, this has been really annoying me for a long while. WARN() code gen is often horrible crap because of that.
Everything I've tried so far is worse though :/ So in the end I try to never use WARN(), its just not worth it.
... /me goes down the rabbit-hole again, because well, you can't let something simple like this defeat you ;-)
Results of today's hackery below. It might actually be worth cleaning up.
- Unless we change the user-facing interface that allows suppression
based on function names, we still need to work with those names at runtime.
I'm not sure I understand this. What interface and what names? This is a new feature, so how can there be an interface that needs to be preserved?
- This leaves us with two main strategies: converting function names
to pointers (e.g., via kallsyms) or continuing to work with names. The former requires name resolution at suppression time and pointer comparison in the handler, but function names are often altered by the compiler due to inlining or other optimizations[2]. Some WARN() sites are even marked __always_inline[3], making it difficult to prevent inlining altogether.
Arguably __func__ should be the function name of the function you get inlined into. C inlining does not preserve the sequence point, so there is absolutely no point in trying to preserve the inline name.
I'm again confused though; [2] does not use __func__ at all.
Anyway, when I do something like:
void __attribute__((__always_inline__)) foo(void) { puts(__func__); }
void bar(void) { foo(); }
it uses a "foo" string, which IMO is just plain wrong. Anyway, do both compilers guarantee it will always be foo? I don't think I've seen the GCC manual be explicit about this case.
- An alternative is to embed function names in the __bug_table. While potentially workable, this increases the size of the table and
requires attention to handle position-independent builds (-fPIC/-fPIE), such as using offsets relative to __start_rodata.
However, the central challenge remains: any logic that aims to suppress WARN() output must either move the entire message emission into the exception handler or accept that user-specified parts of the message will still be printed.
Well, we can set suppress_printk and then all is quiet :-) Why isn't this good enough?
As a secondary point, there are also less common architectures where it's unclear whether suppressing these warnings is a priority, which might influence how broadly the effort is applied. I hoped to have addressed the concern of having faster runtime, by exposing a counter that could skip the logic. Kess suggested using static branching that would make things even better. Could Kess' suggestion mitigate your concern on this strategy? I’m absolutely open to any further thoughts or suggestions you may have, and I appreciate your continued guidance.
I'm not really concerned with performance here, but more with the size of the code emitted by WARN_ONCE(). There are a *ton* of WARN sites, while only one report_bug() and printk().
The really offensive thing is that this is for a feature most nobody will ever need :/
The below results in:
03dc 7ac: 48 c7 c0 00 00 00 00 mov $0x0,%rax 7af: R_X86_64_32S .rodata.str1.1+0x223 03e3 7b3: ba 2a 00 00 00 mov $0x2a,%edx 03e8 7b8: 48 0f b9 d0 ud1 %rax,%rdx
And it even works :-)
Hmm... I should try and stick the format string into the __bug_table, its const after all. Then I can get 2 arguments covered.
--- diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index f0e9acf72547..88b305d49f35 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -5,6 +5,7 @@ #include <linux/stringify.h> #include <linux/instrumentation.h> #include <linux/objtool.h> +#include <linux/args.h>
/* * Despite that some emulators terminate on UD2, we use it for WARN(). @@ -28,50 +29,44 @@ #define BUG_UD1_UBSAN 0xfffc #define BUG_EA 0xffea #define BUG_LOCK 0xfff0 +#define BUG_WARN 0xfe00 +#define BUG_WARN_END 0xfeff
#ifdef CONFIG_GENERIC_BUG
#ifdef CONFIG_X86_32 -# define __BUG_REL(val) ".long " __stringify(val) +#define ASM_BUG_REL(val) .long val #else -# define __BUG_REL(val) ".long " __stringify(val) " - ." +#define ASM_BUG_REL(val) .long val - . #endif
#ifdef CONFIG_DEBUG_BUGVERBOSE +#define ASM_BUGTABLE_VERBOSE(file, line) \ + ASM_BUG_REL(file) ; \ + .word line +#define ASM_BUGTABLE_VERBOSE_SIZE 6 +#else +#define ASM_BUGTABLE_VERBOSE(file, line) +#define ASM_BUGTABLE_VERBOSE_SIZE 0 +#endif
-#define _BUG_FLAGS(ins, flags, extra) \ -do { \ - asm_inline volatile("1:\t" ins "\n" \ - ".pushsection __bug_table,"aw"\n" \ - "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ - "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \ - "\t.word %c1" "\t# bug_entry::line\n" \ - "\t.word %c2" "\t# bug_entry::flags\n" \ - "\t.org 2b+%c3\n" \ - ".popsection\n" \ - extra \ - : : "i" (__FILE__), "i" (__LINE__), \ - "i" (flags), \ - "i" (sizeof(struct bug_entry))); \ -} while (0) - -#else /* !CONFIG_DEBUG_BUGVERBOSE */ +#define ASM_BUGTABLE_FLAGS(at, file, line, flags) \ + .pushsection __bug_table, "aw" ; \ + 123: ASM_BUG_REL(at) ; \ + ASM_BUGTABLE_VERBOSE(file, line) ; \ + .word flags ; \ + .org 123b + 6 + ASM_BUGTABLE_VERBOSE_SIZE ; \ + .popsection
-#define _BUG_FLAGS(ins, flags, extra) \ +#define _BUG_FLAGS(ins, flags, extra, extra_args...) \ do { \ asm_inline volatile("1:\t" ins "\n" \ - ".pushsection __bug_table,"aw"\n" \ - "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ - "\t.word %c0" "\t# bug_entry::flags\n" \ - "\t.org 2b+%c1\n" \ - ".popsection\n" \ - extra \ - : : "i" (flags), \ - "i" (sizeof(struct bug_entry))); \ + __stringify(ASM_BUGTABLE_FLAGS(1b, %c0, %c1, %c2)) "\n" \ + extra \ + : : "i" (__FILE__), "i" (__LINE__), \ + "i" (flags), ## extra_args); \ } while (0)
-#endif /* CONFIG_DEBUG_BUGVERBOSE */ - #else
#define _BUG_FLAGS(ins, flags, extra) asm volatile(ins) @@ -100,6 +95,40 @@ do { \ instrumentation_end(); \ } while (0)
+#define __WARN_printf_1(taint, format) \ +do { \ + __auto_type __flags = BUGFLAG_WARNING | BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint); \ + unsigned long dummy = 0; \ + instrumentation_begin(); \ + asm_inline volatile("1: ud1 %[fmt], %[arg]\n" \ + __stringify(ASM_BUGTABLE_FLAGS(1b, %c0, %c1, %c2)) "\n" \ + : : "i" (__FILE__), "i" (__LINE__), \ + "i" (__flags), [fmt] "r" (format), [arg] "r" (dummy)); \ + instrumentation_end(); \ +} while (0) + +#define __WARN_printf_2(taint, format, _arg) \ +do { \ + __auto_type __flags = BUGFLAG_WARNING | BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint); \ + instrumentation_begin(); \ + asm_inline volatile("1: ud1 %[fmt], %[arg]\n" \ + __stringify(ASM_BUGTABLE_FLAGS(1b, %c0, %c1, %c2)) "\n" \ + : : "i" (__FILE__), "i" (__LINE__), \ + "i" (__flags), [fmt] "r" (format), [arg] "r" ((unsigned long)(_arg))); \ + instrumentation_end(); \ +} while (0) + +#define __WARN_printf_n(taint, fmt, arg...) do { \ + instrumentation_begin(); \ + __warn_printk(fmt, arg); \ + __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ + instrumentation_end(); \ + } while (0) + +#define WARN_ARGS(X...) __COUNT_ARGS(, ##X, n, n, n, n, n, n, n, n, n, n, n, n, n, 2, 1, 0) + +#define __WARN_printf(taint, arg...) CONCATENATE(__WARN_printf_, WARN_ARGS(arg))(taint, arg) + #include <asm-generic/bug.h>
#endif /* _ASM_X86_BUG_H */ diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 94c0236963c6..b7f69f4addf4 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -81,18 +81,6 @@
DECLARE_BITMAP(system_vectors, NR_VECTORS);
-__always_inline int is_valid_bugaddr(unsigned long addr) -{ - if (addr < TASK_SIZE_MAX) - return 0; - - /* - * We got #UD, if the text isn't readable we'd have gotten - * a different exception. - */ - return *(unsigned short *)addr == INSN_UD2; -} - /* * Check for UD1 or UD2, accounting for Address Size Override Prefixes. * If it's a UD1, further decode to determine its use: @@ -102,25 +90,37 @@ __always_inline int is_valid_bugaddr(unsigned long addr) * UBSan{0}: 67 0f b9 00 ud1 (%eax),%eax * UBSan{10}: 67 0f b9 40 10 ud1 0x10(%eax),%eax * static_call: 0f b9 cc ud1 %esp,%ecx + * WARN_printf: ud1 %reg,%reg * - * Notably UBSAN uses EAX, static_call uses ECX. + * Notably UBSAN uses (%eax), static_call uses %esp,%ecx */ __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) { unsigned long start = addr; + u8 v, rex = 0, reg, rm; bool lock = false; - u8 v; + int type = BUG_UD1;
if (addr < TASK_SIZE_MAX) return BUG_NONE;
- v = *(u8 *)(addr++); - if (v == INSN_ASOP) + for (;;) { v = *(u8 *)(addr++);
- if (v == INSN_LOCK) { - lock = true; - v = *(u8 *)(addr++); + if (v == INSN_ASOP) + continue; + + if (v == INSN_LOCK) { + lock = true; + continue; + } + + if ((v & 0xf0) == 0x40) { + rex = v; + continue; + } + + break; }
switch (v) { @@ -156,9 +156,13 @@ __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) if (X86_MODRM_MOD(v) != 3 && X86_MODRM_RM(v) == 4) addr++; /* SIB */
+ reg = X86_MODRM_REG(v) + 8*!!X86_REX_R(rex); + rm = X86_MODRM_RM(v) + 8*!!X86_REX_B(rex); + /* Decode immediate, if present */ switch (X86_MODRM_MOD(v)) { - case 0: if (X86_MODRM_RM(v) == 5) + case 0: *imm = 0; + if (X86_MODRM_RM(v) == 5) addr += 4; /* RIP + disp32 */ break;
@@ -170,18 +174,37 @@ __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) addr += 4; break;
- case 3: break; + case 3: if (rm != 4) /* %esp */ + type = BUG_WARN | (rm << 4) | reg; + break; }
/* record instruction length */ *len = addr - start;
- if (X86_MODRM_REG(v) == 0) /* EAX */ + if (!rm && X86_MODRM_MOD(v) != 3) /* (%eax) */ return BUG_UD1_UBSAN;
- return BUG_UD1; + return type; }
+int is_valid_bugaddr(unsigned long addr) +{ + int ud_type, ud_len; + u32 ud_imm; + + if (addr < TASK_SIZE_MAX) + return 0; + + /* + * We got #UD, if the text isn't readable we'd have gotten + * a different exception. + */ + ud_type = decode_bug(addr, &ud_imm, &ud_len); + + return ud_type == BUG_UD2 || + (ud_type >= BUG_WARN && ud_type <= BUG_WARN_END); +}
static nokprobe_inline int do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str, @@ -305,6 +328,14 @@ static inline void handle_invalid_op(struct pt_regs *regs) ILL_ILLOPN, error_get_trap_addr(regs)); }
+static inline unsigned long pt_regs_val(struct pt_regs *regs, int nr) +{ + int offset = pt_regs_offset(regs, nr); + if (WARN_ON_ONCE(offset < -0)) + return 0; + return *((unsigned long *)((void *)regs + offset)); +} + static noinstr bool handle_bug(struct pt_regs *regs) { unsigned long addr = regs->ip; @@ -334,6 +365,14 @@ static noinstr bool handle_bug(struct pt_regs *regs) raw_local_irq_enable();
switch (ud_type) { + case BUG_WARN ... BUG_WARN_END: + int ud_reg = ud_type & 0xf; + int ud_rm = (ud_type >> 4) & 0xf; + + __warn_printk((const char *)(pt_regs_val(regs, ud_rm)), + pt_regs_val(regs, ud_reg)); + fallthrough; + case BUG_UD2: if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) { handled = true; diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 387720933973..a5960c92d70a 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -101,12 +101,16 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); } while (0) #else #define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) + +#ifndef __WARN_printf #define __WARN_printf(taint, arg...) do { \ instrumentation_begin(); \ __warn_printk(arg); \ __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ instrumentation_end(); \ } while (0) +#endif + #define WARN_ON_ONCE(condition) ({ \ int __ret_warn_on = !!(condition); \ if (unlikely(__ret_warn_on)) \ diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 62b3416f5e43..564513f605ac 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -8703,6 +8703,8 @@ void __init sched_init(void) preempt_dynamic_init();
scheduler_running = 1; + + WARN(true, "Ultimate answer: %d\n", 42); }
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP
On Fri, May 30, 2025 at 04:01:40PM +0200, Peter Zijlstra wrote:
I'm not really concerned with performance here, but more with the size of the code emitted by WARN_ONCE(). There are a *ton* of WARN sites, while only one report_bug() and printk().
The really offensive thing is that this is for a feature most nobody will ever need :/
Well, it won't be enabled often -- this reminds me of ftrace: it needs to work, but it'll be off most of the time.
The below results in:
03dc 7ac: 48 c7 c0 00 00 00 00 mov $0x0,%rax 7af: R_X86_64_32S .rodata.str1.1+0x223 03e3 7b3: ba 2a 00 00 00 mov $0x2a,%edx 03e8 7b8: 48 0f b9 d0 ud1 %rax,%rdx
And it even works :-)
Hmm... I should try and stick the format string into the __bug_table, its const after all. Then I can get 2 arguments covered.
I like the patch! Can you add a _little_ documentation, though? e.g. explaining that BUG_WARN ... BUG_WARN_END is for format string args, etc.
But yes, I *love* that this moves the printk into the handler! Like you, I have been bothered by this for a long time and could not find a good way to do it. This is nice.
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index f0e9acf72547..88b305d49f35 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -5,6 +5,7 @@ #include <linux/stringify.h> #include <linux/instrumentation.h> #include <linux/objtool.h> +#include <linux/args.h> /*
- Despite that some emulators terminate on UD2, we use it for WARN().
@@ -28,50 +29,44 @@ #define BUG_UD1_UBSAN 0xfffc #define BUG_EA 0xffea #define BUG_LOCK 0xfff0 +#define BUG_WARN 0xfe00 +#define BUG_WARN_END 0xfeff #ifdef CONFIG_GENERIC_BUG #ifdef CONFIG_X86_32 -# define __BUG_REL(val) ".long " __stringify(val) +#define ASM_BUG_REL(val) .long val #else -# define __BUG_REL(val) ".long " __stringify(val) " - ." +#define ASM_BUG_REL(val) .long val - . #endif #ifdef CONFIG_DEBUG_BUGVERBOSE +#define ASM_BUGTABLE_VERBOSE(file, line) \
- ASM_BUG_REL(file) ; \
- .word line
+#define ASM_BUGTABLE_VERBOSE_SIZE 6 +#else +#define ASM_BUGTABLE_VERBOSE(file, line) +#define ASM_BUGTABLE_VERBOSE_SIZE 0 +#endif -#define _BUG_FLAGS(ins, flags, extra) \ -do { \
- asm_inline volatile("1:\t" ins "\n" \
".pushsection __bug_table,\"aw\"\n" \"2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \"\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \"\t.word %c1" "\t# bug_entry::line\n" \"\t.word %c2" "\t# bug_entry::flags\n" \"\t.org 2b+%c3\n" \".popsection\n" \extra \: : "i" (__FILE__), "i" (__LINE__), \"i" (flags), \"i" (sizeof(struct bug_entry))); \-} while (0)
-#else /* !CONFIG_DEBUG_BUGVERBOSE */ +#define ASM_BUGTABLE_FLAGS(at, file, line, flags) \
- .pushsection __bug_table, "aw" ; \
- 123: ASM_BUG_REL(at) ; \
- ASM_BUGTABLE_VERBOSE(file, line) ; \
- .word flags ; \
- .org 123b + 6 + ASM_BUGTABLE_VERBOSE_SIZE ; \
- .popsection
-#define _BUG_FLAGS(ins, flags, extra) \ +#define _BUG_FLAGS(ins, flags, extra, extra_args...) \ do { \ asm_inline volatile("1:\t" ins "\n" \
".pushsection __bug_table,\"aw\"\n" \"2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \"\t.word %c0" "\t# bug_entry::flags\n" \"\t.org 2b+%c1\n" \".popsection\n" \extra \: : "i" (flags), \"i" (sizeof(struct bug_entry))); \
__stringify(ASM_BUGTABLE_FLAGS(1b, %c0, %c1, %c2)) "\n" \extra \: : "i" (__FILE__), "i" (__LINE__), \"i" (flags), ## extra_args); \} while (0) -#endif /* CONFIG_DEBUG_BUGVERBOSE */
#else #define _BUG_FLAGS(ins, flags, extra) asm volatile(ins) @@ -100,6 +95,40 @@ do { \ instrumentation_end(); \ } while (0) +#define __WARN_printf_1(taint, format) \ +do { \
- __auto_type __flags = BUGFLAG_WARNING | BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint); \
- unsigned long dummy = 0; \
- instrumentation_begin(); \
- asm_inline volatile("1: ud1 %[fmt], %[arg]\n" \
__stringify(ASM_BUGTABLE_FLAGS(1b, %c0, %c1, %c2)) "\n" \: : "i" (__FILE__), "i" (__LINE__), \"i" (__flags), [fmt] "r" (format), [arg] "r" (dummy)); \- instrumentation_end(); \
+} while (0)
+#define __WARN_printf_2(taint, format, _arg) \ +do { \
- __auto_type __flags = BUGFLAG_WARNING | BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint); \
- instrumentation_begin(); \
- asm_inline volatile("1: ud1 %[fmt], %[arg]\n" \
__stringify(ASM_BUGTABLE_FLAGS(1b, %c0, %c1, %c2)) "\n" \: : "i" (__FILE__), "i" (__LINE__), \"i" (__flags), [fmt] "r" (format), [arg] "r" ((unsigned long)(_arg))); \- instrumentation_end(); \
+} while (0)
+#define __WARN_printf_n(taint, fmt, arg...) do { \
instrumentation_begin(); \__warn_printk(fmt, arg); \__WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\instrumentation_end(); \- } while (0)
+#define WARN_ARGS(X...) __COUNT_ARGS(, ##X, n, n, n, n, n, n, n, n, n, n, n, n, n, 2, 1, 0)
+#define __WARN_printf(taint, arg...) CONCATENATE(__WARN_printf_, WARN_ARGS(arg))(taint, arg)
This needs docs too. I think this is collapsing 1 and 2 argument WARNs into the ud1, and anything larger is explicitly calling __warn_printk + __WARN_FLAGS? If only 1 and 2 args can be collapsed, why reserve 0xfe00 through 0xfeff? I feel like I'm missing something here...
#include <asm-generic/bug.h> #endif /* _ASM_X86_BUG_H */ diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 94c0236963c6..b7f69f4addf4 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -81,18 +81,6 @@ DECLARE_BITMAP(system_vectors, NR_VECTORS); -__always_inline int is_valid_bugaddr(unsigned long addr) -{
- if (addr < TASK_SIZE_MAX)
return 0;- /*
* We got #UD, if the text isn't readable we'd have gotten* a different exception.*/- return *(unsigned short *)addr == INSN_UD2;
-}
/*
- Check for UD1 or UD2, accounting for Address Size Override Prefixes.
- If it's a UD1, further decode to determine its use:
@@ -102,25 +90,37 @@ __always_inline int is_valid_bugaddr(unsigned long addr)
- UBSan{0}: 67 0f b9 00 ud1 (%eax),%eax
- UBSan{10}: 67 0f b9 40 10 ud1 0x10(%eax),%eax
- static_call: 0f b9 cc ud1 %esp,%ecx
- WARN_printf: ud1 %reg,%reg
- Notably UBSAN uses EAX, static_call uses ECX.
*/
- Notably UBSAN uses (%eax), static_call uses %esp,%ecx
__always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) { unsigned long start = addr;
- u8 v, rex = 0, reg, rm; bool lock = false;
- u8 v;
- int type = BUG_UD1;
if (addr < TASK_SIZE_MAX) return BUG_NONE;
- v = *(u8 *)(addr++);
- if (v == INSN_ASOP)
- for (;;) { v = *(u8 *)(addr++);
- if (v == INSN_LOCK) {
lock = true;v = *(u8 *)(addr++);
if (v == INSN_ASOP)continue;if (v == INSN_LOCK) {lock = true;continue;}if ((v & 0xf0) == 0x40) {rex = v;continue;}break;switch (v) { @@ -156,9 +156,13 @@ __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) if (X86_MODRM_MOD(v) != 3 && X86_MODRM_RM(v) == 4) addr++; /* SIB */
- reg = X86_MODRM_REG(v) + 8*!!X86_REX_R(rex);
- rm = X86_MODRM_RM(v) + 8*!!X86_REX_B(rex);
- /* Decode immediate, if present */ switch (X86_MODRM_MOD(v)) {
- case 0: if (X86_MODRM_RM(v) == 5)
- case 0: *imm = 0;
if (X86_MODRM_RM(v) == 5) addr += 4; /* RIP + disp32 */@@ -170,18 +174,37 @@ __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) addr += 4; break;
- case 3: break;
- case 3: if (rm != 4) /* %esp */
type = BUG_WARN | (rm << 4) | reg;break;/* record instruction length */ *len = addr - start;
- if (X86_MODRM_REG(v) == 0) /* EAX */
- if (!rm && X86_MODRM_MOD(v) != 3) /* (%eax) */ return BUG_UD1_UBSAN;
- return BUG_UD1;
- return type;
} +int is_valid_bugaddr(unsigned long addr) +{
- int ud_type, ud_len;
- u32 ud_imm;
- if (addr < TASK_SIZE_MAX)
return 0;- /*
* We got #UD, if the text isn't readable we'd have gotten* a different exception.*/- ud_type = decode_bug(addr, &ud_imm, &ud_len);
- return ud_type == BUG_UD2 ||
(ud_type >= BUG_WARN && ud_type <= BUG_WARN_END);+} static nokprobe_inline int do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str, @@ -305,6 +328,14 @@ static inline void handle_invalid_op(struct pt_regs *regs) ILL_ILLOPN, error_get_trap_addr(regs)); } +static inline unsigned long pt_regs_val(struct pt_regs *regs, int nr) +{
- int offset = pt_regs_offset(regs, nr);
- if (WARN_ON_ONCE(offset < -0))
return 0;- return *((unsigned long *)((void *)regs + offset));
+}
static noinstr bool handle_bug(struct pt_regs *regs) { unsigned long addr = regs->ip; @@ -334,6 +365,14 @@ static noinstr bool handle_bug(struct pt_regs *regs) raw_local_irq_enable(); switch (ud_type) {
- case BUG_WARN ... BUG_WARN_END:
int ud_reg = ud_type & 0xf;int ud_rm = (ud_type >> 4) & 0xf;__warn_printk((const char *)(pt_regs_val(regs, ud_rm)),pt_regs_val(regs, ud_reg));fallthrough;
Yay, internal printk. :):)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 62b3416f5e43..564513f605ac 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -8703,6 +8703,8 @@ void __init sched_init(void) preempt_dynamic_init(); scheduler_running = 1;
- WARN(true, "Ultimate answer: %d\n", 42);
} #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
If any code would emit The Answer, it would be the scheduler. :)
On Fri, May 30, 2025 at 10:48:47AM -0700, Kees Cook wrote:
On Fri, May 30, 2025 at 04:01:40PM +0200, Peter Zijlstra wrote:
I'm not really concerned with performance here, but more with the size of the code emitted by WARN_ONCE(). There are a *ton* of WARN sites, while only one report_bug() and printk().
The really offensive thing is that this is for a feature most nobody will ever need :/
Well, it won't be enabled often -- this reminds me of ftrace: it needs to work, but it'll be off most of the time.
Well, ftrace is useful, but when would I *ever* care about this stuff? I can't operate kunit, don't give a crap about kunit, and if I were to magically run it, I would be more than capable of ignoring WARNs.
I like the patch! Can you add a _little_ documentation, though? e.g. explaining that BUG_WARN ... BUG_WARN_END is for format string args, etc.
Cleaned it up a little bit... I'll add some comments on later :-)
I also need to go fix WARN_ONCE(), at least for the n<=2 cases that can use BUGFLAG_ONCE now.
--- diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index f0e9acf72547..3aecedf46d0c 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h @@ -5,6 +5,7 @@ #include <linux/stringify.h> #include <linux/instrumentation.h> #include <linux/objtool.h> +#include <linux/args.h>
/* * Despite that some emulators terminate on UD2, we use it for WARN(). @@ -26,52 +27,52 @@ #define BUG_UD2 0xfffe #define BUG_UD1 0xfffd #define BUG_UD1_UBSAN 0xfffc +#define BUG_UD1_WARN 0xfffb #define BUG_EA 0xffea #define BUG_LOCK 0xfff0
#ifdef CONFIG_GENERIC_BUG
+#define BUG_HAS_FORMAT + #ifdef CONFIG_X86_32 -# define __BUG_REL(val) ".long " __stringify(val) +#define ASM_BUG_REL(val) .long val #else -# define __BUG_REL(val) ".long " __stringify(val) " - ." +#define ASM_BUG_REL(val) .long val - . #endif
#ifdef CONFIG_DEBUG_BUGVERBOSE +#define ASM_BUGTABLE_VERBOSE(file, line) \ + ASM_BUG_REL(file) ; \ + .word line +#define ASM_BUGTABLE_VERBOSE_SIZE 6 +#else +#define ASM_BUGTABLE_VERBOSE(file, line) +#define ASM_BUGTABLE_VERBOSE_SIZE 0 +#endif
-#define _BUG_FLAGS(ins, flags, extra) \ -do { \ - asm_inline volatile("1:\t" ins "\n" \ - ".pushsection __bug_table,"aw"\n" \ - "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ - "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \ - "\t.word %c1" "\t# bug_entry::line\n" \ - "\t.word %c2" "\t# bug_entry::flags\n" \ - "\t.org 2b+%c3\n" \ - ".popsection\n" \ - extra \ - : : "i" (__FILE__), "i" (__LINE__), \ - "i" (flags), \ - "i" (sizeof(struct bug_entry))); \ -} while (0) +#define ASM_BUGTABLE_FORMAT(format) \ + ASM_BUG_REL(format)
-#else /* !CONFIG_DEBUG_BUGVERBOSE */ +#define ASM_BUGTABLE_FLAGS(at, format, file, line, flags) \ + .pushsection __bug_table, "aw" ; \ + 123: ASM_BUG_REL(at) ; \ + ASM_BUGTABLE_FORMAT(format) ; \ + ASM_BUGTABLE_VERBOSE(file, line) ; \ + .word flags ; \ + .org 123b + 6 + 4 + ASM_BUGTABLE_VERBOSE_SIZE ; \ + .popsection
#define _BUG_FLAGS(ins, flags, extra) \ do { \ asm_inline volatile("1:\t" ins "\n" \ - ".pushsection __bug_table,"aw"\n" \ - "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \ - "\t.word %c0" "\t# bug_entry::flags\n" \ - "\t.org 2b+%c1\n" \ - ".popsection\n" \ - extra \ - : : "i" (flags), \ - "i" (sizeof(struct bug_entry))); \ + __stringify(ASM_BUGTABLE_FLAGS(1b, %c[fmt], %c[file], %c[line], %c[fl])) "\n" \ + extra \ + : : [fmt] "i" (NULL), [file] "i" (__FILE__), \ + [line] "i" (__LINE__), \ + [fl] "i" (flags)); \ } while (0)
-#endif /* CONFIG_DEBUG_BUGVERBOSE */ - #else
#define _BUG_FLAGS(ins, flags, extra) asm volatile(ins) @@ -100,6 +101,62 @@ do { \ instrumentation_end(); \ } while (0)
+#ifndef __ASSEMBLER__ +struct pt_regs; +extern void __warn_printf(const char *fmt, struct pt_regs *regs); +#define __warn_printf __warn_printf +#endif + +#define __WARN_printf_0(taint, format) \ +do { \ + __auto_type __flags = BUGFLAG_WARNING | BUGFLAG_FORMAT | BUGFLAG_TAINT(taint); \ + instrumentation_begin(); \ + asm_inline volatile("1: ud2\n" \ + ANNOTATE_REACHABLE(1b) \ + __stringify(ASM_BUGTABLE_FLAGS(1b, %c[fmt], %c[file], %c[line], %c[fl])) "\n" \ + : : [file] "i" (__FILE__), [line] "i" (__LINE__), \ + [fl] "i" (__flags), [fmt] "i" (format)); \ + instrumentation_end(); \ +} while (0) + +#define __WARN_printf_1(taint, format, arg1) \ +do { \ + __auto_type __flags = BUGFLAG_WARNING | BUGFLAG_FORMAT | BUGFLAG_TAINT(taint); \ + instrumentation_begin(); \ + asm_inline volatile("1: ud1 (%%rcx), %[a1]\n" \ + ANNOTATE_REACHABLE(1b) \ + __stringify(ASM_BUGTABLE_FLAGS(1b, %c[fmt], %c[file], %c[line], %c[fl])) "\n" \ + : : [file] "i" (__FILE__), [line] "i" (__LINE__), \ + [fl] "i" (__flags), [fmt] "i" (format), \ + [a1] "r" ((unsigned long)(arg1))); \ + instrumentation_end(); \ +} while (0) + +#define __WARN_printf_2(taint, format, arg1, arg2) \ +do { \ + __auto_type __flags = BUGFLAG_WARNING | BUGFLAG_FORMAT | BUGFLAG_TAINT(taint); \ + instrumentation_begin(); \ + asm_inline volatile("1: ud1 %[a2], %[a1]\n" \ + ANNOTATE_REACHABLE(1b) \ + __stringify(ASM_BUGTABLE_FLAGS(1b, %c[fmt], %c[file], %c[line], %c[fl])) "\n" \ + : : [file] "i" (__FILE__), [line] "i" (__LINE__), \ + [fl] "i" (__flags), [fmt] "i" (format), \ + [a1] "r" ((unsigned long)(arg1)), \ + [a2] "r" ((unsigned long)(arg2))); \ + instrumentation_end(); \ +} while (0) + +#define __WARN_printf_n(taint, fmt, arg...) do { \ + instrumentation_begin(); \ + __warn_printk(fmt, arg); \ + __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ + instrumentation_end(); \ + } while (0) + +#define WARN_ARGS(X...) __COUNT_ARGS(, ##X, n, n, n, n, n, n, n, n, n, n, n, n, n, 2, 1, 0) + +#define __WARN_printf(taint, fmt, arg...) CONCATENATE(__WARN_printf_, WARN_ARGS(arg))(taint, fmt, ## arg) + #include <asm-generic/bug.h>
#endif /* _ASM_X86_BUG_H */ diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c index 94c0236963c6..bb9193fd3d2d 100644 --- a/arch/x86/kernel/traps.c +++ b/arch/x86/kernel/traps.c @@ -81,18 +81,6 @@
DECLARE_BITMAP(system_vectors, NR_VECTORS);
-__always_inline int is_valid_bugaddr(unsigned long addr) -{ - if (addr < TASK_SIZE_MAX) - return 0; - - /* - * We got #UD, if the text isn't readable we'd have gotten - * a different exception. - */ - return *(unsigned short *)addr == INSN_UD2; -} - /* * Check for UD1 or UD2, accounting for Address Size Override Prefixes. * If it's a UD1, further decode to determine its use: @@ -103,24 +91,39 @@ __always_inline int is_valid_bugaddr(unsigned long addr) * UBSan{10}: 67 0f b9 40 10 ud1 0x10(%eax),%eax * static_call: 0f b9 cc ud1 %esp,%ecx * - * Notably UBSAN uses EAX, static_call uses ECX. + * WARN_printf_0: ud2 + * WARN_printf_1: ud1 (%ecx),%reg + * WARN_printf_2: ud1 %reg,%reg + * + * Notably UBSAN uses (%eax), static_call does not get a bug_entry. */ -__always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) +__always_inline int decode_bug(unsigned long addr, u32 *regs, s32 *imm, int *len) { unsigned long start = addr; + u8 v, rex = 0, reg, rm; bool lock = false; - u8 v; + int type = BUG_UD1;
if (addr < TASK_SIZE_MAX) return BUG_NONE;
- v = *(u8 *)(addr++); - if (v == INSN_ASOP) + for (;;) { v = *(u8 *)(addr++);
- if (v == INSN_LOCK) { - lock = true; - v = *(u8 *)(addr++); + if (v == INSN_ASOP) + continue; + + if (v == INSN_LOCK) { + lock = true; + continue; + } + + if ((v & 0xf0) == 0x40) { + rex = v; + continue; + } + + break; }
switch (v) { @@ -141,6 +144,9 @@ __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) return BUG_NONE; }
+ *regs = 0; + *imm = 0; + v = *(u8 *)(addr++); if (v == SECOND_BYTE_OPCODE_UD2) { *len = addr - start; @@ -150,16 +156,22 @@ __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) if (v != SECOND_BYTE_OPCODE_UD1) return BUG_NONE;
- *imm = 0; v = *(u8 *)(addr++); /* ModRM */ - if (X86_MODRM_MOD(v) != 3 && X86_MODRM_RM(v) == 4) addr++; /* SIB */
+ reg = X86_MODRM_REG(v) + 8*!!X86_REX_R(rex); + rm = X86_MODRM_RM(v) + 8*!!X86_REX_B(rex); + *regs = (rm << 4) | reg; + /* Decode immediate, if present */ switch (X86_MODRM_MOD(v)) { case 0: if (X86_MODRM_RM(v) == 5) - addr += 4; /* RIP + disp32 */ + addr += 4; /* RIP + disp32 */ + if (rm == 1) { /* CX */ + type = BUG_UD1_WARN; + *regs |= 0x100; + } break;
case 1: *imm = *(s8 *)addr; @@ -170,18 +182,37 @@ __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len) addr += 4; break;
- case 3: break; + case 3: type = BUG_UD1_WARN; + *regs |= 0x300; + break; }
/* record instruction length */ *len = addr - start;
- if (X86_MODRM_REG(v) == 0) /* EAX */ + if (!rm && X86_MODRM_MOD(v) != 3) /* (%eax) */ return BUG_UD1_UBSAN;
- return BUG_UD1; + return type; }
+int is_valid_bugaddr(unsigned long addr) +{ + int ud_type, ud_len; + u32 ud_regs; + s32 ud_imm; + + if (addr < TASK_SIZE_MAX) + return 0; + + /* + * We got #UD, if the text isn't readable we'd have gotten + * a different exception. + */ + ud_type = decode_bug(addr, &ud_regs, &ud_imm, &ud_len); + + return ud_type == BUG_UD2 || ud_type == BUG_UD1_WARN; +}
static nokprobe_inline int do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str, @@ -305,14 +336,42 @@ static inline void handle_invalid_op(struct pt_regs *regs) ILL_ILLOPN, error_get_trap_addr(regs)); }
+static inline unsigned long pt_regs_val(struct pt_regs *regs, int nr) +{ + int offset = pt_regs_offset(regs, nr); + if (WARN_ON_ONCE(offset < -0)) + return 0; + return *((unsigned long *)((void *)regs + offset)); +} + +void __warn_printf(const char *fmt, struct pt_regs *regs) +{ + u32 r = regs->orig_ax; + unsigned long a1 = pt_regs_val(regs, (r >> 0) & 0xf); + unsigned long a2 = pt_regs_val(regs, (r >> 4) & 0xf); + + switch ((r >> 8) & 0x3) { + case 0: + printk(fmt); + return; + case 1: + printk(fmt, a1); + return; + case 3: + printk(fmt, a1, a2); + return; + } +} + static noinstr bool handle_bug(struct pt_regs *regs) { unsigned long addr = regs->ip; bool handled = false; int ud_type, ud_len; + u32 ud_regs; s32 ud_imm;
- ud_type = decode_bug(addr, &ud_imm, &ud_len); + ud_type = decode_bug(addr, &ud_regs, &ud_imm, &ud_len); if (ud_type == BUG_NONE) return handled;
@@ -334,7 +393,9 @@ static noinstr bool handle_bug(struct pt_regs *regs) raw_local_irq_enable();
switch (ud_type) { + case BUG_UD1_WARN: case BUG_UD2: + regs->orig_ax = ud_regs; if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) { handled = true; break; diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 387720933973..b8336259f81d 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -13,6 +13,7 @@ #define BUGFLAG_ONCE (1 << 1) #define BUGFLAG_DONE (1 << 2) #define BUGFLAG_NO_CUT_HERE (1 << 3) /* CUT_HERE already sent */ +#define BUGFLAG_FORMAT (1 << 4) #define BUGFLAG_TAINT(taint) ((taint) << 8) #define BUG_GET_TAINT(bug) ((bug)->flags >> 8) #endif @@ -36,6 +37,13 @@ struct bug_entry { #else signed int bug_addr_disp; #endif +#ifdef BUG_HAS_FORMAT +#ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS + const char *format; +#else + signed int format_disp; +#endif +#endif #ifdef CONFIG_DEBUG_BUGVERBOSE #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS const char *file; @@ -101,12 +109,16 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); } while (0) #else #define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) + +#ifndef __WARN_printf #define __WARN_printf(taint, arg...) do { \ instrumentation_begin(); \ __warn_printk(arg); \ __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\ instrumentation_end(); \ } while (0) +#endif + #define WARN_ON_ONCE(condition) ({ \ int __ret_warn_on = !!(condition); \ if (unlikely(__ret_warn_on)) \ @@ -114,6 +126,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); BUGFLAG_TAINT(TAINT_WARN)); \ unlikely(__ret_warn_on); \ }) + #endif
/* used internally by panic.c */ @@ -148,8 +161,10 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); DO_ONCE_LITE_IF(condition, WARN_ON, 1) #endif
+#ifndef WARN_ONCE #define WARN_ONCE(condition, format...) \ DO_ONCE_LITE_IF(condition, WARN, 1, format) +#endif
#define WARN_TAINT_ONCE(condition, taint, format...) \ DO_ONCE_LITE_IF(condition, WARN_TAINT, 1, taint, format) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 62b3416f5e43..4f7bc0e500ba 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -8703,6 +8703,10 @@ void __init sched_init(void) preempt_dynamic_init();
scheduler_running = 1; + + WARN(true, "Ultimate answer: %d\n", 42); + WARN(true, "XXX2 %d %d\n", 43, 44); + WARN(true, "XXX3 %d %d %d\n", 45, 46, 47); }
#ifdef CONFIG_DEBUG_ATOMIC_SLEEP diff --git a/lib/bug.c b/lib/bug.c index b1f07459c2ee..085889e9c096 100644 --- a/lib/bug.c +++ b/lib/bug.c @@ -139,6 +139,17 @@ void bug_get_file_line(struct bug_entry *bug, const char **file, #endif }
+static const char *bug_get_format(struct bug_entry *bug) +{ + const char *format = NULL; +#ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS + format = (const char *)&bug->format_disp + bug->format_disp; +#else + format = bug->format; +#endif + return format; +} + struct bug_entry *find_bug(unsigned long bugaddr) { struct bug_entry *bug; @@ -150,6 +161,14 @@ struct bug_entry *find_bug(unsigned long bugaddr) return module_find_bug(bugaddr); }
+#ifndef __warn_printf +static void __warn_printf(const char *fmt, struct pt_regs *regs) +{ + printk(fmt); +} +#define __warn_printf __warn_printf +#endif + static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *regs) { struct bug_entry *bug; @@ -190,6 +209,9 @@ static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *re if ((bug->flags & BUGFLAG_NO_CUT_HERE) == 0) printk(KERN_DEFAULT CUT_HERE);
+ if (bug->flags & BUGFLAG_FORMAT) + __warn_printf(bug_get_format(bug), regs); + if (warning) { /* this is a WARN_ON rather than BUG/BUG_ON */ __warn(file, line, (void *)bugaddr, BUG_GET_TAINT(bug), regs,
On May 31, 2025 3:23:04 AM PDT, Peter Zijlstra peterz@infradead.org wrote:
On Fri, May 30, 2025 at 10:48:47AM -0700, Kees Cook wrote:
On Fri, May 30, 2025 at 04:01:40PM +0200, Peter Zijlstra wrote:
I'm not really concerned with performance here, but more with the size of the code emitted by WARN_ONCE(). There are a *ton* of WARN sites, while only one report_bug() and printk().
The really offensive thing is that this is for a feature most nobody will ever need :/
Well, it won't be enabled often -- this reminds me of ftrace: it needs to work, but it'll be off most of the time.
Well, ftrace is useful, but when would I *ever* care about this stuff? I can't operate kunit, don't give a crap about kunit, and if I were to magically run it, I would be more than capable of ignoring WARNs.
It's not for you, then. :) I can't operate ftrace, but I use kunit almost daily. Ignoring WARNs makes this much nicer, and especially for CIs.
Cleaned it up a little bit... I'll add some comments on later :-)
I also need to go fix WARN_ONCE(), at least for the n<=2 cases that can use BUGFLAG_ONCE now.
Cool! I'll expand the WARN tests in LKDTM so we can get wider behavioral and architectural coverage.
-Kees
On Sat, May 31, 2025 at 06:51:50AM -0700, Kees Cook wrote:
It's not for you, then. :) I can't operate ftrace, but I use kunit almost daily. Ignoring WARNs makes this much nicer, and especially for CIs.
I'm thinking you are more than capable of ignoring WARNs too. This leaves the CI thing.
So all this is really about telling CIs which WARNs are to be ignored, and which are not? Surely the easiest way to achieve that is by printing more/better identifying information instead of suppressing things?
On Mon, Jun 02, 2025 at 09:57:07AM +0200, Peter Zijlstra wrote:
On Sat, May 31, 2025 at 06:51:50AM -0700, Kees Cook wrote:
It's not for you, then. :) I can't operate ftrace, but I use kunit almost daily. Ignoring WARNs makes this much nicer, and especially for CIs.
I'm thinking you are more than capable of ignoring WARNs too. This leaves the CI thing.
So all this is really about telling CIs which WARNs are to be ignored, and which are not? Surely the easiest way to achieve that is by printing more/better identifying information instead of suppressing things?
You might also want to test that the warn is indeed emitted, and it not being emitted result in a test failure.
And I can see a future where we would fail a test that would trigger an unexpected WARN.
Doing either, or none, would be pretty terrible UX for !CI users too. How on earth would you know if the hundreds of WARN you got from the tests output are legitimate or not, and if you introduced new ones you're supposed to fix?
Maxime
On Mon, Jun 02, 2025 at 12:38:10PM +0200, Maxime Ripard wrote:
On Mon, Jun 02, 2025 at 09:57:07AM +0200, Peter Zijlstra wrote:
On Sat, May 31, 2025 at 06:51:50AM -0700, Kees Cook wrote:
It's not for you, then. :) I can't operate ftrace, but I use kunit almost daily. Ignoring WARNs makes this much nicer, and especially for CIs.
I'm thinking you are more than capable of ignoring WARNs too. This leaves the CI thing.
So all this is really about telling CIs which WARNs are to be ignored, and which are not? Surely the easiest way to achieve that is by printing more/better identifying information instead of suppressing things?
You might also want to test that the warn is indeed emitted, and it not being emitted result in a test failure.
And I can see a future where we would fail a test that would trigger an unexpected WARN.
Doing either, or none, would be pretty terrible UX for !CI users too. How on earth would you know if the hundreds of WARN you got from the tests output are legitimate or not, and if you introduced new ones you're supposed to fix?
Yeah we'd like to make sure that when drivers misuse subsystem api, things blow up. Kunit that makes sure we hit the warn we put in place for that seems like the best way to go about that, because in the past we've had cases where we thought we should have caught abuse but didn't. And this isn't the only thing we use, it's just one tool in the box among many others to keep the flood of driver issues at a manageable level.
Cheers, Sima
On Sat, May 31, 2025 at 12:23:04PM +0200, Peter Zijlstra wrote:
On Fri, May 30, 2025 at 10:48:47AM -0700, Kees Cook wrote:
On Fri, May 30, 2025 at 04:01:40PM +0200, Peter Zijlstra wrote:
I'm not really concerned with performance here, but more with the size of the code emitted by WARN_ONCE(). There are a *ton* of WARN sites, while only one report_bug() and printk().
The really offensive thing is that this is for a feature most nobody will ever need :/
Well, it won't be enabled often -- this reminds me of ftrace: it needs to work, but it'll be off most of the time.
Well, ftrace is useful, but when would I *ever* care about this stuff? I can't operate kunit
Why not?
don't give a crap about kunit
That's your choice, of course, and it might not be useful to you anyway, but it's *really* nice and closed a major gap in testing in some other areas.
I'd still encourage you to try it, it might be worth your time.
and if I were to magically run it, I would be more than capable of ignoring WARNs.
Yeah, it's not just about ignoring WARNs, but mostly about knowing which ones you can ignore, and which ones you should fix.
We're getting at a point (on some subsystems I guess) where we actually have a decent testing suite we can ask contributors to run and have all tests passing.
We also want to ask them to fix whatever issue they might introduce :)
Thanks for your help on getting a cleaner solution!
Maxime
On Mon, Jun 02, 2025 at 01:13:29PM +0200, Maxime Ripard wrote:
I can't operate kunit
Why not?
Too complicated. People have even wrecked tools/testing/selftests/ to the point that it is now nearly impossible to run the simple selftests :-(
And while I don't mind tests -- they're quite useful. Kunit just looks to make it all more complicated that it needs to be. Not to mention there seems to be snakes involved -- and I never can remember how that works.
Basically, if the stuff takes more effort to make run, than the time it runs for, its a loss. And in that respect much of the kernel testing stuff is a fail. Just too damn hard to make work.
I want to: make; ./run.sh or something similarly trivial. But clearly that is too much to task these days :-(
I spent almost a full day trying to get kvm selftests working a couple of weeks ago; that's time I don't have. And it makes me want to go hulk and smash things.
On Tue, Jun 3, 2025 at 9:26 PM Peter Zijlstra peterz@infradead.org wrote:
On Mon, Jun 02, 2025 at 01:13:29PM +0200, Maxime Ripard wrote:
I can't operate kunit
Why not?
Too complicated. People have even wrecked tools/testing/selftests/ to the point that it is now nearly impossible to run the simple selftests :-(
And while I don't mind tests -- they're quite useful. Kunit just looks to make it all more complicated that it needs to be. Not to mention there seems to be snakes involved -- and I never can remember how that works.
I've been out of the loop for a while, but I'm curious. What parts in particular are the most annoying, or is it basically all of them?
Is it that adding a new test file requires editing at least 3 files (Makefile, Kconfig, the actual test.c file)? Is it editing/writing new tests because the C API is hard to use? (Has too many funcs to know how to do simple things, etc.?)
For me personally, it's the first part about all the additional edits you have to make. I _hate_ doing it, but can't think of a good alternative that feels it makes the right tradeoffs (implementation complexity, requiring users to learn some new system or not, etc.)
Basically, if the stuff takes more effort to make run, than the time it runs for, its a loss. And in that respect much of the kernel testing stuff is a fail. Just too damn hard to make work.
I want to: make; ./run.sh or something similarly trivial. But clearly that is too much to task these days :-(
Agreed that ultimately, it would be nice if it was as simple as one of these $ run_kunit_tests --suite=test_suite_name $ run_kunit_tests --in_file=lib/my_test.c or something similar.
But while I don't see a way to get there, if you've set your new test config to `default KUNIT_ALL_TESTS` and are fine running on UML, then it could be as simple as $ ./tools/testing/kunit/kunit.py run
It should basically do what you want: `make` to regen the .config and build and then execute the tests.
But I can see these pain points with it, a) it'll run a bunch of other tests too by default but they shouldn't be failing, nor should they add too much build or test runtime overhead. b) if the new test you're adding doesn't work on UML, you'd have to fiddle with enabling more kconfig options or switch arches c) it can be confusing how it has multiple subcommands in addition to `run` and it's not immediately clear when/why you'd ever use them d) the fact it's not like kselftest and just part of make, i.e. `make TARGETS="foo" kselftest` * even if kunit.py was dead simple (and it's not, but I don't think it's _that_ complex), it's another tool to learn and keep in your head.
Do these cover what you've experienced? Or are there others?
I spent almost a full day trying to get kvm selftests working a couple of weeks ago; that's time I don't have. And it makes me want to go hulk and smash things.
Stepping back, I think there'll always be relatively simple things that take a bit too much effort to do in KUnit.
But I'd like to get to the point where anyone can feel comfortable doing the very simple things. And I don't want it to be with the caveat of "after they've read 10 pages of docs", because none of us have the time for that, as you say.
E.g. If someone is introducing a new data structure, it should be easy to ask them to learn a enough KUnit so that they write _basic_ sanity tests for it and add it to their patch series. Maybe it's annoying to cover all edge cases properly and very difficult to try and test concurrent read/writes, but those are inherently harder problems.
Cheers, Daniel
On Tue, Jun 03, 2025 at 02:26:03PM +0200, Peter Zijlstra wrote:
On Mon, Jun 02, 2025 at 01:13:29PM +0200, Maxime Ripard wrote:
I can't operate kunit
Why not?
Too complicated. People have even wrecked tools/testing/selftests/ to the point that it is now nearly impossible to run the simple selftests :-(
And while I don't mind tests -- they're quite useful. Kunit just looks to make it all more complicated that it needs to be. Not to mention there seems to be snakes involved -- and I never can remember how that works.
Basically, if the stuff takes more effort to make run, than the time it runs for, its a loss. And in that respect much of the kernel testing stuff is a fail. Just too damn hard to make work.
I want to: make; ./run.sh or something similarly trivial. But clearly that is too much to task these days :-(
Are you sure you're not confusing kunit with kselftests?
You can run all tests in the kernel using: ./tools/testing/kunit/kunit.py run
Restrict it to a single subsystem with (for DRM for example): ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests
Both would compile a UML kernel and run the tests on your workstation, but you can also run them in qemu with: ./tools/testing/kunit/kunit.py run --arch x86_64
So it looks close to what you expect?
Maxime
On Fri, May 30, 2025 at 10:48:47AM -0700, Kees Cook wrote:
This needs docs too. I think this is collapsing 1 and 2 argument WARNs into the ud1, and anything larger is explicitly calling __warn_printk + __WARN_FLAGS? If only 1 and 2 args can be collapsed, why reserve 0xfe00 through 0xfeff? I feel like I'm missing something here...
I did something really evil in that first version, I encoded the registers into the bug type. I've fixed that and have decode_bug emit a ud_regs thingy now.
On Fri, May 30, 2025 at 04:01:40PM +0200, Peter Zijlstra wrote:
I'm not really concerned with performance here, but more with the size of the code emitted by WARN_ONCE(). There are a *ton* of WARN sites, while only one report_bug() and printk().
We need a new and stronger unlikely(), resulting in the compiler being forced to split a .cold sub-function/part which lives in .text.unlikely
At that point it becomes less of a concern I suppose.
AFAIK the only means of achieving that with the current compilers is doing a manual function split and marking the part __cold -- which is unfortunate.
At some point GCC explored label attributes, and we were able to mark labels with cold, but that never really worked / went anywhere.
Hello Peter, thanks for the clear explanation. I finally understand what was bothering you, it wasn’t the __bug_table size or the execution time overhead, but the code size itself.
On Fri, May 30, 2025 at 4:02 PM Peter Zijlstra peterz@infradead.org wrote:
+Mark because he loves a hack :-)
On Thu, May 29, 2025 at 12:36:55PM +0200, Alessandro Carminati wrote:
Like I said before; you need to do this on the report_bug() size of things.
I fully understand your concerns, and I truly appreciate both yours and Josh’s feedback on this matter. Please rest assured that I took your suggestions seriously and carefully evaluated the possibility of consolidating all related logic within the exception handler. After a thorough investigation, however, I encountered several limitations that led me to maintain the check in the macro. I’d like to share the rationale behind this decision:
- In the case of WARN() messages, part of the output, the
user-specified content, is emitted directly by the macro, prior to reaching the exception handler [1]. Moving the check solely to the exception handler would not prevent this early output.
Yeah, this has been really annoying me for a long while. WARN() code gen is often horrible crap because of that.
Everything I've tried so far is worse though :/ So in the end I try to never use WARN(), its just not worth it.
... /me goes down the rabbit-hole again, because well, you can't let something simple like this defeat you ;-)
Results of today's hackery below. It might actually be worth cleaning up.
- Unless we change the user-facing interface that allows suppression
based on function names, we still need to work with those names at runtime.
I'm not sure I understand this. What interface and what names? This is a new feature, so how can there be an interface that needs to be preserved?
- This leaves us with two main strategies: converting function names
to pointers (e.g., via kallsyms) or continuing to work with names. The former requires name resolution at suppression time and pointer comparison in the handler, but function names are often altered by the compiler due to inlining or other optimizations[2]. Some WARN() sites are even marked __always_inline[3], making it difficult to prevent inlining altogether.
Arguably __func__ should be the function name of the function you get inlined into. C inlining does not preserve the sequence point, so there is absolutely no point in trying to preserve the inline name.
I'm again confused though; [2] does not use __func__ at all.
Anyway, when I do something like:
void __attribute__((__always_inline__)) foo(void) { puts(__func__); }
void bar(void) { foo(); }
it uses a "foo" string, which IMO is just plain wrong. Anyway, do both compilers guarantee it will always be foo? I don't think I've seen the GCC manual be explicit about this case.
On this point: even if not explicitly stated, __func__ will always be "foo" in your example. I’m confident in this because, according to the GCC manual[1], __func__ is inserted into the source as: static const char __func__[] = "function-name"; At that point, the compiler doesn't yet know what the final code will look like or whether it will be inlined. I’m not a compiler expert, but this definition doesn’t leave much room for alternative interpretations.
- An alternative is to embed function names in the __bug_table. While potentially workable, this increases the size of the table and
requires attention to handle position-independent builds (-fPIC/-fPIE), such as using offsets relative to __start_rodata.
However, the central challenge remains: any logic that aims to suppress WARN() output must either move the entire message emission into the exception handler or accept that user-specified parts of the message will still be printed.
Well, we can set suppress_printk and then all is quiet :-) Why isn't this good enough?
As a secondary point, there are also less common architectures where it's unclear whether suppressing these warnings is a priority, which might influence how broadly the effort is applied. I hoped to have addressed the concern of having faster runtime, by exposing a counter that could skip the logic. Kess suggested using static branching that would make things even better. Could Kess' suggestion mitigate your concern on this strategy? I’m absolutely open to any further thoughts or suggestions you may have, and I appreciate your continued guidance.
I'm not really concerned with performance here, but more with the size of the code emitted by WARN_ONCE(). There are a *ton* of WARN sites, while only one report_bug() and printk().
The really offensive thing is that this is for a feature most nobody will ever need :/
The below results in:
03dc 7ac: 48 c7 c0 00 00 00 00 mov $0x0,%rax 7af: R_X86_64_32S .rodata.str1.1+0x223 03e3 7b3: ba 2a 00 00 00 mov $0x2a,%edx 03e8 7b8: 48 0f b9 d0 ud1 %rax,%rdx
And it even works :-)
Hmm... I should try and stick the format string into the __bug_table, its const after all. Then I can get 2 arguments covered.
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h index f0e9acf72547..88b305d49f35 100644 --- a/arch/x86/include/asm/bug.h +++ b/arch/x86/include/asm/bug.h
The rework is impressive. I’m not in a position to judge, but for what it’s worth, you have my admiration.
That said, I see a problem, the same I faced when embedding __func__ in the bug table. __func__, and I believe also printk fmt, are constants, but their pointers might not be, especially in position-independent code. This causes issues depending on the compiler. For example, my tests worked fine with recent GCC on x86_64, but failed with Clang. Changing architecture complicates things further, GCC added support for this only in newer versions. I ran into this in v3 when the s390x build failed[2].
As mentioned in the patchset cover letter, my solution to avoid copying the strings into the bug table is to store their pointer as an offset from __start_rodata. [1]. https://gcc.gnu.org/onlinedocs/gcc/Function-Names.html [2]. https://lore.kernel.org/all/202503200847.LbkIJIXa-lkp@intel.com/
On Mon, May 26, 2025 at 01:27:51PM +0000, Alessandro Carminati wrote:
A helper function, `__kunit_is_suppressed_warning()`, is used to determine whether suppression applies. It is marked as `noinstr`, since some `WARN*()` sites reside in non-instrumentable sections. As it uses `strcmp`, a `noinstr` version of `strcmp` was introduced.
That just sounds all sorts of wrong.
KUnit support is not consistently present across distributions, some include it in their stock kernels, while others do not. While both KUNIT and KUNIT_SUPPRESS_BACKTRACE can be considered debug features, the fact that some distros ship with KUnit enabled means it's important to minimize the runtime impact of this patch. To that end, this patch introduces a counter for the number of suppressed symbols and skips execution of __kunit_is_suppressed_warning() entirely when no symbols are currently being suppressed.
Signed-off-by: Alessandro Carminati acarmina@redhat.com --- include/asm-generic/bug.h | 21 ++++++++++++++------- include/kunit/bug.h | 1 + lib/kunit/bug.c | 4 ++++ 3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 3cc8cb100ccd..c5587820bd8c 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -62,7 +62,8 @@ struct bug_entry { */ #ifndef HAVE_ARCH_BUG #define BUG() do { \ - if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ + if (suppressed_symbols_cnt && \ + !KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ printk("BUG: failure at %s:%d/%s()!\n", __FILE__, \ __LINE__, __func__); \ barrier_before_unreachable(); \ @@ -99,7 +100,8 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #ifndef __WARN_FLAGS #define __WARN() __WARN_printf(TAINT_WARN, NULL) #define __WARN_printf(taint, arg...) do { \ - if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ + if (suppressed_symbols_cnt && \ + !KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ instrumentation_begin(); \ warn_slowpath_fmt(__FILE__, __LINE__, taint, arg);\ instrumentation_end(); \ @@ -108,7 +110,8 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #else #define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) #define __WARN_printf(taint, arg...) do { \ - if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ + if (suppressed_symbols_cnt && \ + !KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ instrumentation_begin(); \ __warn_printk(arg); \ __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | \ @@ -118,7 +121,8 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); } while (0) #define WARN_ON_ONCE(condition) ({ \ int __ret_warn_on = !!(condition); \ - if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ + if (unlikely(__ret_warn_on) && suppressed_symbols_cnt &&\ + !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN_FLAGS(BUGFLAG_ONCE | \ BUGFLAG_TAINT(TAINT_WARN)); \ unlikely(__ret_warn_on); \ @@ -130,7 +134,8 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #ifndef WARN_ON #define WARN_ON(condition) ({ \ int __ret_warn_on = !!(condition); \ - if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ + if (unlikely(__ret_warn_on) && suppressed_symbols_cnt && \ + !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN(); \ unlikely(__ret_warn_on); \ }) @@ -147,7 +152,8 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
#define WARN_TAINT(condition, taint, format...) ({ \ int __ret_warn_on = !!(condition); \ - if (unlikely(__ret_warn_on) && !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ + if (unlikely(__ret_warn_on) && suppressed_symbols_cnt && \ + !KUNIT_IS_SUPPRESSED_WARNING(__func__)) \ __WARN_printf(taint, format); \ unlikely(__ret_warn_on); \ }) @@ -166,7 +172,8 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #else /* !CONFIG_BUG */ #ifndef HAVE_ARCH_BUG #define BUG() do { \ - if (!KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ + if (suppressed_symbols_cnt && \ + !KUNIT_IS_SUPPRESSED_WARNING(__func__)) { \ do {} while (1); \ unreachable(); \ } \ diff --git a/include/kunit/bug.h b/include/kunit/bug.h index 9a4eff2897e9..3256e3f2c165 100644 --- a/include/kunit/bug.h +++ b/include/kunit/bug.h @@ -23,6 +23,7 @@ struct __suppressed_warning { const char *function; int counter; }; +extern int suppressed_symbols_cnt;
void __kunit_start_suppress_warning(struct __suppressed_warning *warning); void __kunit_end_suppress_warning(struct __suppressed_warning *warning); diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c index 4da9ae478f25..5beaee1049eb 100644 --- a/lib/kunit/bug.c +++ b/lib/kunit/bug.c @@ -11,15 +11,19 @@ #include <linux/list.h>
static LIST_HEAD(suppressed_warnings); +int suppressed_symbols_cnt = 0; +EXPORT_SYMBOL_GPL(suppressed_symbols_cnt);
void __kunit_start_suppress_warning(struct __suppressed_warning *warning) { + suppressed_symbols_cnt++; list_add(&warning->node, &suppressed_warnings); } EXPORT_SYMBOL_GPL(__kunit_start_suppress_warning);
void __kunit_end_suppress_warning(struct __suppressed_warning *warning) { + suppressed_symbols_cnt--; list_del(&warning->node); } EXPORT_SYMBOL_GPL(__kunit_end_suppress_warning);
On Mon, May 26, 2025 at 01:27:52PM +0000, Alessandro Carminati wrote:
KUnit support is not consistently present across distributions, some include it in their stock kernels, while others do not. While both KUNIT and KUNIT_SUPPRESS_BACKTRACE can be considered debug features, the fact that some distros ship with KUnit enabled means it's important to minimize the runtime impact of this patch. To that end, this patch introduces a counter for the number of suppressed symbols and skips execution of __kunit_is_suppressed_warning() entirely when no symbols are currently being suppressed.
If KUnit already serialized? I should have asked this before: you're reading and writing a global list -- I think some kind of RCU may be needed for the list? One thread may be removing a function from the list while another thread is executing a WARN-induced walk of the list...
This global may have the same problem. Why not use a static branch, or is that just overkill?
-Kees
From: Guenter Roeck linux@roeck-us.net
Add unit tests to verify that warning backtrace suppression works.
If backtrace suppression does _not_ work, the unit tests will likely trigger unsuppressed backtraces, which should actually help to get the affected architectures / platforms fixed.
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- lib/kunit/Makefile | 3 + lib/kunit/backtrace-suppression-test.c | 105 +++++++++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 lib/kunit/backtrace-suppression-test.c
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index 3195e861d63c..05fb19d69709 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -23,6 +23,9 @@ obj-y += hooks.o \
obj-$(CONFIG_KUNIT_TEST) += kunit-test.o obj-$(CONFIG_KUNIT_TEST) += platform-test.o +ifeq ($(CONFIG_KUNIT_SUPPRESS_BACKTRACE),y) +obj-$(CONFIG_KUNIT_TEST) += backtrace-suppression-test.o +endif
# string-stream-test compiles built-in only. ifeq ($(CONFIG_KUNIT_TEST),y) diff --git a/lib/kunit/backtrace-suppression-test.c b/lib/kunit/backtrace-suppression-test.c new file mode 100644 index 000000000000..a3d5991b9b15 --- /dev/null +++ b/lib/kunit/backtrace-suppression-test.c @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for suppressing warning tracebacks + * + * Copyright (C) 2024, Guenter Roeck + * Author: Guenter Roeck linux@roeck-us.net + */ + +#include <kunit/test.h> +#include <linux/bug.h> + +static void backtrace_suppression_test_warn_direct(struct kunit *test) +{ + KUNIT_DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct); + + KUNIT_START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct); + WARN(1, "This backtrace should be suppressed"); + KUNIT_END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct); + + KUNIT_EXPECT_EQ(test, KUNIT_SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_direct), 1); +} + +static void trigger_backtrace_warn(void) +{ + WARN(1, "This backtrace should be suppressed"); +} + +static void backtrace_suppression_test_warn_indirect(struct kunit *test) +{ + KUNIT_DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn); + + KUNIT_START_SUPPRESSED_WARNING(trigger_backtrace_warn); + trigger_backtrace_warn(); + KUNIT_END_SUPPRESSED_WARNING(trigger_backtrace_warn); + + KUNIT_EXPECT_EQ(test, KUNIT_SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn), 1); +} + +static void backtrace_suppression_test_warn_multi(struct kunit *test) +{ + KUNIT_DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn); + KUNIT_DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi); + + KUNIT_START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi); + KUNIT_START_SUPPRESSED_WARNING(trigger_backtrace_warn); + WARN(1, "This backtrace should be suppressed"); + trigger_backtrace_warn(); + KUNIT_END_SUPPRESSED_WARNING(trigger_backtrace_warn); + KUNIT_END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi); + + KUNIT_EXPECT_EQ(test, KUNIT_SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_multi), 1); + KUNIT_EXPECT_EQ(test, KUNIT_SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn), 1); +} + +static void backtrace_suppression_test_warn_on_direct(struct kunit *test) +{ + KUNIT_DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct); + + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE) && !IS_ENABLED(CONFIG_KALLSYMS)) + kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE or CONFIG_KALLSYMS"); + + KUNIT_START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct); + WARN_ON(1); + KUNIT_END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct); + + KUNIT_EXPECT_EQ(test, + KUNIT_SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_on_direct), 1); +} + +static void trigger_backtrace_warn_on(void) +{ + WARN_ON(1); +} + +static void backtrace_suppression_test_warn_on_indirect(struct kunit *test) +{ + KUNIT_DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn_on); + + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE)) + kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE"); + + KUNIT_START_SUPPRESSED_WARNING(trigger_backtrace_warn_on); + trigger_backtrace_warn_on(); + KUNIT_END_SUPPRESSED_WARNING(trigger_backtrace_warn_on); + + KUNIT_EXPECT_EQ(test, KUNIT_SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn_on), 1); +} + +static struct kunit_case backtrace_suppression_test_cases[] = { + KUNIT_CASE(backtrace_suppression_test_warn_direct), + KUNIT_CASE(backtrace_suppression_test_warn_indirect), + KUNIT_CASE(backtrace_suppression_test_warn_multi), + KUNIT_CASE(backtrace_suppression_test_warn_on_direct), + KUNIT_CASE(backtrace_suppression_test_warn_on_indirect), + {} +}; + +static struct kunit_suite backtrace_suppression_test_suite = { + .name = "backtrace-suppression-test", + .test_cases = backtrace_suppression_test_cases, +}; +kunit_test_suites(&backtrace_suppression_test_suite); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("KUnit test to verify warning backtrace suppression");
From: Guenter Roeck linux@roeck-us.net
The drm_test_rect_calc_hscale and drm_test_rect_calc_vscale unit tests intentionally trigger warning backtraces by providing bad parameters to the tested functions. What is tested is the return value, not the existence of a warning backtrace. Suppress the backtraces to avoid clogging the kernel log and distraction from real problems.
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Acked-by: Maíra Canal mcanal@igalia.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: David Airlie airlied@gmail.com Cc: Daniel Vetter daniel@ffwll.ch Signed-off-by: Guenter Roeck linux@roeck-us.net Signed-off-by: Alessandro Carminati acarmina@redhat.com --- drivers/gpu/drm/tests/drm_rect_test.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_rect_test.c b/drivers/gpu/drm/tests/drm_rect_test.c index 17e1f34b7610..867845e7d5ab 100644 --- a/drivers/gpu/drm/tests/drm_rect_test.c +++ b/drivers/gpu/drm/tests/drm_rect_test.c @@ -406,22 +406,38 @@ KUNIT_ARRAY_PARAM(drm_rect_scale, drm_rect_scale_cases, drm_rect_scale_case_desc
static void drm_test_rect_calc_hscale(struct kunit *test) { + KUNIT_DEFINE_SUPPRESSED_WARNING(drm_calc_scale); const struct drm_rect_scale_case *params = test->param_value; int scaling_factor;
+ /* + * drm_rect_calc_hscale() generates a warning backtrace whenever bad + * parameters are passed to it. This affects all unit tests with an + * error code in expected_scaling_factor. + */ + KUNIT_START_SUPPRESSED_WARNING(drm_calc_scale); scaling_factor = drm_rect_calc_hscale(¶ms->src, ¶ms->dst, params->min_range, params->max_range); + KUNIT_END_SUPPRESSED_WARNING(drm_calc_scale);
KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor); }
static void drm_test_rect_calc_vscale(struct kunit *test) { + KUNIT_DEFINE_SUPPRESSED_WARNING(drm_calc_scale); const struct drm_rect_scale_case *params = test->param_value; int scaling_factor;
+ /* + * drm_rect_calc_vscale() generates a warning backtrace whenever bad + * parameters are passed to it. This affects all unit tests with an + * error code in expected_scaling_factor. + */ + KUNIT_START_SUPPRESSED_WARNING(drm_calc_scale); scaling_factor = drm_rect_calc_vscale(¶ms->src, ¶ms->dst, params->min_range, params->max_range); + KUNIT_END_SUPPRESSED_WARNING(drm_calc_scale);
KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor); }
From: Guenter Roeck linux@roeck-us.net
Document API functions for suppressing warning backtraces.
Tested-by: Linux Kernel Functional Testing lkft@linaro.org Acked-by: Dan Carpenter dan.carpenter@linaro.org Reviewed-by: Kees Cook keescook@chromium.org Signed-off-by: Guenter Roeck linux@roeck-us.net Reviewed-by: David Gow davidgow@google.com Signed-off-by: Alessandro Carminati acarmina@redhat.com --- Documentation/dev-tools/kunit/usage.rst | 30 ++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index 22955d56b379..b2f1e56d53b4 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -157,6 +157,34 @@ Alternatively, one can take full control over the error message by using if (some_setup_function()) KUNIT_FAIL(test, "Failed to setup thing for testing");
+Suppressing warning backtraces +------------------------------ + +Some unit tests trigger warning backtraces either intentionally or as side +effect. Such backtraces are normally undesirable since they distract from +the actual test and may result in the impression that there is a problem. + +Such backtraces can be suppressed. To suppress a backtrace in some_function(), +use the following code. + +.. code-block:: c + + static void some_test(struct kunit *test) + { + DEFINE_SUPPRESSED_WARNING(some_function); + + KUNIT_START_SUPPRESSED_WARNING(some_function); + trigger_backtrace(); + KUNIT_END_SUPPRESSED_WARNING(some_function); + } + +SUPPRESSED_WARNING_COUNT() returns the number of suppressed backtraces. If the +suppressed backtrace was triggered on purpose, this can be used to check if +the backtrace was actually triggered. + +.. code-block:: c + + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(some_function), 1);
Test Suites ~~~~~~~~~~~ @@ -857,4 +885,4 @@ For example: dev_managed_string = devm_kstrdup(fake_device, "Hello, World!");
// Everything is cleaned up automatically when the test ends. - } \ No newline at end of file + }
I like suppressing warning messages but there are still many cases, such as mm/kasan/kasan_test_c.c where printing the warning message is the whole point.
We should create a standard way that test bots can filter out deliberate errors from unintentional errors. This would also help humans who have to look at test results.
#define intentional_warning_marker(type) do { \ pr_err("Triggering intentional %s warning!", type); \ } while (0)
intentional_warning_marker("KASAN");
regards, dan carpenter
On Mon, Jun 02, 2025 at 10:24:17AM +0300, Dan Carpenter wrote:
I like suppressing warning messages but there are still many cases, such as mm/kasan/kasan_test_c.c where printing the warning message is the whole point.
We should create a standard way that test bots can filter out deliberate errors from unintentional errors. This would also help humans who have to look at test results.
#define intentional_warning_marker(type) do { \ pr_err("Triggering intentional %s warning!", type); \ } while (0)
intentional_warning_marker("KASAN");
I understand what your usecase is, and would definitely appreciate something like that too, but I don't think this is the right way to do it.
Once we have the basic infrastructure in place to flag which warnings are legitimate and which aren't, I believe a better way to achieve what you're asking for would be to treat as failures any warning with a WARN, and any test expecting a warn that didn't trigger any.
This would bring kunit on par with pretty much every other unit test frameworks out there, and would make it pretty obvious to any users (CI and humans) when it works and when it doesn't.
Maxime
linux-kselftest-mirror@lists.linaro.org
-
Alessandro Carminati -
Dan Carpenter -
Daniel Latypov -
Kees Cook -
Maxime Ripard -
Peter Zijlstra -
Simona Vetter