kunit: Add support for suppressing warning backtraces

[PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Alessandro Carminati 6 months, 3 weeks ago

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);
-- 
2.34.1

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 3 weeks ago

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.

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 3 weeks ago

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.

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Alessandro Carminati 6 months, 3 weeks ago

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/include/asm-generic/bug.h#n106
[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/include/linux/file_ref.h#n118
this is one example, others exist.
-- 
---
Thanks
Alessandro

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 3 weeks ago

+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

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Alessandro Carminati 6 months, 3 weeks ago

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/

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 3 weeks ago

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.

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Kees Cook 6 months, 3 weeks ago

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 */
>  		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;

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. :)

-- 
Kees Cook

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 3 weeks ago

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.

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 3 weeks ago

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,

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Maxime Ripard 6 months, 2 weeks ago

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

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 2 weeks ago

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.

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Maxime Ripard 6 months, 2 weeks ago

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

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Daniel Latypov 6 months, 2 weeks ago

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

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Kees Cook 6 months, 3 weeks ago


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


-- 
Kees Cook

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 2 weeks ago

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?

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Maxime Ripard 6 months, 2 weeks ago

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

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Simona Vetter 6 months, 2 weeks ago

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
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Kees Cook 6 months, 3 weeks ago

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>

-- 
Kees Cook

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 3 weeks ago

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.

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Kees Cook 6 months, 3 weeks ago

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...

-- 
Kees Cook

Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces

Posted by Peter Zijlstra 6 months, 3 weeks ago

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' ?

[PATCH v5 1/5] bug/kunit: Core support for suppressing warning backtraces
[PATCH v5 2/5] bug/kunit: Suppressing warning backtraces reduced impact on WARN*() sites
[PATCH v5 3/5] Add unit tests to verify that warning backtrace suppression works.
[PATCH v5 4/5] drm: Suppress intentional warning backtraces in scaling unit tests
[PATCH v5 5/5] kunit: Add documentation for warning backtrace suppression API