BUILD REGRESSION a2e5790d841658485d642196dbb0927303d6c22f

Adding more people for this funky warning from the kbuild robot.

Something is confused. UD0 is 0f ff, the bytes after that shouldn't matter. But I guess they can be interpreted as modrm bytes, and somebody started doing that.

That said, intel only _documents_ UD2 (0f 0b).

Maybe we should avoid using UD0/UD1 entirely.

Linus

They documented UD0 and UD1 a year ago or so:

0F FF /r UD0¹ r32, r/m32 RM Valid Valid Raise invalid opcode exception 0F B9 /r UD1 r32, r/m32 RM Valid Valid Raise invalid opcode exception.

and the footnote says

"1. Some older processors decode the UD0 instruction without a ModR/M byte. As a result, those processors would deliver an invalid- opcode exception instead of a fault on instruction fetch when the instruction with a ModR/M byte (and any implied bytes) would cross a page or segment boundary."

So those two take a ModRM byte.

And we chose UD0 for WARN, see arch/x86/include/asm/bug.h for the reasoning.

Except objdump can't handle that insn because it doesn't have it in its insn tables. Thus it says:

  b3:   0f ff                   (bad)
  b5:   eb                      .byte 0xeb

Or that test should ignore UD0.

Or we should add UD0 only *decoding* support to binutils - not generating.

Regards/Gruss,
    Boris.

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

Argh. So the SDM I'm looking at (March 2017) doesn't list UD0 as having a ModR/M byte, it doesn't have that footnote.

Right, we picked UD0 because we _thought_ everybody agreed it being 2 bytes, just like UD2. This is now not true anymore?

Both UD0 and UD1 are documented to have modrm in the latest Intel SDM I can find (Order number 325462-065US, December 2017):

https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf

for the direct link.

Linus

Thanks, and yes :/ They changed it in October17 (which I actually had a copy of so I'm not sure why I was looking at March17).

Back when we did that patch the SDM didn't have that extra ModR/M on, and binutils disassembled it as the 2 byte (bad) thing, which was all good.

Then someone went and wrecked it.

Yeah, note says UD0 didn't eat a ModRM byte on old CPUs. But then that changed too. Fun stuff changing insn encoding underway.

So if we opt for adding a ModRM byte, could a 0x90 NOP work so that it doesn't shit itself on those old CPUs?

/me goes and checks

Well, no:

b3: 0f ff eb ud0 %ebx,%ebp

decoded with latest objdump turns into:

  b3:   0f                      .byte 0xf
  b4:   ff                      .byte 0xff
  b5:   90                      nop

because 0x90, when used as a ModRM means, AFAICT, register-indirect addressing with a 32-bit offset which would need more bytes. :-\

/me adds more bytes...

Yap, yuck, that works:

b3: 0f ff 90 90 90 90 90 ud0 -0x6f6f6f70(%rax),%edx

I guess we need to experiment a bit to find a suitable byte to add...

Nasty.

Regards/Gruss,
    Boris.

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

We could just also decide that the only thing that the modrm bytes of UD0 actually *affect* is how the CPU might act for a page-crossing instruction.

Because I think that's the only semantic difference: if it's a page-crosser, the instruction could take a page fault before raising the #UD.

Is there any other decode issue we might want to look out for?

Anyway, then we'd make the rule be:

- we promise to always pad up the following bytes (our extra warning
information etc) so that we never have a missing page afterwards (this is presumably practically speaking already the case). It might still be a page-crossing instruction, but we won't take a page fault in kernel space (due to it being at the end of some text sectoin or whatever that changes the NX bit or due to DEBUG_PAGEALLOC having unmapped the next page).

- we special-case the decoder so that we don't get this warning

and then we just ignore the issue entirely.

Hmm?

Linus

Yes, AMD doesn't do UD0 with a ModRM:

"UD0 0F FF Raise an invalid opcode exception"

And I don't know about the other x86 vendors; what fun they've had with the UD*.

I guess we should make sure the following bytes are a valid insn.

Btw Rudolf had experimented with this, CCed, and he showed that on Intel you get a SIGSEGV for a page crosser with UD1 and SIGILL on AMD.

I guess you can get the same with UD0 apparently.

Yeah, I guess we can control the bytes there.

That's easy - the decoder should simply say to upgrade objdump.

Sounds like a plan but with those things the devil's in the detail. Nothing sounds too nasty now, though.

Regards/Gruss,
    Boris.

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

_The_ problem is that new binutils cannot sanely decode any function that has a WARN in (this very much includes perf annotate):

old:

00000000000016a0 <copy_overflow>:

    16a0:       48 89 f2                mov    %rsi,%rdx
    16a3:       89 fe                   mov    %edi,%esi
    16a5:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi
                        16a8: R_X86_64_32S      .rodata.str1.8+0x288
    16ac:       e8 00 00 00 00          callq  16b1 <copy_overflow+0x11>
                        16ad: R_X86_64_PC32     __warn_printk-0x4
    16b1:       0f ff                   (bad)  
    16b3:       c3                      retq   
    16b4:       66 90                   xchg   %ax,%ax
    16b6:       66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
    16bd:       00 00 00 

new:

00000000000016a0 <copy_overflow>:

    16a0:       48 89 f2                mov    %rsi,%rdx
    16a3:       89 fe                   mov    %edi,%esi
    16a5:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi
                        16a8: R_X86_64_32S      .rodata.str1.8+0x288
    16ac:       e8 00 00 00 00          callq  16b1 <copy_overflow+0x11>
                        16ad: R_X86_64_PC32     __warn_printk-0x4
    16b1:       0f ff c3                ud0    %ebx,%eax
    16b4:       66 90                   xchg   %ax,%ax
    16b6:       66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
    16bd:       00 00 00 

I went through the register opcodes and matched it against the ModR/M encoding, and the best option I've found so far is using 0xd6 as the next byte.

That yields:

old:

0000000000001690 <copy_overflow>:

    1690:       48 89 f2                mov    %rsi,%rdx
    1693:       89 fe                   mov    %edi,%esi
    1695:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi

1698: R_X86_64_32S .rodata.str1.8+0x270
169c: e8 00 00 00 00 callq 16a1 <copy_overflow+0x11>
169d: R_X86_64_PC32 __warn_printk-0x4
16a1: 0f ff (bad)
16a3: d6 (bad)
16a4: c3 retq
16a5: 90 nop
16a6: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)

new:

0000000000001690 <copy_overflow>:

    1690:       48 89 f2                mov    %rsi,%rdx
    1693:       89 fe                   mov    %edi,%esi
    1695:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi
                        1698: R_X86_64_32S      .rodata.str1.8+0x270
    169c:       e8 00 00 00 00          callq  16a1 <copy_overflow+0x11>
                        169d: R_X86_64_PC32     __warn_printk-0x4
    16a1:       0f ff d6                ud0    %esi,%edx
    16a4:       c3                      retq
    16a5:       90                      nop
    16a6:       66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
    16ad:       00 00 00

And only grows a defconfig image by 91 bytes, purely for the purpose of being able to disassemble it :/

text data bss dec hex filename

17307211        4890808 1052880 23250899        162c7d3 defconfig-build/vmlinux
17307302        4890808 1052880 23250990        162c82e defconfig-build/vmlinux

---
 arch/x86/include/asm/bug.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index 34d99af43994..f0d5b4a1512d 100644
@@ -12,16 +12,21 @@
  * (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)
+ *
+ * And now someone went and taught binutils about UD0 as taking a ModR/M too
+ * and it generates crap when disassembling the kernel. Stick a pointless 0xD6
+ * ModR/M on, which the old binutils decodes as (bad) and the new binutils sees
+ * as a valid single byte ModR/M.
  */
 
-#define ASM_UD0		".byte 0x0f, 0xff"
+#define ASM_UD0		".byte 0x0f, 0xff, 0xd6"
 #define ASM_UD1		".byte 0x0f, 0xb9" /* + ModRM */
 #define ASM_UD2		".byte 0x0f, 0x0b"
 
 #define INSN_UD0	0xff0f
 #define INSN_UD2	0x0b0f
 
-#define LEN_UD0		2
+#define LEN_UD0		3
 
 #ifdef CONFIG_GENERIC_BUG
 

And note that this example is fairly 'good' as far as things go, it only eats the single byte ret instruction and is on an instruction boundary again, so the rest is good.

There are examples where what it eats results in shifting the instruction boundary and the rest is _complete_ crap.

Look what it does to sched_cpu_dying:

    7199:       48 39 c3                cmp    %rax,%rbx
    719c:       0f 84 cc 00 00 00       je     726e <sched_cpu_dying+0x1fe>
    71a2:       0f ff 41 c6             ud0    -0x3a(%rcx),%eax
    71a6:       87 3c 07                xchg   %edi,(%rdi,%rax,1)
    71a9:       00 00                   add    %al,(%rax)
    71ab:       00 8b 43 04 83 f8       add    %cl,-0x77cfbbd(%rbx)
    71b1:       01 0f                   add    %ecx,(%rdi)
    71b3:       85 4f ff                test   %ecx,-0x1(%rdi)
    71b6:       ff                      (bad)  
    71b7:       ff 48 8b                decl   -0x75(%rax)
    71ba:       44 24 08                rex.R and $0x8,%al

    7199:       48 39 c3                cmp    %rax,%rbx
    719c:       0f 84 cc 00 00 00       je     726e <sched_cpu_dying+0x1fe>
    71a2:       0f ff                   (bad)  
    71a4:       41 c6 87 3c 07 00 00    movb   $0x0,0x73c(%r15)
    71ab:       00 
    71ac:       8b 43 04                mov    0x4(%rbx),%eax
    71af:       83 f8 01                cmp    $0x1,%eax
    71b2:       0f 85 4f ff ff ff       jne    7107 <sched_cpu_dying+0x97>
    71b8:       48 8b 44 24 08          mov    0x8(%rsp),%rax
    71bd:       48 8b 74 24 10          mov    0x10(%rsp),%rsi

From: Peter Zijlstra

...

Wouldn't 0xc3 work as well.
A retq is probably better than an extra (bad).

Actually objdump ought to be more explicit than (bad) for the explicit UD0/1

David

I explicity did not use c3 because that generates spurious "retq" in the code flow which is mighty confusing when you read the stuff.

Ugh.

Is there any reason why we don't just use UD2 and avoid this whole issue?

Do we even *do* anything different for UD0 vs UD2? I see that 'is_valid_bugaddr()' seems to accept either, I'm not seeing anything that cares.

Yes, yes, we currently use UD0 for warnings, and UD2 for BUG(), but the fact is, we don't actually seem to care, because the *real* differentiator is BUGFLAG_WARNING.

No?

So all the UD0 games seem a bit pointless. Let's not use it at all because it's so badly documented.

Linus

That's what I initially had, but then Arjan told me that a number of emulators fatally terminate on UD2 since they assume it to be BUG. Which is when we all went looking at UD1/UD0.

But yes, for our purposes UD2 is perfectly fine too, it will just mess up the people doing bringup and the like.

Oh, we'll inconvenience people inside Intel?

The same people who changed and screwed up the definition of UD0 just a couple of months ago?

I'm *so* sorry for the inconvenience to those people /s.

F*ck them. They introduced the problem, they get to live with it.

Linus

OK, here's the patch.. It leaves the UD0 in traps.c such that people could recompile their kernel with a simple change.

---
Subject: x86: Use UD2 for WARN

Since the Intel SDM added an ModR/M byte to UD0 and binutils followed that specification, we now cannot disassemble our kernel anymore.

This now mean Intel and AMD disagree on the encoding of UD0. And instead of playing games with additional bytes that are valid ModR/M and single byte instructions (0xd6 for instance), simply use UD2 for both WARN and BUG.

---
 arch/x86/include/asm/bug.h | 15 ++++++---------
 arch/x86/kernel/traps.c    |  2 +-
 2 files changed, 7 insertions(+), 10 deletions(-)

index 34d99af43994..2ea100e7a60f 100644
@@ -5,23 +5,20 @@
 #include <linux/stringify.h>
 
 /*
- * 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.
+ * Despite that some emulators terminate on UD2, we use it 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)
+ * Since various instruction decoders/specs disagree on the encoding of
+ * UD0/UD1,
  */
 
-#define ASM_UD0		".byte 0x0f, 0xff"
+#define ASM_UD0		".byte 0x0f, 0xff" /* + ModRM (for Intel) */
 #define ASM_UD1		".byte 0x0f, 0xb9" /* + ModRM */
 #define ASM_UD2		".byte 0x0f, 0x0b"
 
 #define INSN_UD0	0xff0f
 #define INSN_UD2	0x0b0f
 
-#define LEN_UD0		2
+#define LEN_UD2		2
 
 #ifdef CONFIG_GENERIC_BUG
 

@@ -77,7 +74,7 @@ do {								\
 	unreachable();						\
 } while (0)
 
-#define __WARN_FLAGS(flags)	_BUG_FLAGS(ASM_UD0, BUGFLAG_WARNING|(flags))
+#define __WARN_FLAGS(flags)	_BUG_FLAGS(ASM_UD2, BUGFLAG_WARNING|(flags))
 
 #include <asm-generic/bug.h>
 

index 446c9ef8cfc3..3d9b2308e7fa 100644
@@ -181,7 +181,7 @@ int fixup_bug(struct pt_regs *regs, int trapnr)
 		break;
 
 	case BUG_TRAP_TYPE_WARN:
-		regs->ip += LEN_UD0;
+		regs->ip += LEN_UD2;
 		return 1;
 	}
 

Ack, that seems about the best we can do.

Linus

	     ^

Does that comma mean the sentence continues somewhere?

Regards/Gruss,
    Boris.

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

Josh Poimboeuf (2):
objtool: Fix seg fault in ignore_unreachable_insn()
x86: Annotate WARN-related UD2 as reachable

 arch/x86/include/asm/bug.h |  6 +++++-
 tools/objtool/check.c      | 12 +++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

2.14.3

Peter Zijlstra's patch for converting WARN() to use UD2 triggered a bunch of false "unreachable instruction" warnings, which then triggered a seg fault in ignore_unreachable_insn().

The seg fault happened when it tried to dereference a NULL 'insn->func' pointer. Thanks to static_cpu_has(), some functions can jump to a non-function area in the .altinstr_aux section. That breaks ignore_unreachable_insn()'s assumption that it's always inside the original function.

Make sure ignore_unreachable_insn() only follows jumps within the current function.

---
 tools/objtool/check.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

index b00b1896547e..4ec1a3bf7bf2 100644
@@ -1899,13 +1899,19 @@ static bool ignore_unreachable_insn(struct instruction *insn)
 		if (is_kasan_insn(insn) || is_ubsan_insn(insn))
 			return true;
 
-		if (insn->type == INSN_JUMP_UNCONDITIONAL && insn->jump_dest) {
-			insn = insn->jump_dest;
-			continue;
+		if (insn->type == INSN_JUMP_UNCONDITIONAL) {
+			if (insn->jump_dest &&
+			    insn->jump_dest->func == insn->func) {
+				insn = insn->jump_dest;
+				continue;
+			}
+
+			break;
 		}
 
 		if (insn->offset + insn->len >= insn->func->offset + insn->func->len)
 			break;
+
 		insn = list_next_entry(insn, list);
 	}
 

2.14.3

By default, objtool assumes that a UD2 is a dead end. This is mainly because GCC 7+ sometimes inserts a UD2 when it detects a divide-by-zero condition.

Now that WARN() is moving back to UD2, annotate the code after it as reachable so objtool can follow the code flow.

---
 arch/x86/include/asm/bug.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index f2da5fc90e5c..697d89d6fa76 100644
@@ -74,7 +74,11 @@ do {								\
 	unreachable();						\
 } while (0)
 
-#define __WARN_FLAGS(flags)	_BUG_FLAGS(ASM_UD2, BUGFLAG_WARNING|(flags))
+#define __WARN_FLAGS(flags)					\
+do {								\
+	_BUG_FLAGS(ASM_UD2, BUGFLAG_WARNING|(flags));		\
+	annotate_reachable();					\
+} while (0)
 
 #include <asm-generic/bug.h>
 

2.14.3

Ah, thanks for saving me the trouble of digging these out of history. I did remember you did them back then, now I don't even have to go find them ;-)

No that's what doing patches while baking cup-cakes with my oldest daughter gets me :-)

Intel Order Number: 325383-064US, October 2017, documents UD0, UD1, and UD2. Section A.2.5, Table A-1, says:

Use the 0F0B opcode (UD2 instruction), the 0FB9H opcode (UD1 instruction), or the 0FFFH opcode (UD0 instruction) when deliberately trying to generate an invalid opcode exception (#UD).

Could this regression be a result of:

commit f5b5fab1780c98b74526dbac527574bd02dc16f8

Author: Randy Dunlap <[email protected]>
Date:   Mon Dec 11 10:38:36 2017 -0800

x86/decoder: Fix and update the opcodes map

Update x86-opcode-map.txt based on the October 2017 Intel SDM publication. Fix INVPID to INVVPID.
Add UD0 and UD1 instruction opcodes.

~Randy

Ahh, I had an older version.

Looking at the latest one I can find (325462-065US), it does specify that it has a modrm byte:

0F FF /r UD0 1 r32, r/m32

so I think that our opcode maps are wrong, and it's a bit dangerous to put random constants right after the UD0.

Maybe we should make our use of UD0 have a third byte: add a harmless modrm byte before the warning constants?

But yes, at least my objdump just thinks it's a bad 2-byte sequence, and doesn't look at any modrm bytes at all.

Linus

Look what my objdump does:

$ objdump --version
GNU objdump (GNU Binutils for Debian) 2.29.90.20180122

$ objdump -dr defconfig-build/kernel/sched/core.o | grep ud0
    183a:       0f ff 65 48             ud0    0x48(%rbp),%esp
    1881:       0f ff c3                ud0    %ebx,%eax
    18b1:       0f ff 89 c0 48 0f a3    ud0    -0x5cf0b740(%rcx),%ecx
    1940:       0f ff 89 c0 48 0f a3    ud0    -0x5cf0b740(%rcx),%ecx
    19c2:       0f ff 89 c0 48 0f a3    ud0    -0x5cf0b740(%rcx),%ecx

So yeah, we're screwed :-(

Adding a harmless modr/m to our UD0 would grow the kernel image. ARGGH

PeterA, at the time there was talk of arranging a single byte UD instruction. Any luck arranging that?