lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250530140140.GE21197@noisy.programming.kicks-ass.net>
Date: Fri, 30 May 2025 16:01:40 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Alessandro Carminati <acarmina@...hat.com>
Cc: linux-kselftest@...r.kernel.org,
	Dan Carpenter <dan.carpenter@...aro.org>,
	Kees Cook <keescook@...omium.org>,
	Daniel Diaz <daniel.diaz@...aro.org>,
	David Gow <davidgow@...gle.com>,
	Arthur Grillo <arthurgrillo@...eup.net>,
	Brendan Higgins <brendan.higgins@...ux.dev>,
	Naresh Kamboju <naresh.kamboju@...aro.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Maxime Ripard <mripard@...nel.org>,
	Ville Syrjala <ville.syrjala@...ux.intel.com>,
	Daniel Vetter <daniel@...ll.ch>, Guenter Roeck <linux@...ck-us.net>,
	Alessandro Carminati <alessandro.carminati@...il.com>,
	Jani Nikula <jani.nikula@...el.com>,
	Jeff Johnson <jeff.johnson@....qualcomm.com>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	Shuah Khan <skhan@...uxfoundation.org>,
	Linux Kernel Functional Testing <lkft@...aro.org>,
	dri-devel@...ts.freedesktop.org, kunit-dev@...glegroups.com,
	linux-kernel@...r.kernel.org, Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v5 1/5] bug/kunit: Core support for suppressing warning
 backtraces


+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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ