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

Groups > linux.kernel > #1586880 > unrolled thread

[PATCH] x86: Implement __WARN using UD0

Started byPeter Zijlstra <peterz@infradead.org>
First post2017-02-23 14:30 +0100
Last post2017-02-25 11:50 +0100
Articles 20 on this page of 29 — 7 participants

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 1 of 2  [1] 2  Next page →

#1586880 — [PATCH] x86: Implement __WARN using UD0

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 14:30 +0100
Subject[PATCH] x86: Implement __WARN using UD0
Message-ID<te1D4-1LK-9@gated-at.bofh.it>
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 instruction
stream we'll loose because of the __bug_table entries. Still, saves on
I$ footprint and the total image size does go down a bit.

  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

(um didn't seem to use GENERIC_BUG at all, so remove it)

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Richard Weinberger <richard.weinberger@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/um/Kconfig.common         |    5 ---
 arch/x86/include/asm/bug.h     |   62 +++++++++++++++++++++++++++++++++--------
 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, 95 insertions(+), 70 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,74 @@
 #ifndef _ASM_X86_BUG_H
 #define _ASM_X86_BUG_H
 
+#include <linux/stringify.h>
+
 #define HAVE_ARCH_BUG
 
-#ifdef CONFIG_DEBUG_BUGVERBOSE
+/*
+ * 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
+ *
+ * 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)	"\t.long " __stringify(val) "\n"
 #else
-# define __BUG_C0	"2:\t.long 1b - 2b, %c0 - 2b\n"
+# define __BUG_REL(val)	"\t.long " __stringify(val) " - 2b\n"
 #endif
 
-#define BUG()							\
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+
+#define _BUG_FLAGS(ins, flags)					\
 do {								\
-	asm volatile("1:\tud2\n"				\
+	asm volatile("1:\t" ins "\n"				\
 		     ".pushsection __bug_table,\"a\"\n"		\
-		     __BUG_C0					\
-		     "\t.word %c1, 0\n"				\
-		     "\t.org 2b+%c2\n"				\
+		     "2:" __BUG_REL(1b)				\
+			  __BUG_REL(%c0)			\
+		     "\t.word %c1, %c2\n"			\
+		     "\t.org 2b+%c3\n"				\
 		     ".popsection"				\
 		     : : "i" (__FILE__), "i" (__LINE__),	\
-		     "i" (sizeof(struct bug_entry)));		\
-	unreachable();						\
+			 "i" (flags),				\
+			 "i" (sizeof(struct bug_entry)));	\
 } while (0)
 
 #else
+
+#define _BUG_FLAGS(ins, flags)					\
+do {								\
+	asm volatile("1:\t" ins "\n"				\
+		     ".pushsection __bug_table,\"a\"\n"		\
+		     "2:" __BUG_REL(1b)				\
+		     "\t.word %c0\n"				\
+		     "\t.org 2b+%c1\n"				\
+		     ".popsection"				\
+		     : : "i" (flags),				\
+			 "i" (sizeof(struct bug_entry)));	\
+} while (0)
+
+#endif
+
 #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 (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] | [next] | [standalone]

#1586908

FromArjan van de Ven <arjan@linux.intel.com>
Date2017-02-23 15:20 +0100
Message-ID<te2pr-2oI-1@gated-at.bofh.it>
In reply to#1586880
On 2/23/2017 5:28 AM, Peter Zijlstra wrote:
>
> 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 instruction
> stream we'll loose because of the __bug_table entries. Still, saves on
> I$ footprint and the total image size does go down a bit.

well I am a little sceptical; WARNs are rare so the code (other than the test)
should be waaay out of line already (unlikely() and co).
And I assume you're not removing the __FILE__ and __LINE__ info, since that info
is actually high value for us developers... so what are you actually saving?

(icache saving is only real if the line that the cold code lives on would actually
end up in icache for other reasons; I would hope the compiler puts the out
of line code WAY out of line)

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

#1586934

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 16:00 +0100
Message-ID<te329-2EY-13@gated-at.bofh.it>
In reply to#1586908
On Thu, Feb 23, 2017 at 06:14:45AM -0800, Arjan van de Ven wrote:
> On 2/23/2017 5:28 AM, Peter Zijlstra wrote:
> >
> >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 instruction
> >stream we'll loose because of the __bug_table entries. Still, saves on
> >I$ footprint and the total image size does go down a bit.
> 
> well I am a little sceptical; WARNs are rare so the code (other than the test)
> should be waaay out of line already (unlikely() and co).

There's only so much you can do in small functions. Sure it tries to
move the crud to the end, but at the end of the day, its still in the
same function.

> And I assume you're not removing the __FILE__ and __LINE__ info, since that info
> is actually high value for us developers... so what are you actually saving?

OK, so going by my own numbers the total image size does not in fact go
down (it did earlier when I initially wrote this patch) :/

I think back when I wrote this I had refcount_t generate inline WARNs
and that generates a huge amount of junk all over the place. This very
much did clear much of that up.

And as said; there's only so much you can do in small functions.

Look at the below for example (frobbed the code to do WARN_ON instead of
WARN), depending on alignment the WARN code will be in the same
cacheline as 'normal' code.


0000000000000016 <refcount_add_not_zero>:
  16:   55                      push   %rbp
  17:   8b 16                   mov    (%rsi),%edx
  19:   41 83 c8 ff             or     $0xffffffff,%r8d
  1d:   48 89 e5                mov    %rsp,%rbp
  20:   85 d2                   test   %edx,%edx
  22:   74 25                   je     49 <refcount_add_not_zero+0x33>
  24:   83 fa ff                cmp    $0xffffffff,%edx
  27:   74 1c                   je     45 <refcount_add_not_zero+0x2f>
  29:   89 d1                   mov    %edx,%ecx
  2b:   89 d0                   mov    %edx,%eax
  2d:   01 f9                   add    %edi,%ecx
  2f:   41 0f 42 c8             cmovb  %r8d,%ecx
  33:   f0 0f b1 0e             lock cmpxchg %ecx,(%rsi)
  37:   39 c2                   cmp    %eax,%edx
  39:   74 04                   je     3f <refcount_add_not_zero+0x29>
  3b:   89 c2                   mov    %eax,%edx
  3d:   eb e1                   jmp    20 <refcount_add_not_zero+0xa>
  3f:   ff c1                   inc    %ecx
  41:   75 02                   jne    45 <refcount_add_not_zero+0x2f>
  43:   0f ff                   (bad)  
  45:   b0 01                   mov    $0x1,%al
  47:   eb 02                   jmp    4b <refcount_add_not_zero+0x35>
  49:   31 c0                   xor    %eax,%eax
  4b:   5d                      pop    %rbp
  4c:   c3                      retq  



0000000000000016 <refcount_add_not_zero>:
  16:   8b 16                   mov    (%rsi),%edx
  18:   41 83 c8 ff             or     $0xffffffff,%r8d
  1c:   85 d2                   test   %edx,%edx
  1e:   74 3b                   je     5b <refcount_add_not_zero+0x45>
  20:   83 fa ff                cmp    $0xffffffff,%edx
  23:   75 03                   jne    28 <refcount_add_not_zero+0x12>
  25:   b0 01                   mov    $0x1,%al
  27:   c3                      retq   
  28:   89 d1                   mov    %edx,%ecx
  2a:   89 d0                   mov    %edx,%eax
  2c:   01 f9                   add    %edi,%ecx
  2e:   41 0f 42 c8             cmovb  %r8d,%ecx
  32:   f0 0f b1 0e             lock cmpxchg %ecx,(%rsi)
  36:   39 c2                   cmp    %eax,%edx
  38:   74 04                   je     3e <refcount_add_not_zero+0x28>
  3a:   89 c2                   mov    %eax,%edx
  3c:   eb de                   jmp    1c <refcount_add_not_zero+0x6>
  3e:   ff c1                   inc    %ecx
  40:   75 e3                   jne    25 <refcount_add_not_zero+0xf>
  42:   55                      push   %rbp
  43:   be 3f 00 00 00          mov    $0x3f,%esi
  48:   48 c7 c7 00 00 00 00    mov    $0x0,%rdi
                        4b: R_X86_64_32S        .rodata.str1.1
  4f:   48 89 e5                mov    %rsp,%rbp
  52:   e8 00 00 00 00          callq  57 <refcount_add_not_zero+0x41>
                        53: R_X86_64_PC32       warn_slowpath_null-0x4
  57:   b0 01                   mov    $0x1,%al
  59:   5d                      pop    %rbp
  5a:   c3                      retq   
  5b:   31 c0                   xor    %eax,%eax
  5d:   c3                      retq

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

#1587329

FromIngo Molnar <mingo@kernel.org>
Date2017-02-24 09:00 +0100
Message-ID<teiXg-5sB-5@gated-at.bofh.it>
In reply to#1586934
* Peter Zijlstra <peterz@infradead.org> wrote:

> 0000000000000016 <refcount_add_not_zero>:
>   16:   55                      push   %rbp
>   17:   8b 16                   mov    (%rsi),%edx
>   19:   41 83 c8 ff             or     $0xffffffff,%r8d
>   1d:   48 89 e5                mov    %rsp,%rbp
>   20:   85 d2                   test   %edx,%edx
>   22:   74 25                   je     49 <refcount_add_not_zero+0x33>
>   24:   83 fa ff                cmp    $0xffffffff,%edx
>   27:   74 1c                   je     45 <refcount_add_not_zero+0x2f>
>   29:   89 d1                   mov    %edx,%ecx
>   2b:   89 d0                   mov    %edx,%eax
>   2d:   01 f9                   add    %edi,%ecx
>   2f:   41 0f 42 c8             cmovb  %r8d,%ecx
>   33:   f0 0f b1 0e             lock cmpxchg %ecx,(%rsi)
>   37:   39 c2                   cmp    %eax,%edx
>   39:   74 04                   je     3f <refcount_add_not_zero+0x29>
>   3b:   89 c2                   mov    %eax,%edx
>   3d:   eb e1                   jmp    20 <refcount_add_not_zero+0xa>
>   3f:   ff c1                   inc    %ecx
>   41:   75 02                   jne    45 <refcount_add_not_zero+0x2f>
>   43:   0f ff                   (bad)  
>   45:   b0 01                   mov    $0x1,%al
>   47:   eb 02                   jmp    4b <refcount_add_not_zero+0x35>
>   49:   31 c0                   xor    %eax,%eax
>   4b:   5d                      pop    %rbp
>   4c:   c3                      retq  

BTW., one thing that is probably not represented fairly by this example is the 
better, much lower register clobbering impact of trap-driven WARN_ON() versus 
function call driven WARN_ON(): the trap will preserve all registers, while a call 
driven slow path will clobber all the caller-save registers.

In this example this does not show up much because the WARN_ON() is done at the 
end of the function. In function where WARN()s are emitted earlier the size of the 
slow path should be even better.

In any case, I like your patch, I believe what counts is the .text size reduction:

  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

Put differently: for essentially zero RAM and runtime cost we've bought a 0.2% 
larger (instruction) cache (!!). That's quite significant, IMHO, as lots of kernel 
intense workloads involve many small functions.

The only high level question is whether we trust the trap machinery to generate 
WARN_ON()s. I believe we do.

BTW.: why not use INT3 instead of all these weird #UD opcodes? It's a single byte 
opcode and we can do a quick exception table search in do_debug(). This way we'll 
also have irqs disabled which might help getting the message out before any irq 
handler comes in and muddies the waters.

In a sense WARN_ON()s and BUG_ON()s can be considered permanently installed 
in-line kprobes, with a special, built-in handler.

BTW. #2: side note, GCC generated crap code here. Why didn't it do:

>   3f:   ff c1                   inc    %ecx
>   41:   75 02                   jne    55 <refcount_add_not_zero+0x2f>
>   45:   b0 01                   mov    $0x1,%al
>   4b:   5d                      pop    %rbp
>   4c:   c3                      retq  
>
>   49:   31 c0                   xor    %eax,%eax
>   4b:   5d                      pop    %rbp
>   4c:   c3                      retq  
>
>   55:   0f ff                   (bad)  

?

It's hand edited so the offsets are wrong, but the idea is that for the same 14 
bytes we get a straight fall-through fast path to the RETQ and one JMP less 
executed in the 'return 1' case. Both the 'return 0' case and the #UD are in tail 
part of the function.

Thanks,

	Ingo

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

#1587358

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-24 09:40 +0100
Message-ID<tejzY-5Xe-9@gated-at.bofh.it>
In reply to#1587329
On Fri, Feb 24, 2017 at 08:43:25AM +0100, Ingo Molnar wrote:
> The only high level question is whether we trust the trap machinery to generate 
> WARN_ON()s. I believe we do.
> 
> BTW.: why not use INT3 instead of all these weird #UD opcodes? It's a single byte 
> opcode and we can do a quick exception table search in do_debug(). This way we'll 
> also have irqs disabled which might help getting the message out before any irq 
> handler comes in and muddies the waters.
> 
> In a sense WARN_ON()s and BUG_ON()s can be considered permanently installed 
> in-line kprobes, with a special, built-in handler.

I've actually been looking into that. There's a bunch of 'fun' details
that I've been checking, but I think I can make that happen.

My initial patch extended the existing UD2 BUG trap to include the WARN,
this is what many other architectures already do. Arjan then complained
that some emulators terminate on UD2 and could I please not use that for
WARN, at which point Borislav called my attention to UD0/UD1.

So I made the UD0 change and posted (fwiw, there's a lost refresh in the
patch I posted and it will not actually work).

I think I'll post an update of said patch and then attempt to do the
INT3 thing in a later patch -- that will require at least one new knob
in the generic BUG code ...

> BTW. #2: side note, GCC generated crap code here. Why didn't it do:

I've seen GCC do 'wonderful' things the past few weeks. Absolutely mind
boggling stuff.

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

#1587436

FromIngo Molnar <mingo@kernel.org>
Date2017-02-24 10:20 +0100
Message-ID<tekcF-6wz-7@gated-at.bofh.it>
In reply to#1587358
* hpa@zytor.com <hpa@zytor.com> wrote:

> Incidentally, as an alternative to a #UD, int $9 could be an alternative 
> (exception vector 9 was discontinued with the 486.)

With thousands of call sites a one byte opcode would be preferred, that's why INT3 
is so nice.

Thanks,

	Ingo

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

#1587467

FromBorislav Petkov <bp@alien8.de>
Date2017-02-24 10:50 +0100
Message-ID<tekFI-6GV-9@gated-at.bofh.it>
In reply to#1587436
On Fri, Feb 24, 2017 at 10:15:37AM +0100, Ingo Molnar wrote:
> With thousands of call sites a one byte opcode would be preferred, that's why INT3 
> is so nice.

ICEBP is also a single-byte: F1. And should be pretty unused :)

-- 
Regards/Gruss,
    Boris.

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

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

#1587535

From"H. Peter Anvin" <hpa@zytor.com>
Date2017-02-24 12:50 +0100
Message-ID<temxQ-7YF-11@gated-at.bofh.it>
In reply to#1587467
On 02/24/17 01:46, Borislav Petkov wrote:
> On Fri, Feb 24, 2017 at 10:15:37AM +0100, Ingo Molnar wrote:
>> With thousands of call sites a one byte opcode would be preferred, that's why INT3 
>> is so nice.
> 
> ICEBP is also a single-byte: F1. And should be pretty unused :)
> 

That would make debugging preproduction hardware on Linux a living
nightmare.

	-hpa

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

#1587539

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-24 12:50 +0100
Message-ID<temxQ-7YF-25@gated-at.bofh.it>
In reply to#1587436
On Fri, Feb 24, 2017 at 01:53:27AM -0800, H. Peter Anvin wrote:
> On 02/24/17 01:15, Ingo Molnar wrote:
> > 
> > * hpa@zytor.com <hpa@zytor.com> wrote:
> > 
> >> Incidentally, as an alternative to a #UD, int $9 could be an alternative 
> >> (exception vector 9 was discontinued with the 486.)
> > 
> > With thousands of call sites a one byte opcode would be preferred, that's why INT3 
> > is so nice.
> > 
> 
> Yes, but using INT3 would bugger up debuggers.

So I've been looking into this, in general debuggers will refuse to poke
INT3 on top of INT3 (sane and obvious behaviour).

Also, we'd run any actual handlers before fixup_bug(), the only thing
here is that we need to also do fixup_bug() if the instruction pointer
isn't changed. This would allow debuggers to effectively stack on top
of the already present breakpoint.

Optimized kprobes will make sure to not clobber INT3 (although it will
happily clobber anything else from __ex_table[]).

The only other point is that currently report_bug() relies on
is_valid_bugaddr() to tell if the instruction is 'good', but then
reports a BUG if there is no entry in __bug_table[]. We'd have to change
that to be strict and assume __bug_table[] is complete, and report
BUG_TRAP_TYPE_NONE when there is no __bug_table[] entry, this would also
avoid the need for is_valid_bugaddr().

So yes, its tricky but it could be done. A new single byte #UD
instruction would be much nicer though.

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

#1588116

FromBorislav Petkov <bp@alien8.de>
Date2017-02-25 11:50 +0100
Message-ID<teI5k-6sz-13@gated-at.bofh.it>
In reply to#1587539
On Fri, Feb 24, 2017 at 11:41:33AM +0100, Peter Zijlstra wrote:
> So yes, its tricky but it could be done. A new single byte #UD
> instruction would be much nicer though.

Btw, if we did a new insn which means new functionality instead of
"stealing" an invalid one, we would have to have a fallback for all
those current CPUs which don't support it, which means, alternatives
patching.

Perhaps it would be better to take one of the invalid ones and future
hw can then extend it and actually make it into a special OS-INT
instruction which is small enough to be inline and can, if hit, run a
handler where you do fixup.

And then that insn could even have a immed8 arg which you can use to
pass info from the call site. IOW, something like

	...
	OSINT $12
	...

and handler inspects opcode and does things based on it...

Oh well.

-- 
Regards/Gruss,
    Boris.

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

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

#1588181

Fromhpa@zytor.com
Date2017-02-25 19:00 +0100
Message-ID<teONr-2VX-1@gated-at.bofh.it>
In reply to#1588116
On February 25, 2017 2:38:08 AM PST, Borislav Petkov <bp@alien8.de> wrote:
>On Fri, Feb 24, 2017 at 11:41:33AM +0100, Peter Zijlstra wrote:
>> So yes, its tricky but it could be done. A new single byte #UD
>> instruction would be much nicer though.
>
>Btw, if we did a new insn which means new functionality instead of
>"stealing" an invalid one, we would have to have a fallback for all
>those current CPUs which don't support it, which means, alternatives
>patching.
>
>Perhaps it would be better to take one of the invalid ones and future
>hw can then extend it and actually make it into a special OS-INT
>instruction which is small enough to be inline and can, if hit, run a
>handler where you do fixup.
>
>And then that insn could even have a immed8 arg which you can use to
>pass info from the call site. IOW, something like
>
>	...
>	OSINT $12
>	...
>
>and handler inspects opcode and does things based on it...
>
>Oh well.

You mean like the INT instruction?
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

#1588207

FromBorislav Petkov <bp@alien8.de>
Date2017-02-25 20:40 +0100
Message-ID<teQmd-42X-15@gated-at.bofh.it>
In reply to#1588181
On Sat, Feb 25, 2017 at 09:55:45AM -0800, hpa@zytor.com wrote:
> You mean like the INT instruction?

Right, you mentioned reusing INT $9 upthread.

That doesn't have the additional info in the immed8 - it is the vector
in this case. But that's not really important for our usage.

In any case, the hw does react to it when I do

	"int $9"

so I guess we could look into using that one.

[   93.668930] test: loading out-of-tree module taints kernel.
[   93.674785] Starting test.
[   93.677571] coprocessor segment overrun: 0000 [#1] PREEMPT SMP
[   93.683459] Modules linked in: test(O+) xfs libcrc32c exportfs snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_coded
[   93.683489] CPU: 4 PID: 2136 Comm: insmod Tainted: G           O    4.10.0-rc7+ #7
[   93.683500] Hardware name: MICRO-STAR INTERNATIONAL CO.,LTD MS-7599/870-C45 (MS-7599), BIOS V1.15 03/04/2011
[   93.683503] task: ffff88012f154380 task.stack: ffffc90006cf4000
[   93.683512] RIP: 0010:test_init+0x17/0x20 [test]
[   93.683515] RSP: 0018:ffffc90006cf7cb8 EFLAGS: 00000296
[   93.683518] RAX: 000000000000000e RBX: ffffffffa03ca0c0 RCX: 0000000000000000
[   93.683521] RDX: 000000000000000e RSI: ffffffff802d4e48 RDI: ffffffff802d4e48
[   93.683523] RBP: ffffc90006cf7cb8 R08: 0000000000000000 R09: 0000000000000274
[   93.683525] R10: 0000000000000000 R11: 0000000000000273 R12: ffffffffa03ca000
[   93.683527] R13: 0000000000000000 R14: 0000000000000001 R15: ffffc90006cf7eb0
[   93.683530] FS:  00007f55d1e3e700(0000) GS:ffff880136d00000(0000) knlGS:0000000000000000
[   93.683533] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   93.683535] CR2: 000055be657f51e8 CR3: 0000000131014000 CR4: 00000000000006e0
[   93.683537] Call Trace:
[   93.683548]  do_one_initcall+0x53/0x1a0
[   93.683554]  ? __vunmap+0xaa/0xf0
[   93.683559]  ? kfree+0x151/0x1b0
[   93.683566]  do_init_module+0x5f/0x1d5
[   93.683573]  load_module+0x2026/0x2530
[   93.683578]  ? __symbol_put+0x80/0x80
[   93.683589]  SYSC_finit_module+0xcb/0xd0
[   93.683597]  SyS_finit_module+0xe/0x10
[   93.683602]  entry_SYSCALL_64_fastpath+0x18/0xa8
[   93.683605] RIP: 0033:0x7f55d1970a59
[   93.683607] RSP: 002b:00007ffc3cf781d8 EFLAGS: 00000206 ORIG_RAX: 0000000000000139
[   93.683610] RAX: ffffffffffffffda RBX: 00007f55d1c2ab78 RCX: 00007f55d1970a59
[   93.683612] RDX: 0000000000000000 RSI: 000055be655d73d9 RDI: 0000000000000003
[   93.683614] RBP: 000000000000270f R08: 0000000000000000 R09: 00007f55d1c2cf00
[   93.683616] R10: 0000000000000003 R11: 0000000000000206 R12: 00007f55d1c2ab78
[   93.683618] R13: 0000000000001020 R14: 00007f55d1c2ab78 R15: 00007f55d1c2ab20
[   93.683623] Code: <31> c0 5d c3 0f 1f 44 00 00 0f 1f 44 00 00 55 48 c7 c7 6e a0 3c a0 
[   93.683655] RIP: test_init+0x17/0x20 [test] RSP: ffffc90006cf7cb8
[   93.683699] ---[ end trace e034b65a8bb8cf26 ]---
[   93.683702] Kernel panic - not syncing: Fatal exception
[   93.709252] Kernel Offset: disabled

-- 
Regards/Gruss,
    Boris.

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

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

#1588219

FromBorislav Petkov <bp@alien8.de>
Date2017-02-25 21:40 +0100
Message-ID<teRih-4H9-11@gated-at.bofh.it>
In reply to#1588207
On Sat, Feb 25, 2017 at 12:04:18PM -0800, hpa@zytor.com wrote:
> Note that once you have a trap you can create an immediate yourself,
> the CPU doesn't need to do it for you, unless you really care about
> latency (reading the instruction steam can be kind of expensive,
> although it is quite a bit simpler if we know we come from kernel
> space.)

You mean, put it after the INT 9 opcode?

CD 09 immed8

Yeah, that could be one way to pass callsite-specific info, if needed.

-- 
Regards/Gruss,
    Boris.

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

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

#1588222

Fromhpa@zytor.com
Date2017-02-25 21:50 +0100
Message-ID<teRih-4H9-13@gated-at.bofh.it>
In reply to#1588207
On February 25, 2017 11:38:44 AM PST, Borislav Petkov <bp@alien8.de> wrote:
>On Sat, Feb 25, 2017 at 09:55:45AM -0800, hpa@zytor.com wrote:
>> You mean like the INT instruction?
>
>Right, you mentioned reusing INT $9 upthread.
>
>That doesn't have the additional info in the immed8 - it is the vector
>in this case. But that's not really important for our usage.
>
>In any case, the hw does react to it when I do
>
>	"int $9"
>
>so I guess we could look into using that one.
>
>[   93.668930] test: loading out-of-tree module taints kernel.
>[   93.674785] Starting test.
>[   93.677571] coprocessor segment overrun: 0000 [#1] PREEMPT SMP
>[   93.683459] Modules linked in: test(O+) xfs libcrc32c exportfs
>snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_coded
>[   93.683489] CPU: 4 PID: 2136 Comm: insmod Tainted: G           O   
>4.10.0-rc7+ #7
>[   93.683500] Hardware name: MICRO-STAR INTERNATIONAL CO.,LTD
>MS-7599/870-C45 (MS-7599), BIOS V1.15 03/04/2011
>[   93.683503] task: ffff88012f154380 task.stack: ffffc90006cf4000
>[   93.683512] RIP: 0010:test_init+0x17/0x20 [test]
>[   93.683515] RSP: 0018:ffffc90006cf7cb8 EFLAGS: 00000296
>[   93.683518] RAX: 000000000000000e RBX: ffffffffa03ca0c0 RCX:
>0000000000000000
>[   93.683521] RDX: 000000000000000e RSI: ffffffff802d4e48 RDI:
>ffffffff802d4e48
>[   93.683523] RBP: ffffc90006cf7cb8 R08: 0000000000000000 R09:
>0000000000000274
>[   93.683525] R10: 0000000000000000 R11: 0000000000000273 R12:
>ffffffffa03ca000
>[   93.683527] R13: 0000000000000000 R14: 0000000000000001 R15:
>ffffc90006cf7eb0
>[   93.683530] FS:  00007f55d1e3e700(0000) GS:ffff880136d00000(0000)
>knlGS:0000000000000000
>[   93.683533] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>[   93.683535] CR2: 000055be657f51e8 CR3: 0000000131014000 CR4:
>00000000000006e0
>[   93.683537] Call Trace:
>[   93.683548]  do_one_initcall+0x53/0x1a0
>[   93.683554]  ? __vunmap+0xaa/0xf0
>[   93.683559]  ? kfree+0x151/0x1b0
>[   93.683566]  do_init_module+0x5f/0x1d5
>[   93.683573]  load_module+0x2026/0x2530
>[   93.683578]  ? __symbol_put+0x80/0x80
>[   93.683589]  SYSC_finit_module+0xcb/0xd0
>[   93.683597]  SyS_finit_module+0xe/0x10
>[   93.683602]  entry_SYSCALL_64_fastpath+0x18/0xa8
>[   93.683605] RIP: 0033:0x7f55d1970a59
>[   93.683607] RSP: 002b:00007ffc3cf781d8 EFLAGS: 00000206 ORIG_RAX:
>0000000000000139
>[   93.683610] RAX: ffffffffffffffda RBX: 00007f55d1c2ab78 RCX:
>00007f55d1970a59
>[   93.683612] RDX: 0000000000000000 RSI: 000055be655d73d9 RDI:
>0000000000000003
>[   93.683614] RBP: 000000000000270f R08: 0000000000000000 R09:
>00007f55d1c2cf00
>[   93.683616] R10: 0000000000000003 R11: 0000000000000206 R12:
>00007f55d1c2ab78
>[   93.683618] R13: 0000000000001020 R14: 00007f55d1c2ab78 R15:
>00007f55d1c2ab20
>[   93.683623] Code: <31> c0 5d c3 0f 1f 44 00 00 0f 1f 44 00 00 55 48
>c7 c7 6e a0 3c a0 
>[   93.683655] RIP: test_init+0x17/0x20 [test] RSP: ffffc90006cf7cb8
>[   93.683699] ---[ end trace e034b65a8bb8cf26 ]---
>[   93.683702] Kernel panic - not syncing: Fatal exception
>[   93.709252] Kernel Offset: disabled

Note that once you have a trap you can create an immediate yourself, the CPU doesn't need to do it for you, unless you really care about latency (reading the instruction steam can be kind of expensive, although it is quite a bit simpler if we know we come from kernel space.)
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

#1587439

Fromhpa@zytor.com
Date2017-02-24 10:20 +0100
Message-ID<tekcF-6wz-9@gated-at.bofh.it>
In reply to#1587358
On February 24, 2017 12:31:15 AM PST, Peter Zijlstra <peterz@infradead.org> wrote:
>On Fri, Feb 24, 2017 at 08:43:25AM +0100, Ingo Molnar wrote:
>> The only high level question is whether we trust the trap machinery
>to generate 
>> WARN_ON()s. I believe we do.
>> 
>> BTW.: why not use INT3 instead of all these weird #UD opcodes? It's a
>single byte 
>> opcode and we can do a quick exception table search in do_debug().
>This way we'll 
>> also have irqs disabled which might help getting the message out
>before any irq 
>> handler comes in and muddies the waters.
>> 
>> In a sense WARN_ON()s and BUG_ON()s can be considered permanently
>installed 
>> in-line kprobes, with a special, built-in handler.
>
>I've actually been looking into that. There's a bunch of 'fun' details
>that I've been checking, but I think I can make that happen.
>
>My initial patch extended the existing UD2 BUG trap to include the
>WARN,
>this is what many other architectures already do. Arjan then complained
>that some emulators terminate on UD2 and could I please not use that
>for
>WARN, at which point Borislav called my attention to UD0/UD1.
>
>So I made the UD0 change and posted (fwiw, there's a lost refresh in
>the
>patch I posted and it will not actually work).
>
>I think I'll post an update of said patch and then attempt to do the
>INT3 thing in a later patch -- that will require at least one new knob
>in the generic BUG code ...
>
>> BTW. #2: side note, GCC generated crap code here. Why didn't it do:
>
>I've seen GCC do 'wonderful' things the past few weeks. Absolutely mind
>boggling stuff.

Incidentally, as an alternative to a #UD, int $9 could be an alternative (exception vector 9 was discontinued with the 486.)
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

#1587474

Fromhpa@zytor.com
Date2017-02-24 10:50 +0100
Message-ID<tekFJ-6GV-31@gated-at.bofh.it>
In reply to#1587358
On February 24, 2017 12:31:15 AM PST, Peter Zijlstra <peterz@infradead.org> wrote:
>On Fri, Feb 24, 2017 at 08:43:25AM +0100, Ingo Molnar wrote:
>> The only high level question is whether we trust the trap machinery
>to generate 
>> WARN_ON()s. I believe we do.
>> 
>> BTW.: why not use INT3 instead of all these weird #UD opcodes? It's a
>single byte 
>> opcode and we can do a quick exception table search in do_debug().
>This way we'll 
>> also have irqs disabled which might help getting the message out
>before any irq 
>> handler comes in and muddies the waters.
>> 
>> In a sense WARN_ON()s and BUG_ON()s can be considered permanently
>installed 
>> in-line kprobes, with a special, built-in handler.
>
>I've actually been looking into that. There's a bunch of 'fun' details
>that I've been checking, but I think I can make that happen.
>
>My initial patch extended the existing UD2 BUG trap to include the
>WARN,
>this is what many other architectures already do. Arjan then complained
>that some emulators terminate on UD2 and could I please not use that
>for
>WARN, at which point Borislav called my attention to UD0/UD1.
>
>So I made the UD0 change and posted (fwiw, there's a lost refresh in
>the
>patch I posted and it will not actually work).
>
>I think I'll post an update of said patch and then attempt to do the
>INT3 thing in a later patch -- that will require at least one new knob
>in the generic BUG code ...
>
>> BTW. #2: side note, GCC generated crap code here. Why didn't it do:
>
>I've seen GCC do 'wonderful' things the past few weeks. Absolutely mind
>boggling stuff.

Terminating on UD2 is really obnoxious, btw. JMPE, ICEBP/INT1, some I/O port or MSR would be appropriate choices for that, but UD2 is rude in the extreme.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

#1586910

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 15:20 +0100
Message-ID<te2pr-2oI-5@gated-at.bofh.it>
In reply to#1586880
On Thu, Feb 23, 2017 at 02:28:13PM +0100, Peter Zijlstra wrote:
> + * 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"

http://repo.or.cz/nasm.git/blob/HEAD:/x86/insns.dat

has:

1378 UD0             void                            [       0f ff]                                  186,UNDOC
1379 UD1             void                            [       0f b9]                                  186,UNDOC
1380 UD2B            void                            [       0f b9]                                  186,UNDOC,ND
1381 UD2             void                            [       0f 0b]                                  186
1382 UD2A            void                            [       0f 0b]                                  186,ND

which seems to use the 2 byte version of UD1.

hpa, any input?

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

#1586935

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 16:00 +0100
Message-ID<te329-2EY-15@gated-at.bofh.it>
In reply to#1586910
On Thu, Feb 23, 2017 at 03:09:29PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 23, 2017 at 02:28:13PM +0100, Peter Zijlstra wrote:
> > + * 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"
> 
> http://repo.or.cz/nasm.git/blob/HEAD:/x86/insns.dat
> 
> has:
> 
> 1378 UD0             void                            [       0f ff]                                  186,UNDOC
> 1379 UD1             void                            [       0f b9]                                  186,UNDOC
> 1380 UD2B            void                            [       0f b9]                                  186,UNDOC,ND
> 1381 UD2             void                            [       0f 0b]                                  186
> 1382 UD2A            void                            [       0f 0b]                                  186,ND
> 
> which seems to use the 2 byte version of UD1.
> 
> hpa, any input?

N/m, as Josh said, they're listed in the latest SDM (Dec 2016) and that
lists UD1 /r, so 3 bytes.

This means binutils decodes it wrong.

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

#1586950

Fromhpa@zytor.com
Date2017-02-23 16:20 +0100
Message-ID<te3lw-323-19@gated-at.bofh.it>
In reply to#1586910
On February 23, 2017 6:09:29 AM PST, Peter Zijlstra <peterz@infradead.org> wrote:
>On Thu, Feb 23, 2017 at 02:28:13PM +0100, Peter Zijlstra wrote:
>> + * 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"
>
>http://repo.or.cz/nasm.git/blob/HEAD:/x86/insns.dat
>
>has:
>
>1378 UD0             void                            [       0f ff]    
>                             186,UNDOC
>1379 UD1             void                            [       0f b9]    
>                             186,UNDOC
>1380 UD2B            void                            [       0f b9]    
>                             186,UNDOC,ND
>1381 UD2             void                            [       0f 0b]    
>                             186
>1382 UD2A            void                            [       0f 0b]    
>                             186,ND
>
>which seems to use the 2 byte version of UD1.
>
>hpa, any input?

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.

This was documented very recently and Nasm hasn't caught up yet (hence the UNDOC flag.)
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

#1586953

FromPeter Zijlstra <peterz@infradead.org>
Date2017-02-23 16:30 +0100
Message-ID<te3vb-35q-3@gated-at.bofh.it>
In reply to#1586950
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..

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web