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

[Kees Cook <kees@kernel.org>]

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