[PATCH 00/11] x86: WARN() hackery

[PATCH 00/11] x86: WARN() hackery

[Peter Zijlstra]

Hi,

Slightly less mad this time :-)

The primary purpose of all this is to get the WARN() printk() and
__warn() calls into the same context. Notably the current state is that
WARN() ends up doing printk() in-place, then takes an exception and has
the exception do the __warn().

The problem with all this is the ONCE logic. Normal WARN_ON_ONCE()
(without the printk) has the ONCE logic in the exception
(__report_bug()). But because WARN() essentially results in two distinct
actions (printk + trap) this cannot work.  With the result that
additional ONCE logic is sprinkled around for each such site.

Current proposals look to make this worse by adding KUnit checks for all
this, including regular WARN. Making the per-instance code overhead even
worse.

As such, by moving the printk() into the exception, and having the
exception (__report_bug() in fact) do everything, we get rid of the
external ONCE logic and provide a cental place for additional conditions
without need to litter all the instances.

[PATCH 01/11] x86: Provide assembly __bug_table helpers

[Peter Zijlstra]

Rework the __bug_table helpers such that usage from assembly becomes
possible.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/bug.h |   54 ++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -32,46 +32,40 @@
 #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 /* sizeof(file) + sizeof(line) */
+#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_BASE_SIZE		6 /* sizeof(bug_addr) + sizeof(flags) */
 
-#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 + ASM_BUGTABLE_BASE_SIZE + ASM_BUGTABLE_VERBOSE_SIZE ;\
+	.popsection
 
-#define _BUG_FLAGS(ins, flags, extra)					\
+#define _BUG_FLAGS(insn, 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)));		\
+	asm_inline volatile("1:\t" insn "\n"				\
+	    __stringify(ASM_BUGTABLE_FLAGS(1b, %c[file], %c[line], %c[fl])) "\n" \
+			    extra					\
+		     : : [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)

[PATCH 02/11] bug: Add BUG_FORMAT infrastructure

[Peter Zijlstra]

Add BUG_FORMAT; an architecture opt-in feature that allows adding the
WARN_printf() format string to the bug_entry table.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/asm-generic/bug.h |    7 +++++++
 lib/bug.c                 |   39 ++++++++++++++++++++++++++++++++-------
 2 files changed, 39 insertions(+), 7 deletions(-)

--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -36,6 +36,13 @@ struct bug_entry {
 #else
 	signed int	bug_addr_disp;
 #endif
+#ifdef HAVE_ARCH_BUG_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;
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -139,6 +139,19 @@ void bug_get_file_line(struct bug_entry
 #endif
 }
 
+static const char *bug_get_format(struct bug_entry *bug)
+{
+	const char *format = NULL;
+#ifdef HAVE_ARCH_BUG_FORMAT
+#ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
+	format = (const char *)&bug->format_disp + bug->format_disp;
+#else
+	format = bug->format;
+#endif
+#endif
+	return format;
+}
+
 struct bug_entry *find_bug(unsigned long bugaddr)
 {
 	struct bug_entry *bug;
@@ -150,11 +163,19 @@ struct bug_entry *find_bug(unsigned long
 	return module_find_bug(bugaddr);
 }
 
+static void __warn_printf(const char *fmt)
+{
+	if (!fmt)
+		return;
+
+	printk("%s", fmt);
+}
+
 static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *regs)
 {
-	struct bug_entry *bug;
-	const char *file;
-	unsigned line, warning, once, done;
+	bool warning, once, done, no_cut, has_args;
+	const char *file, *fmt;
+	unsigned line;
 
 	if (!is_valid_bugaddr(bugaddr))
 		return BUG_TRAP_TYPE_NONE;
@@ -166,10 +187,12 @@ static enum bug_trap_type __report_bug(u
 	disable_trace_on_warning();
 
 	bug_get_file_line(bug, &file, &line);
+	fmt = bug_get_format(bug);
 
-	warning = (bug->flags & BUGFLAG_WARNING) != 0;
-	once = (bug->flags & BUGFLAG_ONCE) != 0;
-	done = (bug->flags & BUGFLAG_DONE) != 0;
+	warning  = bug->flags & BUGFLAG_WARNING;
+	once     = bug->flags & BUGFLAG_ONCE;
+	done     = bug->flags & BUGFLAG_DONE;
+	no_cut   = bug->flags & BUGFLAG_NO_CUT_HERE;
 
 	if (warning && once) {
 		if (done)
@@ -187,8 +210,10 @@ static enum bug_trap_type __report_bug(u
 	 * "cut here" line now. WARN() issues its own "cut here" before the
 	 * extra debugging message it writes before triggering the handler.
 	 */
-	if ((bug->flags & BUGFLAG_NO_CUT_HERE) == 0)
+	if (!no_cut) {
 		printk(KERN_DEFAULT CUT_HERE);
+		__warn_printf(fmt);
+	}
 
 	if (warning) {
 		/* this is a WARN_ON rather than BUG/BUG_ON */

[PATCH 03/11] bug: Clean up CONFIG_GENERIC_BUG_RELATIVE_POINTERS

[Peter Zijlstra]

Three repeated CONFIG_GENERIC_BUG_RELATIVE_POINTERS #ifdefs right
after one another yields unreadable code. Add a helper.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/asm-generic/bug.h |   22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -30,26 +30,20 @@ void __warn(const char *file, int line,
 
 #ifdef CONFIG_BUG
 
-#ifdef CONFIG_GENERIC_BUG
-struct bug_entry {
 #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
-	unsigned long	bug_addr;
+#define BUG_REL(type, name) type name
 #else
-	signed int	bug_addr_disp;
+#define BUG_REL(type, name) signed int name##_disp
 #endif
+
+#ifdef CONFIG_GENERIC_BUG
+struct bug_entry {
+	BUG_REL(unsigned long, bug_addr);
 #ifdef HAVE_ARCH_BUG_FORMAT
-#ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
-	const char	*format;
-#else
-	signed int	format_disp;
-#endif
+	BUG_REL(const char *, format);
 #endif
 #ifdef CONFIG_DEBUG_BUGVERBOSE
-#ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
-	const char	*file;
-#else
-	signed int	file_disp;
-#endif
+	BUG_REL(const char *, file);
 	unsigned short	line;
 #endif
 	unsigned short	flags;

[PATCH 04/11] bug: Add BUG_FORMAT_ARGS infrastructure

[Peter Zijlstra]

Add BUG_FORMAT_ARGS; when an architecture is able to provide a va_list
given pt_regs, use this to print format arguments.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/asm-generic/bug.h |    1 +
 lib/bug.c                 |   17 +++++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

--- 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_ARGS		(1 << 4)
 #define BUGFLAG_TAINT(taint)	((taint) << 8)
 #define BUG_GET_TAINT(bug)	((bug)->flags >> 8)
 #endif
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -163,11 +163,23 @@ struct bug_entry *find_bug(unsigned long
 	return module_find_bug(bugaddr);
 }
 
-static void __warn_printf(const char *fmt)
+static void __warn_printf(const char *fmt, struct pt_regs *regs)
 {
 	if (!fmt)
 		return;
 
+#ifdef HAVE_ARCH_BUG_FORMAT_ARGS
+	if (regs) {
+		struct arch_va_list _args;
+		va_list *args = __warn_args(&_args, regs);
+
+		if (args) {
+			vprintk(fmt, *args);
+			return;
+		}
+	}
+#endif
+
 	printk("%s", fmt);
 }
 
@@ -193,6 +205,7 @@ static enum bug_trap_type __report_bug(u
 	once     = bug->flags & BUGFLAG_ONCE;
 	done     = bug->flags & BUGFLAG_DONE;
 	no_cut   = bug->flags & BUGFLAG_NO_CUT_HERE;
+	has_args = bug->flags & BUGFLAG_ARGS;
 
 	if (warning && once) {
 		if (done)
@@ -212,7 +225,7 @@ static enum bug_trap_type __report_bug(u
 	 */
 	if (!no_cut) {
 		printk(KERN_DEFAULT CUT_HERE);
-		__warn_printf(fmt);
+		__warn_printf(fmt, has_args ? regs : NULL);
 	}
 
 	if (warning) {

[PATCH 05/11] bug: Add report_bug_entry()

[Peter Zijlstra]

Add a report_bug() variant where the bug_entry is already known. This
is useful when the exception instruction is not instantiated per-site.
But instead has a single instance. In such a case the bug_entry
address might be passed along in a known register or something.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/bug.h |    1 +
 lib/bug.c           |   32 +++++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 9 deletions(-)

--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -42,6 +42,7 @@ void bug_get_file_line(struct bug_entry
 struct bug_entry *find_bug(unsigned long bugaddr);
 
 enum bug_trap_type report_bug(unsigned long bug_addr, struct pt_regs *regs);
+enum bug_trap_type report_bug_entry(struct bug_entry *bug, struct pt_regs *regs);
 
 /* These are defined by the architecture */
 int is_valid_bugaddr(unsigned long addr);
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -183,18 +183,20 @@ static void __warn_printf(const char *fm
 	printk("%s", fmt);
 }
 
-static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *regs)
+static enum bug_trap_type __report_bug(struct bug_entry *bug, unsigned long bugaddr, struct pt_regs *regs)
 {
 	bool warning, once, done, no_cut, has_args;
 	const char *file, *fmt;
 	unsigned line;
 
-	if (!is_valid_bugaddr(bugaddr))
-		return BUG_TRAP_TYPE_NONE;
-
-	bug = find_bug(bugaddr);
-	if (!bug)
-		return BUG_TRAP_TYPE_NONE;
+	if (!bug) {
+		if (!is_valid_bugaddr(bugaddr))
+			return BUG_TRAP_TYPE_NONE;
+
+		bug = find_bug(bugaddr);
+		if (!bug)
+			return BUG_TRAP_TYPE_NONE;
+	}
 
 	disable_trace_on_warning();
 
@@ -244,13 +246,25 @@ static enum bug_trap_type __report_bug(u
 	return BUG_TRAP_TYPE_BUG;
 }
 
+enum bug_trap_type report_bug_entry(struct bug_entry *bug, struct pt_regs *regs)
+{
+	enum bug_trap_type ret;
+	bool rcu = false;
+
+	rcu = warn_rcu_enter();
+	ret = __report_bug(bug, 0, regs);
+	warn_rcu_exit(rcu);
+
+	return ret;
+}
+
 enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 {
 	enum bug_trap_type ret;
 	bool rcu = false;
 
 	rcu = warn_rcu_enter();
-	ret = __report_bug(bugaddr, regs);
+	ret = __report_bug(NULL, bugaddr, regs);
 	warn_rcu_exit(rcu);
 
 	return ret;

[PATCH 06/11] bug: Allow architectures to provide __WARN_printf()

[Peter Zijlstra]

Instead of providing __WARN_FLAGS(), allow an architecture to provide
__WARN_printf(), which allows for optimizing WARN(), rather than
WARN_ON().

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/asm-generic/bug.h |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -94,14 +94,7 @@ void warn_slowpath_fmt(const char *file,
 		       const char *fmt, ...);
 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();					\
-	} while (0)
-#else
+#if defined(__WARN_FLAGS) && !defined(__WARN_printf)
 #define __WARN()		__WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN))
 #define __WARN_printf(taint, arg...) do {				\
 		instrumentation_begin();				\
@@ -118,6 +111,18 @@ extern __printf(1, 2) void __warn_printk
 })
 #endif
 
+#ifndef __WARN_printf
+#define __WARN_printf(taint, arg...) do {				\
+		instrumentation_begin();				\
+		warn_slowpath_fmt(__FILE__, __LINE__, taint, arg);	\
+		instrumentation_end();					\
+	} while (0)
+#endif
+
+#ifndef __WARN
+#define __WARN()		__WARN_printf(TAINT_WARN, NULL)
+#endif
+
 /* used internally by panic.c */
 
 #ifndef WARN_ON

[PATCH 07/11] x86_64/bug: Add BUG_FORMAT basics

[Peter Zijlstra]

Opt-in to BUG_FORMAT for x86_64, adjust the BUGTABLE helper and for
now, just store NULL pointers.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/bug.h |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -47,22 +47,35 @@
 #define ASM_BUGTABLE_VERBOSE_SIZE	0
 #endif
 
+#ifdef CONFIG_X86_64
+#define HAVE_ARCH_BUG_FORMAT
+#define ASM_BUGTABLE_FORMAT(format)					\
+	ASM_BUG_REL(format)
+#define ASM_BUGTABLE_FORMAT_SIZE	4 /* sizeof(format) */
+#else
+#define ASM_BUGTABLE_FORMAT(format)
+#define ASM_BUGTABLE_FORMAT_SIZE	0
+#endif
+
 #define ASM_BUGTABLE_BASE_SIZE		6 /* sizeof(bug_addr) + sizeof(flags) */
 
-#define ASM_BUGTABLE_FLAGS(at, file, line, flags)			\
+#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 + ASM_BUGTABLE_BASE_SIZE + ASM_BUGTABLE_VERBOSE_SIZE ;\
+	.org 123b + ASM_BUGTABLE_BASE_SIZE + ASM_BUGTABLE_FORMAT_SIZE + ASM_BUGTABLE_VERBOSE_SIZE ; \
 	.popsection
 
 #define _BUG_FLAGS(insn, flags, extra)					\
 do {									\
 	asm_inline volatile("1:\t" insn "\n"				\
-	    __stringify(ASM_BUGTABLE_FLAGS(1b, %c[file], %c[line], %c[fl])) "\n" \
+	    __stringify(ASM_BUGTABLE_FLAGS(1b, %c[fmt], %c[file], %c[line], %c[fl])) "\n" \
 			    extra					\
-		     : : [file] "i" (__FILE__), [line] "i" (__LINE__),	\
+		     : : [fmt] "i" (NULL),				\
+		         [file] "i" (__FILE__),				\
+			 [line] "i" (__LINE__),				\
 			 [fl] "i" (flags));				\
 } while (0)
 

[PATCH 08/11] x86_64/bug: Implement __WARN_printf()

[Peter Zijlstra]

The basic idea is to have __WARN_printf() be a vararg function such
that the compiler can do the optimal calling convention for us. This
function body will be a #UD and then set up a va_list in the exception
from pt_regs.

But because the trap will be in a called function, the bug_entry must
be passed in. Have that be the first argument, with the format tucked
away inside the bug_entry.

The comments should clarify the real fun details.

The big downside is that all WARNs will now show:

 RIP: 0010:__WARN_trap:+0

This will be adressed in a follow up patch.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/entry.S     |    8 +++
 arch/x86/include/asm/bug.h |   47 +++++++++++++++++++
 arch/x86/kernel/traps.c    |  107 ++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 152 insertions(+), 10 deletions(-)

--- a/arch/x86/entry/entry.S
+++ b/arch/x86/entry/entry.S
@@ -32,6 +32,14 @@ SYM_FUNC_END(write_ibpb)
 /* For KVM */
 EXPORT_SYMBOL_GPL(write_ibpb);
 
+SYM_FUNC_START(__WARN_trap)
+	ANNOTATE_NOENDBR
+	ANNOTATE_REACHABLE
+	ud1 (%ecx), %_ASM_ARG1
+	RET
+SYM_FUNC_END(__WARN_trap)
+EXPORT_SYMBOL(__WARN_trap)
+
 .popsection
 
 /*
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -26,6 +26,7 @@
 #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
 
@@ -49,6 +50,7 @@
 
 #ifdef CONFIG_X86_64
 #define HAVE_ARCH_BUG_FORMAT
+#define HAVE_ARCH_BUG_FORMAT_ARGS
 #define ASM_BUGTABLE_FORMAT(format)					\
 	ASM_BUG_REL(format)
 #define ASM_BUGTABLE_FORMAT_SIZE	4 /* sizeof(format) */
@@ -107,6 +109,51 @@ do {								\
 	instrumentation_end();					\
 } while (0)
 
+#define __WARN()		__WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN))
+
+#ifdef HAVE_ARCH_BUG_FORMAT
+
+#ifndef __ASSEMBLY__
+struct bug_entry;
+extern void __WARN_trap(struct bug_entry *bug, ...);
+
+struct pt_regs;
+struct sysv_va_list { /* from AMD64 System V ABI */
+	unsigned int gp_offset;
+	unsigned int fp_offset;
+	void *overflow_arg_area;
+	void *reg_save_area;
+};
+struct arch_va_list {
+	unsigned long regs[6];
+	struct sysv_va_list args;
+};
+extern void *__warn_args(struct arch_va_list *args, struct pt_regs *regs);
+#endif /* __ASSEMBLY__ */
+
+#define __WARN_bug_entry(flags, format) ({				\
+	struct bug_entry *bug;						\
+	asm(__stringify(ASM_BUGTABLE_FLAGS(1f, %c[fmt], %c[file], %c[line], %c[fl])) "\n" \
+	     "\tlea (123b)(%%rip), %[addr] \n1:\t"			\
+	     : [addr] "=r" (bug)					\
+	     : [fmt] "i" (format),					\
+	       [file] "i" (__FILE__),					\
+	       [line] "i" (__LINE__),					\
+	       [fl] "i" (flags));					\
+	bug; })
+
+#define __WARN_print_arg(flags, format, arg...)				\
+do {									\
+	int __flags = (flags) | BUGFLAG_WARNING | BUGFLAG_ARGS ;	\
+	__WARN_trap(__WARN_bug_entry(__flags, format), ## arg);		\
+	asm (""); /* inhibit tail-call optimization */			\
+} while (0)
+
+#define __WARN_printf(taint, fmt, arg...) \
+	__WARN_print_arg(BUGFLAG_TAINT(taint), fmt, ## arg)
+
+#endif /* HAVE_ARCH_BUG_FORMAT */
+
 #include <asm-generic/bug.h>
 
 #endif /* _ASM_X86_BUG_H */
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -108,19 +108,29 @@ __always_inline int is_valid_bugaddr(uns
 __always_inline int decode_bug(unsigned long addr, s32 *imm, int *len)
 {
 	unsigned long start = addr;
+	u8 v, reg, rm, rex = 0;
+	int type = BUG_UD1;
 	bool lock = false;
-	u8 v;
 
 	if (addr < TASK_SIZE_MAX)
 		return BUG_NONE;
 
-	v = *(u8 *)(addr++);
-	if (v == INSN_ASOP)
+	for (;;) {
 		v = *(u8 *)(addr++);
+		if (v == INSN_ASOP)
+			continue;
 
-	if (v == INSN_LOCK) {
-		lock = true;
-		v = *(u8 *)(addr++);
+		if (v == INSN_LOCK) {
+			lock = true;
+			continue;
+		}
+
+		if ((v & 0xf0) == 0x40) {
+			rex = v;
+			continue;
+		}
+
+		break;
 	}
 
 	switch (v) {
@@ -156,10 +166,21 @@ __always_inline int decode_bug(unsigned
 	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)
-			addr += 4; /* RIP + disp32 */
+			addr += 4;	/* RIP + disp32 */
+
+		if (rm == 0)		/* (%eax) */
+			type = BUG_UD1_UBSAN;
+
+		if (rm == 1) {		/* (%ecx) */
+			*imm = reg;
+			type = BUG_UD1_WARN;
+		}
 		break;
 
 	case 1: *imm = *(s8 *)addr;
@@ -176,12 +197,73 @@ __always_inline int decode_bug(unsigned
 	/* record instruction length */
 	*len = addr - start;
 
-	if (X86_MODRM_REG(v) == 0)	/* EAX */
-		return BUG_UD1_UBSAN;
+	return type;
+}
 
-	return BUG_UD1;
+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));
 }
 
+#ifdef HAVE_ARCH_BUG_FORMAT
+/*
+ * Create a va_list from an exception context.
+ */
+void *__warn_args(struct arch_va_list *args, struct pt_regs *regs)
+{
+	/*
+	 * Register save area; populate with function call argument registers
+	 */
+	args->regs[0] = regs->di;
+	args->regs[1] = regs->si;
+	args->regs[2] = regs->dx;
+	args->regs[3] = regs->cx;
+	args->regs[4] = regs->r8;
+	args->regs[5] = regs->r9;
+
+	/*
+	 * From the ABI document:
+	 *
+	 * @gp_offset - the element holds the offset in bytes from
+	 * reg_save_area to the place where the next available general purpose
+	 * argument register is saved. In case all argument registers have
+	 * been exhausted, it is set to the value 48 (6*8).
+	 *
+	 * @fp_offset - the element holds the offset in bytes from
+	 * reg_save_area to the place where the next available floating point
+	 * argument is saved. In case all argument registers have been
+	 * exhausted, it is set to the value 176 (6*8 + 8*16)
+	 *
+	 * @overflow_arg_area - this pointer is used to fetch arguments passed
+	 * on the stack. It is initialized with the address of the first
+	 * argument passed on the stack, if any, and then always updated to
+	 * point to the start of the next argument on the stack.
+	 *
+	 * @reg_save_area - the element points to the start of the register
+	 * save area.
+	 *
+	 * Notably the vararg starts with the second argument and there are no
+	 * floating point arguments in the kernel.
+	 */
+	args->args.gp_offset = 1*8;
+	args->args.fp_offset = 6*8 + 8*16;
+	args->args.reg_save_area = &args->regs;
+	args->args.overflow_arg_area = (void *)regs->sp;
+
+	/*
+	 * If the exception came from __WARN_trap, there is a return
+	 * address on the stack, skip that. This is why any __WARN_trap()
+	 * caller must inhibit tail-call optimization.
+	 */
+	if ((void *)regs->ip == &__WARN_trap)
+		args->args.overflow_arg_area += 8;
+
+	return &args->args;
+}
+#endif /* HAVE_ARCH_BUG_FORMAT */
 
 static nokprobe_inline int
 do_trap_no_signal(struct task_struct *tsk, int trapnr, const char *str,
@@ -334,6 +416,11 @@ static noinstr bool handle_bug(struct pt
 		raw_local_irq_enable();
 
 	switch (ud_type) {
+	case BUG_UD1_WARN:
+		if (report_bug_entry((void *)pt_regs_val(regs, ud_imm), regs) == BUG_TRAP_TYPE_WARN)
+			handled = true;
+		break;
+
 	case BUG_UD2:
 		if (report_bug(regs->ip, regs) == BUG_TRAP_TYPE_WARN) {
 			handled = true;

Re: [PATCH 08/11] x86_64/bug: Implement __WARN_printf()

[Peter Zijlstra]

On Sat, Jun 07, 2025 at 11:42:32AM +0200, Peter Zijlstra wrote:
> @@ -156,10 +166,21 @@ __always_inline int decode_bug(unsigned
>  	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)
> -			addr += 4; /* RIP + disp32 */
> +			addr += 4;	/* RIP + disp32 */
> +
> +		if (rm == 0)		/* (%eax) */
> +			type = BUG_UD1_UBSAN;
> +
> +		if (rm == 1) {		/* (%ecx) */
> +			*imm = reg;
> +			type = BUG_UD1_WARN;
> +		}
>  		break;
>  
>  	case 1: *imm = *(s8 *)addr;
> @@ -176,12 +197,73 @@ __always_inline int decode_bug(unsigned
>  	/* record instruction length */
>  	*len = addr - start;
>  
> -	if (X86_MODRM_REG(v) == 0)	/* EAX */
> -		return BUG_UD1_UBSAN;
> +	return type;
> +}

Oh, this breaks the UBSAN case, it must also set UD1_UBSAN for mod 1 and
2. I'll fix.

[PATCH 09/11] x86/bug: Implement WARN_ONCE()

[Peter Zijlstra]

Implement WARN_ONCE like WARN using BUGFLAG_ONCE.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/bug.h |   14 ++++++++++++++
 include/asm-generic/bug.h  |    2 ++
 2 files changed, 16 insertions(+)

--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -152,6 +152,20 @@ do {									\
 #define __WARN_printf(taint, fmt, arg...) \
 	__WARN_print_arg(BUGFLAG_TAINT(taint), fmt, ## arg)
 
+#define __WARN_ONCE(cond, format, arg...) ({				\
+	int __ret_warn_on = !!(cond);					\
+	if (unlikely(__ret_warn_on)) {					\
+		__WARN_print_arg(BUGFLAG_ONCE|BUGFLAG_TAINT(TAINT_WARN),\
+				format, ## arg);			\
+	}								\
+	__ret_warn_on;							\
+})
+
+/*
+ * Make sure WARN_ONCE() arguments get expanded before trying to count them.
+ */
+#define WARN_ONCE(cond, format...) __WARN_ONCE(cond, format)
+
 #endif /* HAVE_ARCH_BUG_FORMAT */
 
 #include <asm-generic/bug.h>
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -155,8 +155,10 @@ extern __printf(1, 2) void __warn_printk
 	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)

[PATCH 10/11] x86: Clean up default rethunk warning

[Peter Zijlstra]

Replace the funny __warn_thunk thing with a more regular WARN_ONCE()
and simplify the ifdeffery.

Notably this avoids RET from having recursive RETs (once from the
thunk and once from the C function) -- recurive RET makes my head hurt
for no good reason.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/entry.S               |    4 ----
 arch/x86/include/asm/nospec-branch.h |    2 --
 arch/x86/kernel/cpu/bugs.c           |    5 -----
 arch/x86/lib/retpoline.S             |   22 +++++++++++++++-------
 4 files changed, 15 insertions(+), 18 deletions(-)

--- a/arch/x86/entry/entry.S
+++ b/arch/x86/entry/entry.S
@@ -13,8 +13,6 @@
 #include <asm/cpufeatures.h>
 #include <asm/nospec-branch.h>
 
-#include "calling.h"
-
 .pushsection .noinstr.text, "ax"
 
 /* Clobbers AX, CX, DX */
@@ -61,8 +59,6 @@ EXPORT_SYMBOL_GPL(mds_verw_sel);
 
 .popsection
 
-THUNK warn_thunk_thunk, __warn_thunk
-
 /*
  * Clang's implementation of TLS stack cookies requires the variable in
  * question to be a TLS variable. If the variable happens to be defined as an
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -386,8 +386,6 @@ extern void clear_bhb_loop(void);
 
 extern void (*x86_return_thunk)(void);
 
-extern void __warn_thunk(void);
-
 #ifdef CONFIG_MITIGATION_CALL_DEPTH_TRACKING
 extern void call_depth_return_thunk(void);
 
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -3415,8 +3415,3 @@ ssize_t cpu_show_indirect_target_selecti
 	return cpu_show_common(dev, attr, buf, X86_BUG_ITS);
 }
 #endif
-
-void __warn_thunk(void)
-{
-	WARN_ONCE(1, "Unpatched return thunk in use. This should not happen!\n");
-}
--- a/arch/x86/lib/retpoline.S
+++ b/arch/x86/lib/retpoline.S
@@ -12,6 +12,7 @@
 #include <asm/percpu.h>
 #include <asm/frame.h>
 #include <asm/nops.h>
+#include <asm/bug.h>
 
 	.section .text..__x86.indirect_thunk
 
@@ -416,6 +417,19 @@ EXPORT_SYMBOL(its_return_thunk)
 
 #endif /* CONFIG_MITIGATION_ITS */
 
+        .pushsection        .rodata.str1.1
+.Lwarn:
+        .string "Unpatched return thunk in use.\n"
+	.popsection
+
+/*
+ * Helper that will trip WARN_ONCE() after alternatives have ran.
+ */
+#define ALT_WARN_RETHUNK						\
+	ANNOTATE_REACHABLE ;						\
+	1: ALTERNATIVE "", "ud2", X86_FEATURE_ALWAYS ;			\
+	ASM_BUGTABLE_FLAGS(1b, .Lwarn, 0, 0, BUGFLAG_WARNING | BUGFLAG_ONCE )
+
 /*
  * This function name is magical and is used by -mfunction-return=thunk-extern
  * for the compiler to generate JMPs to it.
@@ -432,15 +446,9 @@ EXPORT_SYMBOL(its_return_thunk)
 SYM_CODE_START(__x86_return_thunk)
 	UNWIND_HINT_FUNC
 	ANNOTATE_NOENDBR
-#if defined(CONFIG_MITIGATION_UNRET_ENTRY) || \
-    defined(CONFIG_MITIGATION_SRSO) || \
-    defined(CONFIG_MITIGATION_CALL_DEPTH_TRACKING)
-	ALTERNATIVE __stringify(ANNOTATE_UNRET_SAFE; ret), \
-		   "jmp warn_thunk_thunk", X86_FEATURE_ALWAYS
-#else
+	ALT_WARN_RETHUNK
 	ANNOTATE_UNRET_SAFE
 	ret
-#endif
 	int3
 SYM_CODE_END(__x86_return_thunk)
 SYM_PIC_ALIAS(__x86_return_thunk)

[PATCH 11/11] x86_64/bug: Inline the UD1

[Peter Zijlstra]

(Ab)use the static_call infrastructure to convert all:

  call __WARN_trap

instances into the desired:

  ud1 (%ecx), %rdi

eliminating the CALL/RET, but more importantly, fixing the
fact that all WARNs will have:

  RIP: 0010:__WARN_trap+0

Basically, by making it a static_call trampoline call, objtool will
collect the callsites, and then the inline rewrite will hit the
special case and replace the code with the magic instruction.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/bug.h    |    5 ++++-
 arch/x86/kernel/static_call.c |   13 +++++++++++--
 arch/x86/kernel/traps.c       |    4 ++++
 3 files changed, 19 insertions(+), 3 deletions(-)

--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -114,9 +114,12 @@ do {								\
 #ifdef HAVE_ARCH_BUG_FORMAT
 
 #ifndef __ASSEMBLY__
+#include <linux/static_call_types.h>
 struct bug_entry;
 extern void __WARN_trap(struct bug_entry *bug, ...);
 
+DECLARE_STATIC_CALL(WARN_trap, __WARN_trap);
+
 struct pt_regs;
 struct sysv_va_list { /* from AMD64 System V ABI */
 	unsigned int gp_offset;
@@ -145,7 +148,7 @@ extern void *__warn_args(struct arch_va_
 #define __WARN_print_arg(flags, format, arg...)				\
 do {									\
 	int __flags = (flags) | BUGFLAG_WARNING | BUGFLAG_ARGS ;	\
-	__WARN_trap(__WARN_bug_entry(__flags, format), ## arg);		\
+	static_call_mod(WARN_trap)(__WARN_bug_entry(__flags, format), ## arg); \
 	asm (""); /* inhibit tail-call optimization */			\
 } while (0)
 
--- a/arch/x86/kernel/static_call.c
+++ b/arch/x86/kernel/static_call.c
@@ -26,6 +26,11 @@ static const u8 xor5rax[] = { 0x2e, 0x2e
 
 static const u8 retinsn[] = { RET_INSN_OPCODE, 0xcc, 0xcc, 0xcc, 0xcc };
 
+/*
+ * ud1    (%ecx),%rdi -- see __WARN_trap() / decode_bug()
+ */
+static const u8 warninsn[] = { 0x67, 0x48, 0x0f, 0xb9, 0x39 };
+
 static u8 __is_Jcc(u8 *insn) /* Jcc.d32 */
 {
 	u8 ret = 0;
@@ -69,7 +74,10 @@ static void __ref __static_call_transfor
 			emulate = code;
 			code = &xor5rax;
 		}
-
+		if (func == &__WARN_trap) {
+			emulate = code;
+			code = &warninsn;
+		}
 		break;
 
 	case NOP:
@@ -128,7 +136,8 @@ static void __static_call_validate(u8 *i
 	} else {
 		if (opcode == CALL_INSN_OPCODE ||
 		    !memcmp(insn, x86_nops[5], 5) ||
-		    !memcmp(insn, xor5rax, 5))
+		    !memcmp(insn, xor5rax, 5) ||
+		    !memcmp(insn, warninsn, 5))
 			return;
 	}
 
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -31,6 +31,7 @@
 #include <linux/kexec.h>
 #include <linux/sched.h>
 #include <linux/sched/task_stack.h>
+#include <linux/static_call.h>
 #include <linux/timer.h>
 #include <linux/init.h>
 #include <linux/bug.h>
@@ -209,6 +210,9 @@ static inline unsigned long pt_regs_val(
 }
 
 #ifdef HAVE_ARCH_BUG_FORMAT
+DEFINE_STATIC_CALL(WARN_trap, __WARN_trap);
+EXPORT_STATIC_CALL_TRAMP(WARN_trap);
+
 /*
  * Create a va_list from an exception context.
  */

Re: [PATCH 00/11] x86: WARN() hackery

[Linus Torvalds]

On Sat, 7 Jun 2025 at 03:05, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Slightly less mad this time :-)

Looks a lot better to me - at least by looking at the patches.

So I didn't actually apply the series to test out what the end result
looks like, but from just the patches this looks much nicer. Thanks,

         Linus

Re: [PATCH 00/11] x86: WARN() hackery

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

Hi,

On Sat, Jun 07, 2025 at 11:42:24AM +0200, Peter Zijlstra wrote:
> Slightly less mad this time :-)
> 
> The primary purpose of all this is to get the WARN() printk() and
> __warn() calls into the same context. Notably the current state is that
> WARN() ends up doing printk() in-place, then takes an exception and has
> the exception do the __warn().
> 
> The problem with all this is the ONCE logic. Normal WARN_ON_ONCE()
> (without the printk) has the ONCE logic in the exception
> (__report_bug()). But because WARN() essentially results in two distinct
> actions (printk + trap) this cannot work.  With the result that
> additional ONCE logic is sprinkled around for each such site.
> 
> Current proposals look to make this worse by adding KUnit checks for all
> this, including regular WARN. Making the per-instance code overhead even
> worse.
> 
> As such, by moving the printk() into the exception, and having the
> exception (__report_bug() in fact) do everything, we get rid of the
> external ONCE logic and provide a cental place for additional conditions
> without need to litter all the instances.

Thanks a lot for working on that. What is the status of this patch? It
looks like Linus was happy with it, and I understood that you felt it
was a blocker for the kunit warning work we'd really like to get merged
at some point.

Thanks again,
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 00/11] x86: WARN() hackery

[Peter Zijlstra]

On Thu, Jul 03, 2025 at 03:40:01PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Jun 07, 2025 at 11:42:24AM +0200, Peter Zijlstra wrote:
> > Slightly less mad this time :-)
> > 
> > The primary purpose of all this is to get the WARN() printk() and
> > __warn() calls into the same context. Notably the current state is that
> > WARN() ends up doing printk() in-place, then takes an exception and has
> > the exception do the __warn().
> > 
> > The problem with all this is the ONCE logic. Normal WARN_ON_ONCE()
> > (without the printk) has the ONCE logic in the exception
> > (__report_bug()). But because WARN() essentially results in two distinct
> > actions (printk + trap) this cannot work.  With the result that
> > additional ONCE logic is sprinkled around for each such site.
> > 
> > Current proposals look to make this worse by adding KUnit checks for all
> > this, including regular WARN. Making the per-instance code overhead even
> > worse.
> > 
> > As such, by moving the printk() into the exception, and having the
> > exception (__report_bug() in fact) do everything, we get rid of the
> > external ONCE logic and provide a cental place for additional conditions
> > without need to litter all the instances.
> 
> Thanks a lot for working on that. What is the status of this patch? It
> looks like Linus was happy with it, and I understood that you felt it
> was a blocker for the kunit warning work we'd really like to get merged
> at some point.

I talked to Mark Rutland about arm64 support, to see if the things I did
are generic enough for other architectures to support, or if there's
anything I need to change.

He said he was going to have a poke, but it appears he's not had time
yet.

Once he's happy, I think we can move this forward. Obviously I have to
rebase on top of the things Ingo stuck in meanwhile, but that shouldn't
be too hard.

Re: [PATCH 00/11] x86: WARN() hackery

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

On Thu, Jul 03, 2025 at 03:47:12PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 03, 2025 at 03:40:01PM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Sat, Jun 07, 2025 at 11:42:24AM +0200, Peter Zijlstra wrote:
> > > Slightly less mad this time :-)
> > > 
> > > The primary purpose of all this is to get the WARN() printk() and
> > > __warn() calls into the same context. Notably the current state is that
> > > WARN() ends up doing printk() in-place, then takes an exception and has
> > > the exception do the __warn().
> > > 
> > > The problem with all this is the ONCE logic. Normal WARN_ON_ONCE()
> > > (without the printk) has the ONCE logic in the exception
> > > (__report_bug()). But because WARN() essentially results in two distinct
> > > actions (printk + trap) this cannot work.  With the result that
> > > additional ONCE logic is sprinkled around for each such site.
> > > 
> > > Current proposals look to make this worse by adding KUnit checks for all
> > > this, including regular WARN. Making the per-instance code overhead even
> > > worse.
> > > 
> > > As such, by moving the printk() into the exception, and having the
> > > exception (__report_bug() in fact) do everything, we get rid of the
> > > external ONCE logic and provide a cental place for additional conditions
> > > without need to litter all the instances.
> > 
> > Thanks a lot for working on that. What is the status of this patch? It
> > looks like Linus was happy with it, and I understood that you felt it
> > was a blocker for the kunit warning work we'd really like to get merged
> > at some point.
> 
> I talked to Mark Rutland about arm64 support, to see if the things I did
> are generic enough for other architectures to support, or if there's
> anything I need to change.
> 
> He said he was going to have a poke, but it appears he's not had time
> yet.
> 
> Once he's happy, I think we can move this forward. Obviously I have to
> rebase on top of the things Ingo stuck in meanwhile, but that shouldn't
> be too hard.

Ack, thanks for the update :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]