x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Jann Horn]

Hi!

With the latest kernel from Linus' tree (fb279f4e2386), on a box that
needs KPTI, as soon as I run "sudo <path>/perf top -g", I see this
warning (if I modify the unwinding code to use %px there instead of
%p):

[   83.716065] WARNING: stack recursion on stack type 4
[   83.716070] WARNING: can't dereference registers at
fffffe0000002190 for ip
swapgs_restore_regs_and_return_to_usermode+0x79/0x80

So apparently some ORC annotations are a bit off. I decided to write a
little helper script that can interleave "objtool orc dump" and
"objdump -d" output to make it easier for me to understand what's
going on; you can save it to tools/objtool and run it from there:

==============
#!/usr/bin/env python3
import subprocess
import sys
import re

objfile = sys.argv[1]

orc_dump = subprocess.run([sys.path[0]+'/objtool', 'orc', 'dump',
objfile], stdout=subprocess.PIPE).stdout.decode('utf-8')
orc_map = {}
for line in orc_dump.split('\n'):
  parts = line.split(': ', 1)
  if len(parts) != 2:
    continue
  orc_map[parts[0]] = parts[1]

objdump = subprocess.run(['objdump', '-d', objfile],
stdout=subprocess.PIPE).stdout.decode('utf-8')
cur_section = None
hex_re = re.compile('^[a-f0-9]+$')
for line in objdump.split('\n'):
  if line.startswith('Disassembly of section '):
    cur_section = line.split('of section ')[1].split(':')[0]
  if line.find(':') != -1:
    hex_prefix = line.split(':')[0].lstrip()
    if hex_re.match(hex_prefix):
      orc_val = orc_map.get(cur_section+'+'+hex_prefix)
      if orc_val != None:
        print('\033[92m#######' + orc_val + '\033[0m')
  print(line)
==============

swapgs_restore_regs_and_return_to_usermode+0x79/0x80 is offset 0xaa8
in the object file. Here's the interleaved assembly and ORC unwind
info (with ORC info appearing above the line it refers to):

==============
0000000000000a00 <common_interrupt>:
#######sp:sp+8 bp:(und) type:iret end:0
     a00: 48 83 04 24 80        addq   $0xffffffffffffff80,(%rsp)
     a05: e8 00 00 00 00        callq  a0a <common_interrupt+0xa>
#######sp:(sp+0) bp:(und) type:regs end:0
     a0a: e8 00 00 00 00        callq  a0f <ret_from_intr>

0000000000000a0f <ret_from_intr>:
     a0f: fa                    cli
     a10: 5c                    pop    %rsp
#######sp:sp+0 bp:(und) type:regs end:0
     a11: 65 ff 0c 25 00 00 00 decl   %gs:0x0
     a18: 00
     a19: f6 84 24 88 00 00 00 testb  $0x3,0x88(%rsp)
     a20: 03
     a21: 0f 84 88 00 00 00    je     aaf <retint_kernel>
     a27: 48 89 e7              mov    %rsp,%rdi
     a2a: e8 00 00 00 00        callq  a2f
<swapgs_restore_regs_and_return_to_usermode>

0000000000000a2f <swapgs_restore_regs_and_return_to_usermode>:
     a2f: 41 5f                pop    %r15
#######sp:sp-8 bp:(und) type:regs end:0
     a31: 41 5e                pop    %r14
#######sp:sp-16 bp:(und) type:regs end:0
     a33: 41 5d                pop    %r13
#######sp:sp-24 bp:(und) type:regs end:0
     a35: 41 5c                pop    %r12
#######sp:sp-32 bp:(und) type:regs end:0
     a37: 5d                    pop    %rbp
#######sp:sp-40 bp:(und) type:regs end:0
     a38: 5b                    pop    %rbx
#######sp:sp-48 bp:(und) type:regs end:0
     a39: 41 5b                pop    %r11
#######sp:sp-56 bp:(und) type:regs end:0
     a3b: 41 5a                pop    %r10
#######sp:sp-64 bp:(und) type:regs end:0
     a3d: 41 59                pop    %r9
#######sp:sp-72 bp:(und) type:regs end:0
     a3f: 41 58                pop    %r8
#######sp:sp-80 bp:(und) type:regs end:0
     a41: 58                    pop    %rax
#######sp:sp-88 bp:(und) type:regs end:0
     a42: 59                    pop    %rcx
#######sp:sp-96 bp:(und) type:regs end:0
     a43: 5a                    pop    %rdx
#######sp:sp-104 bp:(und) type:regs end:0
     a44: 5e                    pop    %rsi
#######sp:sp-112 bp:(und) type:regs end:0
     a45: 48 89 e7              mov    %rsp,%rdi
     a48: 65 48 8b 24 25 00 00 mov    %gs:0x0,%rsp
     a4f: 00 00
     a51: ff 77 30              pushq  0x30(%rdi)
#######sp:sp-104 bp:(und) type:regs end:0
     a54: ff 77 28              pushq  0x28(%rdi)
#######sp:sp-96 bp:(und) type:regs end:0
     a57: ff 77 20              pushq  0x20(%rdi)
#######sp:sp-88 bp:(und) type:regs end:0
     a5a: ff 77 18              pushq  0x18(%rdi)
#######sp:sp-80 bp:(und) type:regs end:0
     a5d: ff 77 10              pushq  0x10(%rdi)
#######sp:sp-72 bp:(und) type:regs end:0
     a60: ff 37                pushq  (%rdi)
#######sp:sp-64 bp:(und) type:regs end:0
     a62: 50                    push   %rax
#######sp:sp-56 bp:(und) type:regs end:0
     a63: eb 43                jmp    aa8
<swapgs_restore_regs_and_return_to_usermode+0x79>
     a65: 0f 20 df              mov    %cr3,%rdi
     a68: eb 34                jmp    a9e
<swapgs_restore_regs_and_return_to_usermode+0x6f>
     a6a: 48 89 f8              mov    %rdi,%rax
     a6d: 48 81 e7 ff 07 00 00 and    $0x7ff,%rdi
     a74: 65 48 0f a3 3c 25 00 bt     %rdi,%gs:0x0
     a7b: 00 00 00
     a7e: 73 0f                jae    a8f
<swapgs_restore_regs_and_return_to_usermode+0x60>
     a80: 65 48 0f b3 3c 25 00 btr    %rdi,%gs:0x0
     a87: 00 00 00
     a8a: 48 89 c7              mov    %rax,%rdi
     a8d: eb 08                jmp    a97
<swapgs_restore_regs_and_return_to_usermode+0x68>
     a8f: 48 89 c7              mov    %rax,%rdi
     a92: 48 0f ba ef 3f        bts    $0x3f,%rdi
     a97: 48 81 cf 00 08 00 00 or     $0x800,%rdi
     a9e: 48 81 cf 00 10 00 00 or     $0x1000,%rdi
     aa5: 0f 22 df              mov    %rdi,%cr3
     aa8: 58                    pop    %rax
#######sp:sp-64 bp:(und) type:regs end:0
     aa9: 5f                    pop    %rdi
#######sp:sp-72 bp:(und) type:regs end:0
     aaa: 0f 01 f8              swapgs
     aad: eb 41                jmp    af0 <native_iret>
==============

It looks to me like things go wrong at the point where we switch over
to the trampoline stack? The ORC info claims that we have full user
registers on the trampoline stack (and that we're clobbering them with
our pushes - apparently objtool is not smart enough to realize that
that looks bogus), but at that point we should probably actually use
something like UNWIND_HINT_IRET_REGS, right?


By the way, looking through the rest of the entry stuff, there's some
other funny-looking stuff, too:

============
0000000000000f40 <general_protection>:
#######sp:sp+8 bp:(und) type:iret end:0
     f40:       90                      nop
#######sp:(und) bp:(und) type:call end:0
     f41:       90                      nop
     f42:       90                      nop
#######sp:sp+8 bp:(und) type:iret end:0
     f43:       e8 a8 01 00 00          callq  10f0 <error_entry>
#######sp:sp+0 bp:(und) type:regs end:0
     f48:       f6 84 24 88 00 00 00    testb  $0x3,0x88(%rsp)
     f4f:       03
     f50:       74 00                   je     f52 <general_protection+0x12>
     f52:       48 89 e7                mov    %rsp,%rdi
     f55:       48 8b 74 24 78          mov    0x78(%rsp),%rsi
     f5a:       48 c7 44 24 78 ff ff    movq   $0xffffffffffffffff,0x78(%rsp)
     f61:       ff ff
     f63:       e8 00 00 00 00          callq  f68 <general_protection+0x28>
     f68:       e9 73 02 00 00          jmpq   11e0 <error_exit>
#######sp:(und) bp:(und) type:call end:0
     f6d:       0f 1f 00                nopl   (%rax)
============

So I think on machines without X86_FEATURE_SMAP, trying to unwind from
the two NOPs at f41 and f42 will cause the unwinder to report an
error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1"
marker seems to be reserved for errors.

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Peter Zijlstra]

On Sun, Mar 01, 2020 at 07:02:15AM +0100, Jann Horn wrote:

> 0000000000000a2f <swapgs_restore_regs_and_return_to_usermode>:
>      a2f: 41 5f                pop    %r15
> #######sp:sp-8 bp:(und) type:regs end:0
>      a31: 41 5e                pop    %r14
> #######sp:sp-16 bp:(und) type:regs end:0
>      a33: 41 5d                pop    %r13
> #######sp:sp-24 bp:(und) type:regs end:0
>      a35: 41 5c                pop    %r12
> #######sp:sp-32 bp:(und) type:regs end:0
>      a37: 5d                    pop    %rbp
> #######sp:sp-40 bp:(und) type:regs end:0
>      a38: 5b                    pop    %rbx
> #######sp:sp-48 bp:(und) type:regs end:0
>      a39: 41 5b                pop    %r11
> #######sp:sp-56 bp:(und) type:regs end:0
>      a3b: 41 5a                pop    %r10
> #######sp:sp-64 bp:(und) type:regs end:0
>      a3d: 41 59                pop    %r9
> #######sp:sp-72 bp:(und) type:regs end:0
>      a3f: 41 58                pop    %r8
> #######sp:sp-80 bp:(und) type:regs end:0
>      a41: 58                    pop    %rax
> #######sp:sp-88 bp:(und) type:regs end:0
>      a42: 59                    pop    %rcx
> #######sp:sp-96 bp:(und) type:regs end:0
>      a43: 5a                    pop    %rdx
> #######sp:sp-104 bp:(und) type:regs end:0
>      a44: 5e                    pop    %rsi
> #######sp:sp-112 bp:(und) type:regs end:0
>      a45: 48 89 e7              mov    %rsp,%rdi
>      a48: 65 48 8b 24 25 00 00 mov    %gs:0x0,%rsp
>      a4f: 00 00

Right, so here we flip stacks,

>      a51: ff 77 30              pushq  0x30(%rdi)
> #######sp:sp-104 bp:(und) type:regs end:0
>      a54: ff 77 28              pushq  0x28(%rdi)
> #######sp:sp-96 bp:(und) type:regs end:0
>      a57: ff 77 20              pushq  0x20(%rdi)
> #######sp:sp-88 bp:(und) type:regs end:0
>      a5a: ff 77 18              pushq  0x18(%rdi)
> #######sp:sp-80 bp:(und) type:regs end:0
>      a5d: ff 77 10              pushq  0x10(%rdi)

And here we've pushed an IRET frame

> #######sp:sp-72 bp:(und) type:regs end:0
>      a60: ff 37                pushq  (%rdi)

> It looks to me like things go wrong at the point where we switch over
> to the trampoline stack? The ORC info claims that we have full user
> registers on the trampoline stack (and that we're clobbering them with
> our pushes - apparently objtool is not smart enough to realize that
> that looks bogus), but at that point we should probably actually use
> something like UNWIND_HINT_IRET_REGS, right?

I _think_ you've nailed it, but I'm somewhat new to this part of
objtool.

Josh?

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Josh Poimboeuf]

On Sun, Mar 01, 2020 at 07:02:15AM +0100, Jann Horn wrote:
> It looks to me like things go wrong at the point where we switch over
> to the trampoline stack? The ORC info claims that we have full user
> registers on the trampoline stack (and that we're clobbering them with
> our pushes - apparently objtool is not smart enough to realize that
> that looks bogus), but at that point we should probably actually use
> something like UNWIND_HINT_IRET_REGS, right?

Good timing.  I have a patch set coming in a few days which fixes
several ORC issues, and this was one of them.

> By the way, looking through the rest of the entry stuff, there's some
> other funny-looking stuff, too:
> 
> ============
> 0000000000000f40 <general_protection>:
> #######sp:sp+8 bp:(und) type:iret end:0
>      f40:       90                      nop
> #######sp:(und) bp:(und) type:call end:0
>      f41:       90                      nop
>      f42:       90                      nop
> #######sp:sp+8 bp:(und) type:iret end:0
>      f43:       e8 a8 01 00 00          callq  10f0 <error_entry>
> #######sp:sp+0 bp:(und) type:regs end:0
>      f48:       f6 84 24 88 00 00 00    testb  $0x3,0x88(%rsp)
>      f4f:       03
>      f50:       74 00                   je     f52 <general_protection+0x12>
>      f52:       48 89 e7                mov    %rsp,%rdi
>      f55:       48 8b 74 24 78          mov    0x78(%rsp),%rsi
>      f5a:       48 c7 44 24 78 ff ff    movq   $0xffffffffffffffff,0x78(%rsp)
>      f61:       ff ff
>      f63:       e8 00 00 00 00          callq  f68 <general_protection+0x28>
>      f68:       e9 73 02 00 00          jmpq   11e0 <error_exit>
> #######sp:(und) bp:(und) type:call end:0
>      f6d:       0f 1f 00                nopl   (%rax)
> ============
> 
> So I think on machines without X86_FEATURE_SMAP, trying to unwind from
> the two NOPs at f41 and f42 will cause the unwinder to report an
> error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1"
> marker seems to be reserved for errors.

Hm... good catch.  Not sure why objtool is doing that but I'll look into
it.

-- 
Josh

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Josh Poimboeuf]

On Mon, Mar 02, 2020 at 09:18:29AM -0600, Josh Poimboeuf wrote:
> > So I think on machines without X86_FEATURE_SMAP, trying to unwind from
> > the two NOPs at f41 and f42 will cause the unwinder to report an
> > error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1"
> > marker seems to be reserved for errors.

I think we can blame this one on Peter ;-)

  764eef4b109a ("objtool: Rewrite alt->skip_orig")

With X86_FEATURE_SMAP, alt->skip_orig gets set, which tells objtool to
skip validation of the NOPs.  That has the side effect of not
propagating the ORC state to the NOPs as well.

-- 
Josh

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Josh Poimboeuf]

On Mon, Mar 02, 2020 at 09:52:40AM -0600, Josh Poimboeuf wrote:
> On Mon, Mar 02, 2020 at 09:18:29AM -0600, Josh Poimboeuf wrote:
> > > So I think on machines without X86_FEATURE_SMAP, trying to unwind from
> > > the two NOPs at f41 and f42 will cause the unwinder to report an
> > > error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1"
> > > marker seems to be reserved for errors.
> 
> I think we can blame this one on Peter ;-)
> 
>   764eef4b109a ("objtool: Rewrite alt->skip_orig")
> 
> With X86_FEATURE_SMAP, alt->skip_orig gets set, which tells objtool to
> skip validation of the NOPs.  That has the side effect of not
> propagating the ORC state to the NOPs as well.

I almost forgot about this one, until I rediscovered it just now...
Peter, I just realized you weren't CCed on the original email, oops.

I'm thinking something like this should fix it.  Peter, does this look
ok?

(I should probably split it into two patches: STAC/CLAC fix and then
revert 764eef4b109a.)

From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH] objtool: Fix ORC gap in STAC/CLAC alternatives

Since commit 764eef4b109a ("objtool: Rewrite alt->skip_orig"), the new
skip_orig actually skips objtool validation of the original instruction,
resulting in an empty gap in the ORC data.

Revert the skip_orig logic to how it was before, which is to convert the
original instruction to a NOP.

For the SMAP case, convert the original instruction to a matching
STAC/CLAC, so that it doesn't matter which path is taken.

Fixes: 764eef4b109a ("objtool: Rewrite alt->skip_orig")
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c   | 60 ++++++++++++++++++++++++-----------------
 tools/objtool/special.c | 16 -----------
 tools/objtool/special.h |  1 -
 3 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0c732d586924..541fcf16283c 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -23,7 +23,6 @@
 struct alternative {
 	struct list_head list;
 	struct instruction *insn;
-	bool skip_orig;
 };
 
 const char *objname;
@@ -773,12 +772,26 @@ static int handle_group_alt(struct objtool_file *file,
 	struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL;
 	unsigned long dest_off;
 
+	/*
+	 * For uaccess checking, propagate the STAC/CLAC from the alternative
+	 * to the original insn to avoid paths where we see the STAC but then
+	 * take the NOP instead of CLAC (and vice versa).
+	 */
+	if (!orig_insn->ignore_alts && orig_insn->type == INSN_NOP &&
+	    *new_insn &&
+	    ((*new_insn)->type == INSN_STAC ||
+	     (*new_insn)->type == INSN_CLAC))
+		orig_insn->type = (*new_insn)->type;
+
 	last_orig_insn = NULL;
 	insn = orig_insn;
 	sec_for_each_insn_from(file, insn) {
 		if (insn->offset >= special_alt->orig_off + special_alt->orig_len)
 			break;
 
+		if (special_alt->skip_orig)
+			insn->type = INSN_NOP;
+
 		insn->alt_group = true;
 		last_orig_insn = insn;
 	}
@@ -970,8 +983,6 @@ static int add_special_section_alts(struct objtool_file *file)
 		}
 
 		alt->insn = new_insn;
-		alt->skip_orig = special_alt->skip_orig;
-		orig_insn->ignore_alts |= special_alt->skip_alt;
 		list_add_tail(&alt->list, &orig_insn->alts);
 
 		list_del(&special_alt->list);
@@ -2221,12 +2232,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 		insn->visited |= visited;
 
 		if (!insn->ignore_alts) {
-			bool skip_orig = false;
-
 			list_for_each_entry(alt, &insn->alts, list) {
-				if (alt->skip_orig)
-					skip_orig = true;
-
 				ret = validate_branch(file, func, alt->insn, state);
 				if (ret) {
 					if (backtrace)
@@ -2234,9 +2240,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 					return ret;
 				}
 			}
-
-			if (skip_orig)
-				return 0;
 		}
 
 		switch (insn->type) {
@@ -2325,26 +2328,33 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			break;
 
 		case INSN_STAC:
-			if (state.uaccess) {
-				WARN_FUNC("recursive UACCESS enable", sec, insn->offset);
-				return 1;
-			}
+			if (uaccess) {
+				if (state.uaccess) {
+					WARN_FUNC("recursive UACCESS enable",
+						  sec, insn->offset);
+					return 1;
+				}
 
-			state.uaccess = true;
+				state.uaccess = true;
+			}
 			break;
 
 		case INSN_CLAC:
-			if (!state.uaccess && func) {
-				WARN_FUNC("redundant UACCESS disable", sec, insn->offset);
-				return 1;
-			}
+			if (uaccess) {
+				if (!state.uaccess && func) {
+					WARN_FUNC("redundant UACCESS disable",
+						  sec, insn->offset);
+					return 1;
+				}
 
-			if (func_uaccess_safe(func) && !state.uaccess_stack) {
-				WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
-				return 1;
-			}
+				if (func_uaccess_safe(func) && !state.uaccess_stack) {
+					WARN_FUNC("UACCESS-safe disables UACCESS",
+						  sec, insn->offset);
+					return 1;
+				}
 
-			state.uaccess = false;
+				state.uaccess = false;
+			}
 			break;
 
 		case INSN_STD:
diff --git a/tools/objtool/special.c b/tools/objtool/special.c
index e74e0189de22..ec7adbd32d57 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -100,22 +100,6 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 		 */
 		if (feature == X86_FEATURE_POPCNT)
 			alt->skip_orig = true;
-
-		/*
-		 * If UACCESS validation is enabled; force that alternative;
-		 * otherwise force it the other way.
-		 *
-		 * What we want to avoid is having both the original and the
-		 * alternative code flow at the same time, in that case we can
-		 * find paths that see the STAC but take the NOP instead of
-		 * CLAC and the other way around.
-		 */
-		if (feature == X86_FEATURE_SMAP) {
-			if (uaccess)
-				alt->skip_orig = true;
-			else
-				alt->skip_alt = true;
-		}
 	}
 
 	orig_rela = find_rela_by_dest(elf, sec, offset + entry->orig);
diff --git a/tools/objtool/special.h b/tools/objtool/special.h
index 35061530e46e..ac729a20042c 100644
--- a/tools/objtool/special.h
+++ b/tools/objtool/special.h
@@ -14,7 +14,6 @@ struct special_alt {
 
 	bool group;
 	bool skip_orig;
-	bool skip_alt;
 	bool jump_or_nop;
 
 	struct section *orig_sec;
-- 
2.21.1

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Peter Zijlstra]

On Tue, Apr 28, 2020 at 02:04:50AM -0500, Josh Poimboeuf wrote:
> On Mon, Mar 02, 2020 at 09:52:40AM -0600, Josh Poimboeuf wrote:
> > On Mon, Mar 02, 2020 at 09:18:29AM -0600, Josh Poimboeuf wrote:
> > > > So I think on machines without X86_FEATURE_SMAP, trying to unwind from
> > > > the two NOPs at f41 and f42 will cause the unwinder to report an
> > > > error? Looking at unwind_next_frame(), "sp:(und)" without the "end:1"
> > > > marker seems to be reserved for errors.
> > 
> > I think we can blame this one on Peter ;-)
> > 
> >   764eef4b109a ("objtool: Rewrite alt->skip_orig")
> > 
> > With X86_FEATURE_SMAP, alt->skip_orig gets set, which tells objtool to
> > skip validation of the NOPs.  That has the side effect of not
> > propagating the ORC state to the NOPs as well.
> 
> I almost forgot about this one, until I rediscovered it just now...
> Peter, I just realized you weren't CCed on the original email, oops.

Nah, I got them (through x86@); but I too lost track of it :/

> I'm thinking something like this should fix it.  Peter, does this look
> ok?

Unfortunate. But also, I fear, insufficient. Specifically consider
things like:

	ALTERNATIVE "jmp 1f",
		"alt...
		"..."
		"...insn", X86_FEAT_foo
	1:

This results in something like:


	.text	.altinstr_replacement
	e8 xx	...
	90
	90
	...
	90

Where all our normal single byte nops (0x90) are unreachable with
undefined CFI, but the alternative might have CFI, which is never
propagated.

We ran into this with the validate_alternative stuff from Alexandre.

> @@ -773,12 +772,26 @@ static int handle_group_alt(struct objtool_file *file,
>  	struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL;
>  	unsigned long dest_off;
>  
> +	/*
> +	 * For uaccess checking, propagate the STAC/CLAC from the alternative
> +	 * to the original insn to avoid paths where we see the STAC but then
> +	 * take the NOP instead of CLAC (and vice versa).
> +	 */
> +	if (!orig_insn->ignore_alts && orig_insn->type == INSN_NOP &&
> +	    *new_insn &&
> +	    ((*new_insn)->type == INSN_STAC ||
> +	     (*new_insn)->type == INSN_CLAC))
> +		orig_insn->type = (*new_insn)->type;

Also, this generates a mis-match between actual instruction text and
type. We now have a single byte instruction (0x90) with the type of a 3
byte (SLAC/CLAC). Which currently isn't a problem, but I'm looking at
adding infrastructure for having objtool rewrite instructions.

So rather than hacking around this issue, should we not make
create_orc() smarter?

I'm trying to come up with something, but so far I'm just making a mess.

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Josh Poimboeuf]

On Tue, Apr 28, 2020 at 02:46:27PM +0200, Peter Zijlstra wrote:
> > I'm thinking something like this should fix it.  Peter, does this look
> > ok?
> 
> Unfortunate. But also, I fear, insufficient. Specifically consider
> things like:
> 
> 	ALTERNATIVE "jmp 1f",
> 		"alt...
> 		"..."
> 		"...insn", X86_FEAT_foo
> 	1:
> 
> This results in something like:
> 
> 
> 	.text	.altinstr_replacement
> 	e8 xx	...
> 	90
> 	90
> 	...
> 	90
> 
> Where all our normal single byte nops (0x90) are unreachable with
> undefined CFI, but the alternative might have CFI, which is never
> propagated.
> 
> We ran into this with the validate_alternative stuff from Alexandre.

I don't get what you're saying.  We decided not to allow CFI changes in
alternatives.  And how does this relate to my patch?

> > @@ -773,12 +772,26 @@ static int handle_group_alt(struct objtool_file *file,
> >  	struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL;
> >  	unsigned long dest_off;
> >  
> > +	/*
> > +	 * For uaccess checking, propagate the STAC/CLAC from the alternative
> > +	 * to the original insn to avoid paths where we see the STAC but then
> > +	 * take the NOP instead of CLAC (and vice versa).
> > +	 */
> > +	if (!orig_insn->ignore_alts && orig_insn->type == INSN_NOP &&
> > +	    *new_insn &&
> > +	    ((*new_insn)->type == INSN_STAC ||
> > +	     (*new_insn)->type == INSN_CLAC))
> > +		orig_insn->type = (*new_insn)->type;
> 
> Also, this generates a mis-match between actual instruction text and
> type. We now have a single byte instruction (0x90) with the type of a 3
> byte (SLAC/CLAC). Which currently isn't a problem, but I'm looking at
> adding infrastructure for having objtool rewrite instructions.

But it doesn't actually change the original instruction bytes, it just
changes the decoding.  Is that really going to be a problem?  We do that
in other places as well, and it helps simplify code flow.

Also might I ask why you're going to be rewriting instructions?  That
sounds scary.

> So rather than hacking around this issue, should we not make
> create_orc() smarter?

Maybe, though I don't see how that logic belongs in create_orc().  It
might be tricky distinguishing between normal undefined and "undefined
because of a skip_orig".  Right now create_orc() is blissfully ignorant.

-- 
Josh

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Peter Zijlstra]

On Tue, Apr 28, 2020 at 02:46:27PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 28, 2020 at 02:04:50AM -0500, Josh Poimboeuf wrote:

> > I'm thinking something like this should fix it.  Peter, does this look
> > ok?
> 
> Unfortunate. But also, I fear, insufficient. Specifically consider
> things like:
> 
> 	ALTERNATIVE "jmp 1f",
> 		"alt...
> 		"..."
> 		"...insn", X86_FEAT_foo
> 	1:
> 
> This results in something like:
> 
> 
> 	.text	.altinstr_replacement
> 	e8 xx	...
> 	90
> 	90
> 	...
> 	90
> 
> Where all our normal single byte nops (0x90) are unreachable with
> undefined CFI, but the alternative might have CFI, which is never
> propagated.
> 
> We ran into this with the validate_alternative stuff from Alexandre.

> So rather than hacking around this issue, should we not make
> create_orc() smarter?
> 
> I'm trying to come up with something, but so far I'm just making a mess.

Like this, it's horrid, but it seems to work.

What do you think of the approach? I'll work on cleaning it up if you
don't hate it too much ;-)


---

diff --git a/tools/objtool/orc_gen.c b/tools/objtool/orc_gen.c
index 9d2bf2daaaa6..2a853ae994ea 100644
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -10,17 +10,129 @@
 #include "check.h"
 #include "warn.h"
 
-int create_orc(struct objtool_file *file)
+static bool same_cfi(struct cfi_state *a, struct cfi_state *b)
 {
+	return memcmp(a, b, sizeof(*a));
+}
+
+static struct instruction *next_insn_same_sec(struct objtool_file *file,
+					      struct instruction *insn)
+{
+	struct instruction *next = list_next_entry(insn, list);
+
+	if (!next || &next->list == &file->insn_list || next->sec != insn->sec)
+		return NULL;
+
+	return next;
+}
+
+struct alternative {
+	struct list_head list;
 	struct instruction *insn;
+	bool skip_orig;
+};
+
+static int alt_cfi(struct objtool_file *file,
+		   struct instruction *prev_insn,
+		   struct instruction *orig_insn,
+		   struct instruction *alt_insn)
+{
+	unsigned long orig_offset = orig_insn->offset;
+	unsigned long alt_offset = alt_insn->offset;
+	struct instruction *next_insn;
+	bool orig_orc, alt_orc;
+
+	orig_orc = !prev_insn || same_cfi(&orig_insn->cfi, &prev_insn->cfi);
+	alt_orc  = !prev_insn || same_cfi(&alt_insn->cfi,  &prev_insn->cfi);
+
+again:
+	if ((orig_orc || alt_orc)) {
+		if (orig_insn->offset - orig_offset != alt_insn->offset - alt_offset) {
+			WARN_FUNC("alternative has unaligned ORC", orig_insn->sec, orig_insn->offset);
+			return -1;
+		}
+
+		if (orig_insn->visited) {
+			if (same_cfi(&orig_insn->cfi, &alt_insn->cfi)) {
+				WARN_FUNC("alternative violates ORC invariance", orig_insn->sec, orig_insn->offset);
+				return -1;
+			}
+		} else {
+			/*
+			 * We're in unreachable NOPs, allow the alternative to
+			 * override the CFI/ORC data.
+			 */
+			orig_insn->cfi = alt_insn->cfi;
+		}
+	}
+
+	next_insn = next_insn_same_sec(file, alt_insn);
+	if (!next_insn)
+		return 0;
+
+	if (next_insn->offset == -1 /*FAKE_JUMP_OFFSET*/)
+		return 0;
+
+	alt_orc = same_cfi(&alt_insn->cfi, &next_insn->cfi);
+	alt_insn = next_insn;
+
+	do {
+		next_insn = next_insn_same_sec(file, orig_insn);
+		if (!next_insn)
+			return 0;
+		if (!next_insn->alt_group)
+			return 0;
+
+		orig_orc = next_insn->visited && same_cfi(&orig_insn->cfi, &next_insn->cfi);
+		orig_insn = next_insn;
+	} while (orig_insn->offset - orig_offset < alt_insn->offset - alt_offset);
+
+	goto again;
+}
+
+static void orig_fill(struct objtool_file *file,
+		      struct instruction *prev_insn,
+		      struct instruction *insn)
+{
+	for (;;) {
+		if (!insn->visited)
+			insn->cfi = prev_insn->cfi;
+
+		prev_insn = insn;
+		insn = next_insn_same_sec(file, insn);
+		if (!insn)
+			return;
+		if (!insn->alt_group)
+			return;
+	}
+}
+
+int create_orc(struct objtool_file *file)
+{
+	struct instruction *insn, *prev_insn = NULL;
 
 	for_each_insn(file, insn) {
 		struct orc_entry *orc = &insn->orc;
 		struct cfi_reg *cfa = &insn->cfi.cfa;
 		struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];
+		int ret;
 
 		orc->end = insn->cfi.end;
 
+		if (insn->alt_group && !insn->ignore_alts) {
+			struct alternative *alt;
+
+			list_for_each_entry(alt, &insn->alts, list) {
+				if (alt->insn->offset == -1 /*FAKE_JUMP_OFFSET*/)
+					continue;
+				ret = alt_cfi(file, prev_insn, insn, alt->insn);
+				if (ret)
+					return ret;
+			}
+
+			orig_fill(file, prev_insn, insn);
+		}
+
 		if (cfa->base == CFI_UNDEFINED) {
 			orc->sp_reg = ORC_REG_UNDEFINED;
 			continue;
@@ -76,6 +188,8 @@ int create_orc(struct objtool_file *file)
 		orc->sp_offset = cfa->offset;
 		orc->bp_offset = bp->offset;
 		orc->type = insn->cfi.type;
+
+		prev_insn = insn;
 	}
 
 	return 0;

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Josh Poimboeuf]

On Tue, Apr 28, 2020 at 04:16:14PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 28, 2020 at 02:46:27PM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 28, 2020 at 02:04:50AM -0500, Josh Poimboeuf wrote:
> 
> > > I'm thinking something like this should fix it.  Peter, does this look
> > > ok?
> > 
> > Unfortunate. But also, I fear, insufficient. Specifically consider
> > things like:
> > 
> > 	ALTERNATIVE "jmp 1f",
> > 		"alt...
> > 		"..."
> > 		"...insn", X86_FEAT_foo
> > 	1:
> > 
> > This results in something like:
> > 
> > 
> > 	.text	.altinstr_replacement
> > 	e8 xx	...
> > 	90
> > 	90
> > 	...
> > 	90
> > 
> > Where all our normal single byte nops (0x90) are unreachable with
> > undefined CFI, but the alternative might have CFI, which is never
> > propagated.
> > 
> > We ran into this with the validate_alternative stuff from Alexandre.
> 
> > So rather than hacking around this issue, should we not make
> > create_orc() smarter?
> > 
> > I'm trying to come up with something, but so far I'm just making a mess.
> 
> Like this, it's horrid, but it seems to work.
> 
> What do you think of the approach? I'll work on cleaning it up if you
> don't hate it too much ;-)

How'd you know I'd hate it ;-)

That's quite the monstrosity, and I still don't see the point.  I
thought we decided to just disallow CFI changes in alternatives anyway?
That can be done much simpler.

-- 
Josh

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Peter Zijlstra]

On Tue, Apr 28, 2020 at 09:14:57AM -0500, Josh Poimboeuf wrote:
> On Tue, Apr 28, 2020 at 02:46:27PM +0200, Peter Zijlstra wrote:
> > > I'm thinking something like this should fix it.  Peter, does this look
> > > ok?
> > 
> > Unfortunate. But also, I fear, insufficient. Specifically consider
> > things like:
> > 
> > 	ALTERNATIVE "jmp 1f",
> > 		"alt...
> > 		"..."
> > 		"...insn", X86_FEAT_foo
> > 	1:
> > 
> > This results in something like:
> > 
> > 
> > 	.text	.altinstr_replacement
> > 	e8 xx	...
> > 	90
> > 	90
> > 	...
> > 	90
> > 
> > Where all our normal single byte nops (0x90) are unreachable with
> > undefined CFI, but the alternative might have CFI, which is never
> > propagated.
> > 
> > We ran into this with the validate_alternative stuff from Alexandre.
> 
> I don't get what you're saying.  We decided not to allow CFI changes in
> alternatives.  And how does this relate to my patch?

Ah, I went with a slightly looser invariant rule that allows CFI but
ensures they're the same for all alternatives, and the above orig text
has a giant unreachable hole (that we don't report because NOP), I'm
allowing the alternative CFI changes in that.

Maybe that's too much leaway, but I'm thinking it ought to work.

> > > @@ -773,12 +772,26 @@ static int handle_group_alt(struct objtool_file *file,
> > >  	struct instruction *last_orig_insn, *last_new_insn, *insn, *fake_jump = NULL;
> > >  	unsigned long dest_off;
> > >  
> > > +	/*
> > > +	 * For uaccess checking, propagate the STAC/CLAC from the alternative
> > > +	 * to the original insn to avoid paths where we see the STAC but then
> > > +	 * take the NOP instead of CLAC (and vice versa).
> > > +	 */
> > > +	if (!orig_insn->ignore_alts && orig_insn->type == INSN_NOP &&
> > > +	    *new_insn &&
> > > +	    ((*new_insn)->type == INSN_STAC ||
> > > +	     (*new_insn)->type == INSN_CLAC))
> > > +		orig_insn->type = (*new_insn)->type;
> > 
> > Also, this generates a mis-match between actual instruction text and
> > type. We now have a single byte instruction (0x90) with the type of a 3
> > byte (SLAC/CLAC). Which currently isn't a problem, but I'm looking at
> > adding infrastructure for having objtool rewrite instructions.
> 
> But it doesn't actually change the original instruction bytes, it just
> changes the decoding.  Is that really going to be a problem?  We do that
> in other places as well, and it helps simplify code flow.

It will probably work just fine, it just feels off to me.

> Also might I ask why you're going to be rewriting instructions?  That
> sounds scary.

Variable length jump label support, I can't make gnu-as (I so hate that
thing) emit the right instruction at compile-time :/

> > So rather than hacking around this issue, should we not make
> > create_orc() smarter?
> 
> Maybe, though I don't see how that logic belongs in create_orc().  It
> might be tricky distinguishing between normal undefined and "undefined
> because of a skip_orig".  Right now create_orc() is blissfully ignorant.

Yeah, you're right. I'll look for a better place to stick it. Perhaps I
can frob it in validate_branch() somewhere.

And you're also right on the unreachable because of skip_orig thing,
I'll thnk about that.

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Peter Zijlstra]

On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote:
> That's quite the monstrosity, and I still don't see the point.  I
> thought we decided to just disallow CFI changes in alternatives anyway?
> That can be done much simpler.

Something like so then ?

---
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 8443ec690051..d14d83e6edb0 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file,
 
 		last_new_insn = insn;
 
+		insn->alt_group = true;
 		insn->ignore = orig_insn->ignore_alts;
 		insn->func = orig_insn->func;
 
@@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
 	list_for_each_entry(op, &insn->stack_ops, list) {
 		int res;
 
+		if (insn->alt_group) {
+			WARN_FUNC("alternative has CFI", insn->sec, insn->offset);
+			return 1;
+		}
+
 		res = update_cfi_state(insn, &state->cfi, op);
 		if (res)
 			return res;
@@ -2439,12 +2445,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 
 	sec = insn->sec;
 
-	if (insn->alt_group && list_empty(&insn->alts)) {
-		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
-			  sec, insn->offset);
-		return 1;
-	}
-
 	while (1) {
 		next_insn = next_insn_same_sec(file, insn);
 
@@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				}
 			}
 
-			if (skip_orig)
+			if (skip_orig) {
+				struct instruction *prev_insn = insn;
+				sec_for_each_insn_continue(file, insn) {
+					if (!insn->alt_group)
+						break;
+					if (!insn->visited)
+						insn->cfi = prev_insn->cfi;
+				}
 				return 0;
+			}
 		}
 
 		if (handle_insn_ops(insn, &state))

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Josh Poimboeuf]

On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote:
> > That's quite the monstrosity, and I still don't see the point.  I
> > thought we decided to just disallow CFI changes in alternatives anyway?
> > That can be done much simpler.
> 
> Something like so then ?
> 
> ---
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 8443ec690051..d14d83e6edb0 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file,
>  
>  		last_new_insn = insn;
>  
> +		insn->alt_group = true;
>  		insn->ignore = orig_insn->ignore_alts;
>  		insn->func = orig_insn->func;
>  
> @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
>  	list_for_each_entry(op, &insn->stack_ops, list) {
>  		int res;
>  
> +		if (insn->alt_group) {
> +			WARN_FUNC("alternative has CFI", insn->sec, insn->offset);
> +			return 1;
> +		}
> +

ACK (separate patch)

>  		res = update_cfi_state(insn, &state->cfi, op);
>  		if (res)
>  			return res;
> @@ -2439,12 +2445,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>  
>  	sec = insn->sec;
>  
> -	if (insn->alt_group && list_empty(&insn->alts)) {
> -		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
> -			  sec, insn->offset);
> -		return 1;
> -	}
> -

ACK (separate patch)

>  	while (1) {
>  		next_insn = next_insn_same_sec(file, insn);
>  
> @@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>  				}
>  			}
>  
> -			if (skip_orig)
> +			if (skip_orig) {
> +				struct instruction *prev_insn = insn;
> +				sec_for_each_insn_continue(file, insn) {
> +					if (!insn->alt_group)
> +						break;
> +					if (!insn->visited)
> +						insn->cfi = prev_insn->cfi;
> +				}
>  				return 0;
> +			}

NACK :-)

What happens if you have two alternatives adjacent to each other (which
can definitely happen in this scenario)?

I still like my patch, at least the hack is done before the validate
code, so validate_branch() itself is simpler.

-- 
Josh

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Josh Poimboeuf]

On Tue, Apr 28, 2020 at 10:49:09AM -0500, Josh Poimboeuf wrote:
> On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote:
> > > That's quite the monstrosity, and I still don't see the point.  I
> > > thought we decided to just disallow CFI changes in alternatives anyway?
> > > That can be done much simpler.
> > 
> > Something like so then ?
> > 
> > ---
> > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > index 8443ec690051..d14d83e6edb0 100644
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file,
> >  
> >  		last_new_insn = insn;
> >  
> > +		insn->alt_group = true;
> >  		insn->ignore = orig_insn->ignore_alts;
> >  		insn->func = orig_insn->func;
> >  
> > @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
> >  	list_for_each_entry(op, &insn->stack_ops, list) {
> >  		int res;
> >  
> > +		if (insn->alt_group) {
> > +			WARN_FUNC("alternative has CFI", insn->sec, insn->offset);
> > +			return 1;
> > +		}
> > +
> 
> ACK (separate patch)

BTW, since most people don't know what CFI is, how about something like

	"unsupported stack change in alternatives code"

-- 
Josh

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Sean Christopherson]

On Tue, Apr 28, 2020 at 10:54:13AM -0500, Josh Poimboeuf wrote:
> On Tue, Apr 28, 2020 at 10:49:09AM -0500, Josh Poimboeuf wrote:
> > On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote:
> > > On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote:
> > > > That's quite the monstrosity, and I still don't see the point.  I
> > > > thought we decided to just disallow CFI changes in alternatives anyway?
> > > > That can be done much simpler.
> > > 
> > > Something like so then ?
> > > 
> > > ---
> > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > > index 8443ec690051..d14d83e6edb0 100644
> > > --- a/tools/objtool/check.c
> > > +++ b/tools/objtool/check.c
> > > @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file,
> > >  
> > >  		last_new_insn = insn;
> > >  
> > > +		insn->alt_group = true;
> > >  		insn->ignore = orig_insn->ignore_alts;
> > >  		insn->func = orig_insn->func;
> > >  
> > > @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
> > >  	list_for_each_entry(op, &insn->stack_ops, list) {
> > >  		int res;
> > >  
> > > +		if (insn->alt_group) {
> > > +			WARN_FUNC("alternative has CFI", insn->sec, insn->offset);
> > > +			return 1;
> > > +		}
> > > +
> > 
> > ACK (separate patch)
> 
> BTW, since most people don't know what CFI is, how about something like
> 
> 	"unsupported stack change in alternatives code"

Would it be accurate to print

	"unsupported CFI stack change in alternatives code"?

to give the developer something more explicit to plug into their search
engine?

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Josh Poimboeuf]

On Tue, Apr 28, 2020 at 09:25:51AM -0700, Sean Christopherson wrote:
> On Tue, Apr 28, 2020 at 10:54:13AM -0500, Josh Poimboeuf wrote:
> > On Tue, Apr 28, 2020 at 10:49:09AM -0500, Josh Poimboeuf wrote:
> > > On Tue, Apr 28, 2020 at 05:25:52PM +0200, Peter Zijlstra wrote:
> > > > On Tue, Apr 28, 2020 at 09:31:57AM -0500, Josh Poimboeuf wrote:
> > > > > That's quite the monstrosity, and I still don't see the point.  I
> > > > > thought we decided to just disallow CFI changes in alternatives anyway?
> > > > > That can be done much simpler.
> > > > 
> > > > Something like so then ?
> > > > 
> > > > ---
> > > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > > > index 8443ec690051..d14d83e6edb0 100644
> > > > --- a/tools/objtool/check.c
> > > > +++ b/tools/objtool/check.c
> > > > @@ -940,6 +940,7 @@ static int handle_group_alt(struct objtool_file *file,
> > > >  
> > > >  		last_new_insn = insn;
> > > >  
> > > > +		insn->alt_group = true;
> > > >  		insn->ignore = orig_insn->ignore_alts;
> > > >  		insn->func = orig_insn->func;
> > > >  
> > > > @@ -2242,6 +2243,11 @@ static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
> > > >  	list_for_each_entry(op, &insn->stack_ops, list) {
> > > >  		int res;
> > > >  
> > > > +		if (insn->alt_group) {
> > > > +			WARN_FUNC("alternative has CFI", insn->sec, insn->offset);
> > > > +			return 1;
> > > > +		}
> > > > +
> > > 
> > > ACK (separate patch)
> > 
> > BTW, since most people don't know what CFI is, how about something like
> > 
> > 	"unsupported stack change in alternatives code"
> 
> Would it be accurate to print
> 
> 	"unsupported CFI stack change in alternatives code"?
> 
> to give the developer something more explicit to plug into their search
> engine?

I don't have a strong opinion either way, though this warning is going
to be documented in stack-validation.txt anyway right Peter? :-)

-- 
Josh

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Peter Zijlstra]

On Tue, Apr 28, 2020 at 10:49:09AM -0500, Josh Poimboeuf wrote:
> > @@ -2439,12 +2445,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> >  
> >  	sec = insn->sec;
> >  
> > -	if (insn->alt_group && list_empty(&insn->alts)) {
> > -		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
> > -			  sec, insn->offset);
> > -		return 1;
> > -	}
> > -
> 
> ACK (separate patch)
> 
> >  	while (1) {
> >  		next_insn = next_insn_same_sec(file, insn);
> >  

Yeah, there is one from Julien that does this:

  20200327152847.15294-6-jthierry@redhat.com

> > @@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> >  				}
> >  			}
> >  
> > -			if (skip_orig)
> > +			if (skip_orig) {
> > +				struct instruction *prev_insn = insn;
> > +				sec_for_each_insn_continue(file, insn) {
> > +					if (!insn->alt_group)
> > +						break;
> > +					if (!insn->visited)
> > +						insn->cfi = prev_insn->cfi;
> > +				}
> >  				return 0;
> > +			}
> 
> NACK :-)
> 
> What happens if you have two alternatives adjacent to each other (which
> can definitely happen in this scenario)?

Alexandre's alt_group would help:

  20200414103618.12657-3-alexandre.chartre@oracle.com

Then we can do something like:

static void fill_alternative(struct instruction *insn)
{
	struct instruction *first_insn = insn;
	int alt_group = insn->alt_group;

	sec_for_each_insn_continue(file, insn) {
		if (insn->alt_group != alt_group)
			break;
		if (!insn->visited)
			insn->cfi = first_insn->cfi;
	}
}

> I still like my patch, at least the hack is done before the validate
> code, so validate_branch() itself is simpler.

But it doesn't handle the case where the alternatives themselves have
unreachable holes in them, if that happens we'll generate spurious ORC
entries for them.

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Josh Poimboeuf]

On Tue, Apr 28, 2020 at 06:44:44PM +0200, Peter Zijlstra wrote:
> > > @@ -2494,8 +2494,16 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> > >  				}
> > >  			}
> > >  
> > > -			if (skip_orig)
> > > +			if (skip_orig) {
> > > +				struct instruction *prev_insn = insn;
> > > +				sec_for_each_insn_continue(file, insn) {
> > > +					if (!insn->alt_group)
> > > +						break;
> > > +					if (!insn->visited)
> > > +						insn->cfi = prev_insn->cfi;
> > > +				}
> > >  				return 0;
> > > +			}
> > 
> > NACK :-)
> > 
> > What happens if you have two alternatives adjacent to each other (which
> > can definitely happen in this scenario)?
> 
> Alexandre's alt_group would help:
> 
>   20200414103618.12657-3-alexandre.chartre@oracle.com
> 
> Then we can do something like:
> 
> static void fill_alternative(struct instruction *insn)
> {
> 	struct instruction *first_insn = insn;
> 	int alt_group = insn->alt_group;
> 
> 	sec_for_each_insn_continue(file, insn) {
> 		if (insn->alt_group != alt_group)
> 			break;
> 		if (!insn->visited)
> 			insn->cfi = first_insn->cfi;
> 	}
> }

Ugh...

> > I still like my patch, at least the hack is done before the validate
> > code, so validate_branch() itself is simpler.
> 
> But it doesn't handle the case where the alternatives themselves have
> unreachable holes in them, if that happens we'll generate spurious ORC
> entries for them.

Ah, I see what you mean.

I need to think about it...

-- 
Josh

Re: x86 entry perf unwinding failure (missing IRET_REGS annotation on stack switch?)

[Peter Zijlstra]

On Tue, Apr 28, 2020 at 11:33:27AM -0500, Josh Poimboeuf wrote:
> I don't have a strong opinion either way, though this warning is going
> to be documented in stack-validation.txt anyway right Peter? :-)

Sure... here goes (on top of Alexandre's alt_group patch).

---
Subject: objtool: Fix ORC vs alternatives
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Apr 28 19:37:01 CEST 2020

Jann reported that (for instance) entry_64.o:general_protection has
very odd ORC data:

  0000000000000f40 <general_protection>:
  #######sp:sp+8 bp:(und) type:iret end:0
    f40:       90                      nop
  #######sp:(und) bp:(und) type:call end:0
    f41:       90                      nop
    f42:       90                      nop
  #######sp:sp+8 bp:(und) type:iret end:0
    f43:       e8 a8 01 00 00          callq  10f0 <error_entry>
  #######sp:sp+0 bp:(und) type:regs end:0
    f48:       f6 84 24 88 00 00 00    testb  $0x3,0x88(%rsp)
    f4f:       03
    f50:       74 00                   je     f52 <general_protection+0x12>
    f52:       48 89 e7                mov    %rsp,%rdi
    f55:       48 8b 74 24 78          mov    0x78(%rsp),%rsi
    f5a:       48 c7 44 24 78 ff ff    movq   $0xffffffffffffffff,0x78(%rsp)
    f61:       ff ff
    f63:       e8 00 00 00 00          callq  f68 <general_protection+0x28>
    f68:       e9 73 02 00 00          jmpq   11e0 <error_exit>
  #######sp:(und) bp:(und) type:call end:0
    f6d:       0f 1f 00                nopl   (%rax)

Note the entry at 0xf41. Josh found this was the result of commit:

  764eef4b109a ("objtool: Rewrite alt->skip_orig")

Due to the early return in validate_branch() we no longer set
insn->cfi of the original instruction stream (the NOPs at 0xf41 and
0xf42) and we'll end up with the above weirdness.

In other discussions we realized alternatives should be ORC invariant;
that is, due to there being only a single ORC table, it must be valid
for all alternatives. The easiest way to ensure this is to not allow
any stack modifications in alternatives.

When we enforce this latter observation, we get the property that the
whole alternative must have the same CFI, which we can employ to fix
the former report.

Fixes: 764eef4b109a ("objtool: Rewrite alt->skip_orig")
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/Documentation/stack-validation.txt |    7 ++++
 tools/objtool/check.c                            |   34 ++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

--- a/tools/objtool/Documentation/stack-validation.txt
+++ b/tools/objtool/Documentation/stack-validation.txt
@@ -315,6 +315,13 @@ they mean, and suggestions for how to fi
       function tracing inserts additional calls, which is not obvious from the
       sources).
 
+10. file.o: warning: func()+0x5c: alternative modifies stack
+
+    This means that an alternative includes instructions that modify the
+    stack. The is that there is only one ORC unwind table, this means that the
+    ORC unwind entries must be valid for each of the alternatives. The easiest
+    way to enforce this is to ensure alternative do not contain any ORC
+    entries, which in turn implies the above constraint.
 
 If the error doesn't seem to make sense, it could be a bug in objtool.
 Feel free to ask the objtool maintainer for help.
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2001,6 +2001,11 @@ static int handle_insn_ops(struct instru
 	list_for_each_entry(op, &insn->stack_ops, list) {
 		int res;
 
+		if (insn->alt_group) {
+			WARN_FUNC("alternative modifies stack", insn->sec, insn->offset);
+			return -1;
+		}
+
 		res = update_cfi_state(insn, &state->cfi, op);
 		if (res)
 			return res;
@@ -2177,6 +2182,30 @@ static bool is_branch_to_alternative(str
 }
 
 /*
+ * Alternatives should not contain any ORC entries, this in turn means they
+ * should not contain any CFI ops, which implies all instructions should have
+ * the same same CFI state.
+ *
+ * It is possible to constuct alternatives that have unreachable holes that go
+ * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
+ * states which then results in ORC entries, which we just said we didn't want.
+ *
+ * Avoid them by copying the CFI entry of the first instruction into the whole
+ * alternative.
+ */
+static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
+{
+	struct instruction *first_insn = insn;
+	int alt_group = insn->alt_group;
+
+	sec_for_each_insn_continue(file, insn) {
+		if (insn->alt_group != alt_group)
+			break;
+		insn->cfi = first_insn->cfi;
+	}
+}
+
+/*
  * Follow the branch starting at the given instruction, and recursively follow
  * any other branches (jumps).  Meanwhile, track the frame pointer state at
  * each instruction and validate all the rules described in
@@ -2234,7 +2263,7 @@ static int validate_branch(struct objtoo
 
 		insn->visited |= visited;
 
-		if (!insn->ignore_alts) {
+		if (!insn->ignore_alts && !list_empty(&insn->alts)) {
 			bool skip_orig = false;
 
 			list_for_each_entry(alt, &insn->alts, list) {
@@ -2249,6 +2278,9 @@ static int validate_branch(struct objtoo
 				}
 			}
 
+			if (insn->alt_group)
+				fill_alternative_cfi(file, insn);
+
 			if (skip_orig)
 				return 0;
 		}