Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1586880 > unrolled thread

[PATCH] x86: Implement __WARN using UD0

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86: Implement __WARN using UD0 Peter Zijlstra <peterz@infradead.org> - 2017-02-23 14:30 +0100
    Re: [PATCH] x86: Implement __WARN using UD0 Arjan van de Ven <arjan@linux.intel.com> - 2017-02-23 15:20 +0100
      Re: [PATCH] x86: Implement __WARN using UD0 Peter Zijlstra <peterz@infradead.org> - 2017-02-23 16:00 +0100
        Re: [PATCH] x86: Implement __WARN using UD0 Ingo Molnar <mingo@kernel.org> - 2017-02-24 09:00 +0100
          Re: [PATCH] x86: Implement __WARN using UD0 Peter Zijlstra <peterz@infradead.org> - 2017-02-24 09:40 +0100
            Re: [PATCH] x86: Implement __WARN using UD0 Ingo Molnar <mingo@kernel.org> - 2017-02-24 10:20 +0100
              Re: [PATCH] x86: Implement __WARN using UD0 Borislav Petkov <bp@alien8.de> - 2017-02-24 10:50 +0100
                Re: [PATCH] x86: Implement __WARN using UD0 "H. Peter Anvin" <hpa@zytor.com> - 2017-02-24 12:50 +0100
              Re: [PATCH] x86: Implement __WARN using UD0 Peter Zijlstra <peterz@infradead.org> - 2017-02-24 12:50 +0100
                Re: [PATCH] x86: Implement __WARN using UD0 Borislav Petkov <bp@alien8.de> - 2017-02-25 11:50 +0100
                  Re: [PATCH] x86: Implement __WARN using UD0 hpa@zytor.com - 2017-02-25 19:00 +0100
                    Re: [PATCH] x86: Implement __WARN using UD0 Borislav Petkov <bp@alien8.de> - 2017-02-25 20:40 +0100
                      Re: [PATCH] x86: Implement __WARN using UD0 Borislav Petkov <bp@alien8.de> - 2017-02-25 21:40 +0100
                      Re: [PATCH] x86: Implement __WARN using UD0 hpa@zytor.com - 2017-02-25 21:50 +0100
            Re: [PATCH] x86: Implement __WARN using UD0 hpa@zytor.com - 2017-02-24 10:20 +0100
            Re: [PATCH] x86: Implement __WARN using UD0 hpa@zytor.com - 2017-02-24 10:50 +0100
    Re: [PATCH] x86: Implement __WARN using UD0 Peter Zijlstra <peterz@infradead.org> - 2017-02-23 15:20 +0100
      Re: [PATCH] x86: Implement __WARN using UD0 Peter Zijlstra <peterz@infradead.org> - 2017-02-23 16:00 +0100
      Re: [PATCH] x86: Implement __WARN using UD0 hpa@zytor.com - 2017-02-23 16:20 +0100
        Re: [PATCH] x86: Implement __WARN using UD0 Peter Zijlstra <peterz@infradead.org> - 2017-02-23 16:30 +0100
          Re: [PATCH] x86: Implement __WARN using UD0 hpa@zytor.com - 2017-02-23 16:40 +0100
            Re: [PATCH] x86: Implement __WARN using UD0 Borislav Petkov <bp@alien8.de> - 2017-02-23 17:10 +0100
    Re: [PATCH] x86: Implement __WARN using UD0 Josh Poimboeuf <jpoimboe@redhat.com> - 2017-02-23 15:20 +0100
      Re: [PATCH] x86: Implement __WARN using UD0 Borislav Petkov <bp@alien8.de> - 2017-02-23 15:30 +0100
      Re: [PATCH] x86: Implement __WARN using UD0 Peter Zijlstra <peterz@infradead.org> - 2017-02-23 15:40 +0100
    [PATCH -v2] x86: Implement __WARN using UD0 Peter Zijlstra <peterz@infradead.org> - 2017-02-24 12:20 +0100
      [RFC][PATCH] bug: Add _ONCE logic to report_bug() Peter Zijlstra <peterz@infradead.org> - 2017-02-25 10:10 +0100
        Re: [RFC][PATCH] bug: Add _ONCE logic to report_bug() Ingo Molnar <mingo@kernel.org> - 2017-02-25 10:20 +0100
          Re: [RFC][PATCH] bug: Add _ONCE logic to report_bug() Peter Zijlstra <peterz@infradead.org> - 2017-02-25 11:50 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1586959

On February 23, 2017 7:23:09 AM PST, Peter Zijlstra <peterz@infradead.org> wrote:
>On Thu, Feb 23, 2017 at 07:09:05AM -0800, hpa@zytor.com wrote:
>> Well, it only matters if the instruction extends past a segment
>> boundary or page.  However, the CPU instruction decoder will consume
>a
>> modrm for UD1, and so using just the two opcode bytes may cause a #PF
>> or #GP when a #UD was intended.
>
>It also matters if you want the decoded instruction stream to make
>sense.
>
>If for instance I use UD1 without the ModRM byte for WARN, objtool gets
>mighty confused because the instruction stream doesn't decode properly.
>
>objtool will also consume the extra byte and then the next instruction
>is offset and decodes wrong and it stresses out.
>
>Similarly, if you were to do objdump (and objdump were to actually
>correctly decode UD1) then the resulting asm would make no sense.
>
>The kernel will work 'fine', because even without ModRM it will #UD,
>and
>the #UD handler will IP+=2 and all is well, but it becomes impossible
>to
>actually decode the function..

Well, once you are using invalid instructions, it depends not on what the CPU decodes but what your own handler expects.  Consider Microsoft's use of C4 C4 /ib as a meta-instruction (called BOP, "BIOS operation")... that format has nothing to do with the CPU, but if you want to disassemble the resulting code you need to know about how they encode BOP.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1586982

On Thu, Feb 23, 2017 at 07:32:07AM -0800, hpa@zytor.com wrote:
> Well, once you are using invalid instructions, it depends not on
> what the CPU decodes but what your own handler expects. Consider
> Microsoft's use of C4 C4 /ib as a meta-instruction (called BOP, "BIOS
> operation")... that format has nothing to do with the CPU, but if you
> want to disassemble the resulting code you need to know about how they
> encode BOP.

How do they use that?

They rely on the fact that C4 C4 is going to #UD as it is an invalid VEX
insn? The second C4 selecting the 100b map which is reserved?

Or?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1586912

On Thu, Feb 23, 2017 at 02:28:13PM +0100, Peter Zijlstra wrote:
> +/*
> + * Only UD2 is defined in the Intel SDM and AMD64 docs,
> + * but the interweb provided UD0 and UD1:
> + *
> + *   https://groups.google.com/forum/#!topic/alt.os.assembly/_rS4L0fnqGE

FYI, the latest Intel SDM does have UD0 and UD1.

I guess AMD64 and hypervisors don't need to know about it, since if they
don't, it will still trigger the same invalid opcode exception anyway?

Any idea what the functional difference is between UD0 and UD2?

> + * Since some emulators terminate on UD2, we cannot use it for WARN.

That's too bad, which emulators are those?

> + * Since various instruction decoders disagree on the length of UD1,
> + * we cannot use it either. So use UD0 for WARN.

It shows up as "(bad)" in objtool and gdb, though it still gets the
length right:

  8b9:       0f ff                   (bad)  

It would be nice if the tools knew about it...

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1586913

On Thu, Feb 23, 2017 at 08:12:32AM -0600, Josh Poimboeuf wrote:
> I guess AMD64 and hypervisors don't need to know about it, since if they

So the insns are mentioned in AMD's APMv2, in the instruction tables at
the end. Just not fully documented like UD2.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1586921

On Thu, Feb 23, 2017 at 08:12:32AM -0600, Josh Poimboeuf wrote:
> On Thu, Feb 23, 2017 at 02:28:13PM +0100, Peter Zijlstra wrote:
> > +/*
> > + * Only UD2 is defined in the Intel SDM and AMD64 docs,
> > + * but the interweb provided UD0 and UD1:
> > + *
> > + *   https://groups.google.com/forum/#!topic/alt.os.assembly/_rS4L0fnqGE
> 
> FYI, the latest Intel SDM does have UD0 and UD1.

Ooh, shiny, a new one.

> > + * Since various instruction decoders disagree on the length of UD1,
> > + * we cannot use it either. So use UD0 for WARN.
> 
> It shows up as "(bad)" in objtool and gdb, though it still gets the
> length right:
> 
>   8b9:       0f ff                   (bad)  
> 
> It would be nice if the tools knew about it...

Now that the SDM lists them, I suppose that's going to be any day now
:-)

[toc] | [prev] | [next] | [standalone]

#1587526 — [PATCH -v2] x86: Implement __WARN using UD0

By using "UD0" for WARNs we remove the function call and its possible
__FILE__ and __LINE__ immediate arguments from the instruction stream.

Total image size will not change much, what we win in the instrucion
stream we'll loose because of the __bug_table entries. Still, saves on
I$ footprint and register pressure at the callsites.

  text            data     bss    dec              hex    filename                     size

  10503189        4442584  843776 15789549         f0eded defconfig-build/vmlinux.pre  25242904
  10483798        4442584  843776 15770158         f0a22e defconfig-build/vmlinux.post 25243504

Cc: Richard Weinberger <richard.weinberger@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
-v2:

 - Added ASM comments to the __bug_table[] entries describing the fields
   (requested by bpetkov).
 - Added /* */ markers to the #else and #endif (also bpetkov)
 - Fixed the x86_64 kernel address test in is_valid_bugaddr().
 - Removed the random interweb link because the latest Intel SDM
   (Dec'16) actually lists the UD0/UD1/UD2 instructions.

 arch/um/Kconfig.common         |    5 ---
 arch/x86/include/asm/bug.h     |   68 ++++++++++++++++++++++++++++++-----------
 arch/x86/kernel/dumpstack.c    |    3 -
 arch/x86/kernel/dumpstack_32.c |   12 -------
 arch/x86/kernel/dumpstack_64.c |   10 ------
 arch/x86/kernel/traps.c        |   50 ++++++++++++++++++++++++++----
 arch/x86/um/Makefile           |    2 -
 arch/x86/um/bug.c              |   21 ------------
 8 files changed, 96 insertions(+), 75 deletions(-)

--- a/arch/um/Kconfig.common
+++ b/arch/um/Kconfig.common
@@ -50,11 +50,6 @@ config GENERIC_CALIBRATE_DELAY
 	bool
 	default y
 
-config GENERIC_BUG
-	bool
-	default y
-	depends on BUG
-
 config HZ
 	int
 	default 100
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -1,36 +1,70 @@
 #ifndef _ASM_X86_BUG_H
 #define _ASM_X86_BUG_H
 
+#include <linux/stringify.h>
+
 #define HAVE_ARCH_BUG
 
-#ifdef CONFIG_DEBUG_BUGVERBOSE
+/*
+ * Since some emulators terminate on UD2, we cannot use it for WARN.
+ * Since various instruction decoders disagree on the length of UD1,
+ * we cannot use it either. So use UD0 for WARN.
+ *
+ * (binutils knows about "ud1" but {en,de}codes it as 2 bytes, whereas
+ *  our kernel decoder thinks it takes a ModRM byte, which seems consistent
+ *  with various things like the Intel SDM instruction encoding rules)
+ */
+
+#define ASM_UD0		".byte 0x0f, 0xff"
+#define ASM_UD1		".byte 0x0f, 0xb9" /* + ModRM */
+#define ASM_UD2		".byte 0x0f, 0x0b"
 
 #ifdef CONFIG_X86_32
-# define __BUG_C0	"2:\t.long 1b, %c0\n"
+# define __BUG_REL(val)	".long " __stringify(val)
 #else
-# define __BUG_C0	"2:\t.long 1b - 2b, %c0 - 2b\n"
+# define __BUG_REL(val)	".long " __stringify(val) " - 2b"
 #endif
 
-#define BUG()							\
-do {								\
-	asm volatile("1:\tud2\n"				\
-		     ".pushsection __bug_table,\"a\"\n"		\
-		     __BUG_C0					\
-		     "\t.word %c1, 0\n"				\
-		     "\t.org 2b+%c2\n"				\
-		     ".popsection"				\
-		     : : "i" (__FILE__), "i" (__LINE__),	\
-		     "i" (sizeof(struct bug_entry)));		\
-	unreachable();						\
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+
+#define _BUG_FLAGS(ins, flags)						\
+do {									\
+	asm volatile("1:\t" ins "\n"					\
+		     ".pushsection __bug_table,\"a\"\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"					\
+		     : : "i" (__FILE__), "i" (__LINE__),		\
+			 "i" (flags),					\
+			 "i" (sizeof(struct bug_entry)));		\
 } while (0)
 
-#else
+#else /* !CONFIG_DEBUG_BUGVERBOSE */
+
+#define _BUG_FLAGS(ins, flags)						\
+do {									\
+	asm volatile("1:\t" ins "\n"					\
+		     ".pushsection __bug_table,\"a\"\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"					\
+		     : : "i" (flags),					\
+			 "i" (sizeof(struct bug_entry)));		\
+} while (0)
+
+#endif /* CONFIG_DEBUG_BUGVERBOSE */
+
 #define BUG()							\
 do {								\
-	asm volatile("ud2");					\
+	_BUG_FLAGS(ASM_UD2, 0);					\
 	unreachable();						\
 } while (0)
-#endif
+
+#define __WARN_TAINT(taint)	_BUG_FLAGS(ASM_UD0, BUGFLAG_TAINT(taint))
 
 #include <asm-generic/bug.h>
 
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -289,9 +289,6 @@ void die(const char *str, struct pt_regs
 	unsigned long flags = oops_begin();
 	int sig = SIGSEGV;
 
-	if (!user_mode(regs))
-		report_bug(regs->ip, regs);
-
 	if (__die(str, regs, err))
 		sig = 0;
 	oops_end(flags, regs, sig);
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -162,15 +162,3 @@ void show_regs(struct pt_regs *regs)
 	}
 	pr_cont("\n");
 }
-
-int is_valid_bugaddr(unsigned long ip)
-{
-	unsigned short ud2;
-
-	if (ip < PAGE_OFFSET)
-		return 0;
-	if (probe_kernel_address((unsigned short *)ip, ud2))
-		return 0;
-
-	return ud2 == 0x0b0f;
-}
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -178,13 +178,3 @@ void show_regs(struct pt_regs *regs)
 	}
 	pr_cont("\n");
 }
-
-int is_valid_bugaddr(unsigned long ip)
-{
-	unsigned short ud2;
-
-	if (__copy_from_user(&ud2, (const void __user *) ip, sizeof(ud2)))
-		return 0;
-
-	return ud2 == 0x0b0f;
-}
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -169,6 +169,41 @@ void ist_end_non_atomic(void)
 	preempt_disable();
 }
 
+int is_valid_bugaddr(unsigned long addr)
+{
+	unsigned short ud;
+
+#ifdef CONFIG_X86_32
+	if (addr < PAGE_OFFSET)
+		return 0;
+#else
+	if ((long)addr > 0)
+		return 0;
+#endif
+	if (probe_kernel_address((unsigned short *)addr, ud))
+		return 0;
+
+	return ud == 0x0b0f || ud == 0xff0f;
+}
+
+static int fixup_bug(struct pt_regs *regs, int trapnr)
+{
+	if (trapnr != X86_TRAP_UD)
+		return 0;
+
+	switch (report_bug(regs->ip, regs)) {
+	case BUG_TRAP_TYPE_NONE:
+	case BUG_TRAP_TYPE_BUG:
+		break;
+
+	case BUG_TRAP_TYPE_WARN:
+		regs->ip += 2;
+		return 1;
+	}
+
+	return 0;
+}
+
 static nokprobe_inline int
 do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		  struct pt_regs *regs,	long error_code)
@@ -187,12 +222,15 @@ do_trap_no_signal(struct task_struct *ts
 	}
 
 	if (!user_mode(regs)) {
-		if (!fixup_exception(regs, trapnr)) {
-			tsk->thread.error_code = error_code;
-			tsk->thread.trap_nr = trapnr;
-			die(str, regs, error_code);
-		}
-		return 0;
+		if (fixup_exception(regs, trapnr))
+			return 0;
+
+		if (fixup_bug(regs, trapnr))
+			return 0;
+
+		tsk->thread.error_code = error_code;
+		tsk->thread.trap_nr = trapnr;
+		die(str, regs, error_code);
 	}
 
 	return -1;
--- a/arch/x86/um/Makefile
+++ b/arch/x86/um/Makefile
@@ -8,7 +8,7 @@ else
 	BITS := 64
 endif
 
-obj-y = bug.o bugs_$(BITS).o delay.o fault.o ldt.o \
+obj-y = bugs_$(BITS).o delay.o fault.o ldt.o \
 	ptrace_$(BITS).o ptrace_user.o setjmp_$(BITS).o signal.o \
 	stub_$(BITS).o stub_segv.o \
 	sys_call_table_$(BITS).o sysrq_$(BITS).o tls_$(BITS).o \
--- a/arch/x86/um/bug.c
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * Copyright (C) 2006 Jeff Dike (jdike@addtoit.com)
- * Licensed under the GPL V2
- */
-
-#include <linux/uaccess.h>
-
-/*
- * Mostly copied from i386/x86_86 - eliminated the eip < PAGE_OFFSET because
- * that's not relevant in skas mode.
- */
-
-int is_valid_bugaddr(unsigned long eip)
-{
-	unsigned short ud2;
-
-	if (probe_kernel_address((unsigned short __user *)eip, ud2))
-		return 0;
-
-	return ud2 == 0x0b0f;
-}

[toc] | [prev] | [next] | [standalone]

#1588096 — [RFC][PATCH] bug: Add _ONCE logic to report_bug()

Josh suggested moving the _ONCE logic inside the trap handler, using a
bit in the bug_entry::flags field, avoiding the need for the extra
variable.

Sadly this only works for WARN_ON_ONCE(), since the others have
printk() statements prior to triggering the trap.

Still, this saves some text and data:

  text            data     bss    dec              hex    filename
  10469505        4443448  843776 15756729         f06db9 defconfig-build/vmlinux-ud0
  10452803        4442616  843776 15739195         f0293b defconfig-build/vmlinux-ud0-once

(Only compile tested on x86_64 so far.)

Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/arm64/include/asm/bug.h      |    2 +-
 arch/parisc/include/asm/bug.h     |    8 ++++----
 arch/powerpc/include/asm/bug.h    |    4 ++--
 arch/s390/include/asm/bug.h       |    4 ++--
 arch/sh/include/asm/bug.h         |    4 ++--
 arch/x86/include/asm/bug.h        |    2 +-
 include/asm-generic/bug.h         |   18 +++++++++++++++++-
 include/asm-generic/vmlinux.lds.h |    3 +--
 include/linux/bug.h               |    2 +-
 lib/bug.c                         |   28 ++++++++++++++++++++--------
 10 files changed, 51 insertions(+), 24 deletions(-)

--- a/arch/arm64/include/asm/bug.h
+++ b/arch/arm64/include/asm/bug.h
@@ -55,7 +55,7 @@ _BUGVERBOSE_LOCATION(__FILE__, __LINE__)
 	unreachable();				\
 } while (0)
 
-#define __WARN_TAINT(taint) _BUG_FLAGS(BUGFLAG_TAINT(taint))
+#define __WARN_FLAGS(flags) _BUG_FLAGS(BUGFLAG_WARNING|(flags))
 
 #endif /* ! CONFIG_GENERIC_BUG */
 
--- a/arch/parisc/include/asm/bug.h
+++ b/arch/parisc/include/asm/bug.h
@@ -46,7 +46,7 @@
 #endif
 
 #ifdef CONFIG_DEBUG_BUGVERBOSE
-#define __WARN_TAINT(taint)						\
+#define __WARN_FLAGS(flags)						\
 	do {								\
 		asm volatile("\n"					\
 			     "1:\t" PARISC_BUG_BREAK_ASM "\n"		\
@@ -56,11 +56,11 @@
 			     "\t.org 2b+%c3\n"				\
 			     "\t.popsection"				\
 			     : : "i" (__FILE__), "i" (__LINE__),	\
-			     "i" (BUGFLAG_TAINT(taint)), 		\
+			     "i" (BUGFLAG_WARNING|(flags)),		\
 			     "i" (sizeof(struct bug_entry)) );		\
 	} while(0)
 #else
-#define __WARN_TAINT(taint)						\
+#define __WARN_FLAGS(flags)						\
 	do {								\
 		asm volatile("\n"					\
 			     "1:\t" PARISC_BUG_BREAK_ASM "\n"		\
@@ -69,7 +69,7 @@
 			     "\t.short %c0\n"				\
 			     "\t.org 2b+%c1\n"				\
 			     "\t.popsection"				\
-			     : : "i" (BUGFLAG_TAINT(taint)),		\
+			     : : "i" (BUGFLAG_WARNING|(flags)),		\
 			     "i" (sizeof(struct bug_entry)) );		\
 	} while(0)
 #endif
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -85,12 +85,12 @@
 	}							\
 } while (0)
 
-#define __WARN_TAINT(taint) do {				\
+#define __WARN_FLAGS(flags) do {				\
 	__asm__ __volatile__(					\
 		"1:	twi 31,0,0\n"				\
 		_EMIT_BUG_ENTRY					\
 		: : "i" (__FILE__), "i" (__LINE__),		\
-		  "i" (BUGFLAG_TAINT(taint)),			\
+		  "i" (BUGFLAG_WARNING|(flags)),		\
 		  "i" (sizeof(struct bug_entry)));		\
 } while (0)
 
--- a/arch/s390/include/asm/bug.h
+++ b/arch/s390/include/asm/bug.h
@@ -46,8 +46,8 @@
 	unreachable();					\
 } while (0)
 
-#define __WARN_TAINT(taint) do {			\
-	__EMIT_BUG(BUGFLAG_TAINT(taint));		\
+#define __WARN_FLAGS(flags) do {			\
+	__EMIT_BUG(BUGFLAG_WARNING|(flags));		\
 } while (0)
 
 #define WARN_ON(x) ({					\
--- a/arch/sh/include/asm/bug.h
+++ b/arch/sh/include/asm/bug.h
@@ -50,7 +50,7 @@ do {							\
 		   "i" (sizeof(struct bug_entry)));	\
 } while (0)
 
-#define __WARN_TAINT(taint)				\
+#define __WARN_FLAGS(flags)				\
 do {							\
 	__asm__ __volatile__ (				\
 		"1:\t.short %O0\n"			\
@@ -59,7 +59,7 @@ do {							\
 		 : "n" (TRAPA_BUG_OPCODE),		\
 		   "i" (__FILE__),			\
 		   "i" (__LINE__),			\
-		   "i" (BUGFLAG_TAINT(taint)),		\
+		   "i" (BUGFLAG_WARNING|(flags)),	\
 		   "i" (sizeof(struct bug_entry)));	\
 } while (0)
 
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -64,7 +64,7 @@ do {								\
 	unreachable();						\
 } while (0)
 
-#define __WARN_TAINT(taint)	_BUG_FLAGS(ASM_UD0, BUGFLAG_TAINT(taint))
+#define __WARN_FLAGS(flags)	_BUG_FLAGS(ASM_UD0, BUGFLAG_WARNING|(flags))
 
 #include <asm-generic/bug.h>
 
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -5,6 +5,8 @@
 
 #ifdef CONFIG_GENERIC_BUG
 #define BUGFLAG_WARNING		(1 << 0)
+#define BUGFLAG_ONCE		(1 << 1)
+#define BUGFLAG_DONE		(1 << 2)
 #define BUGFLAG_TAINT(taint)	(BUGFLAG_WARNING | ((taint) << 8))
 #define BUG_GET_TAINT(bug)	((bug)->flags >> 8)
 #endif
@@ -55,6 +57,18 @@ struct bug_entry {
 #define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
 #endif
 
+#ifdef __WARN_FLAGS
+#define __WARN_TAINT(taint)		__WARN_FLAGS(BUGFLAG_TAINT(taint))
+#define __WARN_ONCE_TAINT(taint)	__WARN_FLAGS(BUGFLAG_ONCE|BUGFLAG_TAINT(taint))
+
+#define WARN_ON_ONCE(condition) ({				\
+	int __ret_warn_on = !!(condition);			\
+	if (unlikely(__ret_warn_on))				\
+		__WARN_ONCE_TAINT(TAINT_WARN);			\
+	unlikely(__ret_warn_on);				\
+})
+#endif
+
 /*
  * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
  * significant issues that need prompt attention if they should ever
@@ -97,7 +111,7 @@ void __warn(const char *file, int line,
 #endif
 
 #ifndef WARN
-#define WARN(condition, format...) ({						\
+#define WARN(condition, format...) ({					\
 	int __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on))					\
 		__WARN_printf(format);					\
@@ -112,6 +126,7 @@ void __warn(const char *file, int line,
 	unlikely(__ret_warn_on);					\
 })
 
+#ifndef WARN_ON_ONCE
 #define WARN_ON_ONCE(condition)	({				\
 	static bool __section(.data.unlikely) __warned;		\
 	int __ret_warn_once = !!(condition);			\
@@ -122,6 +137,7 @@ void __warn(const char *file, int line,
 	}							\
 	unlikely(__ret_warn_once);				\
 })
+#endif
 
 #define WARN_ONCE(condition, format...)	({			\
 	static bool __section(.data.unlikely) __warned;		\
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -286,8 +286,6 @@
 		*(.rodata1)						\
 	}								\
 									\
-	BUG_TABLE							\
-									\
 	/* PCI quirks */						\
 	.pci_fixup        : AT(ADDR(.pci_fixup) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start_pci_fixups_early) = .;		\
@@ -855,6 +853,7 @@
 		READ_MOSTLY_DATA(cacheline)				\
 		DATA_DATA						\
 		CONSTRUCTORS						\
+		BUG_TABLE						\
 	}
 
 #define INIT_TEXT_SECTION(inittext_align)				\
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -105,7 +105,7 @@ static inline int is_warning_bug(const s
 	return bug->flags & BUGFLAG_WARNING;
 }
 
-const struct bug_entry *find_bug(unsigned long bugaddr);
+struct bug_entry *find_bug(unsigned long bugaddr);
 
 enum bug_trap_type report_bug(unsigned long bug_addr, struct pt_regs *regs);
 
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -47,7 +47,7 @@
 #include <linux/sched.h>
 #include <linux/rculist.h>
 
-extern const struct bug_entry __start___bug_table[], __stop___bug_table[];
+extern struct bug_entry __start___bug_table[], __stop___bug_table[];
 
 static inline unsigned long bug_addr(const struct bug_entry *bug)
 {
@@ -62,10 +62,10 @@ static inline unsigned long bug_addr(con
 /* Updates are protected by module mutex */
 static LIST_HEAD(module_bug_list);
 
-static const struct bug_entry *module_find_bug(unsigned long bugaddr)
+static struct bug_entry *module_find_bug(unsigned long bugaddr)
 {
 	struct module *mod;
-	const struct bug_entry *bug = NULL;
+	struct bug_entry *bug = NULL;
 
 	rcu_read_lock_sched();
 	list_for_each_entry_rcu(mod, &module_bug_list, bug_list) {
@@ -122,15 +122,15 @@ void module_bug_cleanup(struct module *m
 
 #else
 
-static inline const struct bug_entry *module_find_bug(unsigned long bugaddr)
+static inline struct bug_entry *module_find_bug(unsigned long bugaddr)
 {
 	return NULL;
 }
 #endif
 
-const struct bug_entry *find_bug(unsigned long bugaddr)
+struct bug_entry *find_bug(unsigned long bugaddr)
 {
-	const struct bug_entry *bug;
+	struct bug_entry *bug;
 
 	for (bug = __start___bug_table; bug < __stop___bug_table; ++bug)
 		if (bugaddr == bug_addr(bug))
@@ -141,9 +141,9 @@ const struct bug_entry *find_bug(unsigne
 
 enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 {
-	const struct bug_entry *bug;
+	struct bug_entry *bug;
 	const char *file;
-	unsigned line, warning;
+	unsigned line, warning, once, done;
 
 	if (!is_valid_bugaddr(bugaddr))
 		return BUG_TRAP_TYPE_NONE;
@@ -164,6 +164,18 @@ enum bug_trap_type report_bug(unsigned l
 		line = bug->line;
 #endif
 		warning = (bug->flags & BUGFLAG_WARNING) != 0;
+		once = (bug->flags & BUGFLAG_ONCE) != 0;
+		done = (bug->flags & BUGFLAG_DONE) != 0;
+
+		if (warning && once) {
+			if (done)
+				return BUG_TRAP_TYPE_WARN;
+
+			/*
+			 * Since this is the only store, concurrency is not an issue.
+			 */
+			bug->flags |= BUGFLAG_DONE;
+		}
 	}
 
 	if (warning) {

[toc] | [prev] | [next] | [standalone]

#1588098 — Re: [RFC][PATCH] bug: Add _ONCE logic to report_bug()

* Peter Zijlstra <peterz@infradead.org> wrote:

> 
> Josh suggested moving the _ONCE logic inside the trap handler, using a
> bit in the bug_entry::flags field, avoiding the need for the extra
> variable.

This looks interesting, as the _ONCE() methods of warning are far more 
user-friendly than WARN() spam.

> Sadly this only works for WARN_ON_ONCE(), since the others have
> printk() statements prior to triggering the trap.

Which one is problematic to convert, WARN_ONCE()?

> Still, this saves some text and data:
> 
>   text            data     bss    dec              hex    filename
>   10469505        4443448  843776 15756729         f06db9 defconfig-build/vmlinux-ud0
>   10452803        4442616  843776 15739195         f0293b defconfig-build/vmlinux-ud0-once
> 
> (Only compile tested on x86_64 so far.)

That looks pretty sweet, as various almost never triggered _ONCE() checks tend to 
disturb the generated machine code quite a bit ...

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1588120 — Re: [RFC][PATCH] bug: Add _ONCE logic to report_bug()

On Sat, Feb 25, 2017 at 10:18:23AM +0100, Ingo Molnar wrote:

> > Sadly this only works for WARN_ON_ONCE(), since the others have
> > printk() statements prior to triggering the trap.
> 
> Which one is problematic to convert, WARN_ONCE()?

Yes, WARN_ONCE(), all the ones that have printf fmt crud in.

If we want report_bug() to do the _ONCE thing, we also need that to do
the printk().

I tried the below hackery (beware eye and brain damage ahead) which
actually compiles, but generates horrific junk -- far larger than
without.

The __builtin_va_*() crud was really not meant for things like this.

---
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -66,6 +66,11 @@ do {								\
 
 #define __WARN_FLAGS(flags)	_BUG_FLAGS(ASM_UD0, BUGFLAG_WARNING|(flags))
 
+#define __WARN_ARGS_FLAGS(args, flags) do {		\
+	asm volatile ("" : : "a" (args));		\
+	_BUG_FLAGS(ASM_UD0, BUGFLAG_WARNING|(flags));	\
+} while (0)
+
 #include <asm-generic/bug.h>
 
 #endif /* _ASM_X86_BUG_H */
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -7,6 +7,7 @@
 #define BUGFLAG_WARNING		(1 << 0)
 #define BUGFLAG_ONCE		(1 << 1)
 #define BUGFLAG_DONE		(1 << 2)
+#define BUGFLAG_ARGS		(1 << 3)
 #define BUGFLAG_TAINT(taint)	(BUGFLAG_WARNING | ((taint) << 8))
 #define BUG_GET_TAINT(bug)	((bug)->flags >> 8)
 #endif
@@ -67,8 +68,28 @@ struct bug_entry {
 		__WARN_ONCE_TAINT(TAINT_WARN);			\
 	unlikely(__ret_warn_on);				\
 })
+
+#ifdef __WARN_ARGS_FLAGS
+static inline void __va_hack(va_list *ap, ...)
+{
+	va_start(*ap, ap);
+}
+
+#define __WARN_printf(arg...) do {					\
+	va_list __ap;							\
+	__va_hack(&__ap, arg);						\
+	__WARN_ARGS_FLAGS(&__ap, BUGFLAG_ARGS|BUGFLAG_TAINT(TAINT_WARN));	\
+} while (0)
+
+#define __WARN_printf_taint(taint, arg...) do {				\
+	va_list __ap;							\
+	__va_hack(&__ap, arg);						\
+	__WARN_ARGS_FLAGS(&__ap, BUGFLAG_ARGS|BUGFLAG_TAINT(taint));	\
+} while (0)
 #endif
 
+#endif /* __WARN_FLAGS */
+
 /*
  * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
  * significant issues that need prompt attention if they should ever
@@ -90,10 +111,12 @@ extern void warn_slowpath_null(const cha
 	warn_slowpath_fmt_taint(__FILE__, __LINE__, taint, arg)
 #else
 #define __WARN()		__WARN_TAINT(TAINT_WARN)
+#ifndef __WARN_ARGS_FLAGS
 #define __WARN_printf(arg...)	do { printk(arg); __WARN(); } while (0)
 #define __WARN_printf_taint(taint, arg...)				\
 	do { printk(arg); __WARN_TAINT(taint); } while (0)
 #endif
+#endif
 
 /* used internally by panic.c */
 struct warn_args;
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -143,7 +143,8 @@ enum bug_trap_type report_bug(unsigned l
 {
 	struct bug_entry *bug;
 	const char *file;
-	unsigned line, warning, once, done;
+	unsigned line, warning, once, done, args;
+	va_list *ap;
 
 	if (!is_valid_bugaddr(bugaddr))
 		return BUG_TRAP_TYPE_NONE;
@@ -166,6 +167,7 @@ enum bug_trap_type report_bug(unsigned l
 		warning = (bug->flags & BUGFLAG_WARNING) != 0;
 		once = (bug->flags & BUGFLAG_ONCE) != 0;
 		done = (bug->flags & BUGFLAG_DONE) != 0;
+		args = (bug->flags & BUGFLAG_ARGS) != 0;
 
 		if (warning && once) {
 			if (done)
@@ -176,9 +178,16 @@ enum bug_trap_type report_bug(unsigned l
 			 */
 			bug->flags |= BUGFLAG_DONE;
 		}
+
+		if (args)
+			ap = (va_list *)regs->ax;
 	}
 
 	if (warning) {
+		if (args) {
+			const char *fmt = va_arg(*ap, const char *);
+			vprintk(fmt, *ap);
+		}
 		/* this is a WARN_ON rather than BUG/BUG_ON */
 		__warn(file, line, (void *)bugaddr, BUG_GET_TAINT(bug), regs,
 		       NULL);

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web