+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
