[PATCH v2 00/10] objtool validation of static branches and retpoline

[PATCH v2 00/10] objtool validation of static branches and retpoline

[Peter Zijlstra]

Because dwmw2 is a wee bit paranoid and GCC has a history of being slightly
retarded at times; here are a few patches that validate it will in fact
generate sensible code for static branches.

Also, per dwmw2's request, code to validate retpoline code does not include
dynamic jump or calls.

---
 arch/x86/include/asm/alternative-asm.h |   3 +-
 arch/x86/include/asm/alternative.h     |   7 +-
 arch/x86/include/asm/cpufeature.h      |  82 ++++++------
 arch/x86/include/asm/jump_label.h      |  23 ++++
 arch/x86/kernel/head_64.S              |   5 +-
 scripts/Makefile.build                 |   4 +
 tools/objtool/builtin-check.c          |   5 +-
 tools/objtool/builtin-orc.c            |   4 +-
 tools/objtool/check.c                  | 223 +++++++++++++++++++++++++++++++--
 tools/objtool/check.h                  |   3 +-
 tools/objtool/special.c                |  13 +-
 tools/objtool/special.h                |   1 +
 12 files changed, 318 insertions(+), 55 deletions(-)

[PATCH v2 01/10] x86: Reindent _static_cpu_has

[Peter Zijlstra]

[-- Attachment #1: peterz-fix-static_cpu_has.patch --]
[-- Type: text/plain, Size: 3013 bytes --]

Because its daft..

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/cpufeature.h |   78 +++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 39 deletions(-)

--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -145,45 +145,45 @@ extern void clear_cpu_cap(struct cpuinfo
  */
 static __always_inline __pure bool _static_cpu_has(u16 bit)
 {
-		asm_volatile_goto("1: jmp 6f\n"
-			 "2:\n"
-			 ".skip -(((5f-4f) - (2b-1b)) > 0) * "
-			         "((5f-4f) - (2b-1b)),0x90\n"
-			 "3:\n"
-			 ".section .altinstructions,\"a\"\n"
-			 " .long 1b - .\n"		/* src offset */
-			 " .long 4f - .\n"		/* repl offset */
-			 " .word %P1\n"			/* always replace */
-			 " .byte 3b - 1b\n"		/* src len */
-			 " .byte 5f - 4f\n"		/* repl len */
-			 " .byte 3b - 2b\n"		/* pad len */
-			 ".previous\n"
-			 ".section .altinstr_replacement,\"ax\"\n"
-			 "4: jmp %l[t_no]\n"
-			 "5:\n"
-			 ".previous\n"
-			 ".section .altinstructions,\"a\"\n"
-			 " .long 1b - .\n"		/* src offset */
-			 " .long 0\n"			/* no replacement */
-			 " .word %P0\n"			/* feature bit */
-			 " .byte 3b - 1b\n"		/* src len */
-			 " .byte 0\n"			/* repl len */
-			 " .byte 0\n"			/* pad len */
-			 ".previous\n"
-			 ".section .altinstr_aux,\"ax\"\n"
-			 "6:\n"
-			 " testb %[bitnum],%[cap_byte]\n"
-			 " jnz %l[t_yes]\n"
-			 " jmp %l[t_no]\n"
-			 ".previous\n"
-			 : : "i" (bit), "i" (X86_FEATURE_ALWAYS),
-			     [bitnum] "i" (1 << (bit & 7)),
-			     [cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
-			 : : t_yes, t_no);
-	t_yes:
-		return true;
-	t_no:
-		return false;
+	asm_volatile_goto("1: jmp 6f\n"
+		 "2:\n"
+		 ".skip -(((5f-4f) - (2b-1b)) > 0) * "
+			 "((5f-4f) - (2b-1b)),0x90\n"
+		 "3:\n"
+		 ".section .altinstructions,\"a\"\n"
+		 " .long 1b - .\n"		/* src offset */
+		 " .long 4f - .\n"		/* repl offset */
+		 " .word %P1\n"			/* always replace */
+		 " .byte 3b - 1b\n"		/* src len */
+		 " .byte 5f - 4f\n"		/* repl len */
+		 " .byte 3b - 2b\n"		/* pad len */
+		 ".previous\n"
+		 ".section .altinstr_replacement,\"ax\"\n"
+		 "4: jmp %l[t_no]\n"
+		 "5:\n"
+		 ".previous\n"
+		 ".section .altinstructions,\"a\"\n"
+		 " .long 1b - .\n"		/* src offset */
+		 " .long 0\n"			/* no replacement */
+		 " .word %P0\n"			/* feature bit */
+		 " .byte 3b - 1b\n"		/* src len */
+		 " .byte 0\n"			/* repl len */
+		 " .byte 0\n"			/* pad len */
+		 ".previous\n"
+		 ".section .altinstr_aux,\"ax\"\n"
+		 "6:\n"
+		 " testb %[bitnum],%[cap_byte]\n"
+		 " jnz %l[t_yes]\n"
+		 " jmp %l[t_no]\n"
+		 ".previous\n"
+		 : : "i" (bit), "i" (X86_FEATURE_ALWAYS),
+		     [bitnum] "i" (1 << (bit & 7)),
+		     [cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
+		 : : t_yes, t_no);
+t_yes:
+	return true;
+t_no:
+	return false;
 }
 
 #define static_cpu_has(bit)					\

Re: [PATCH v2 01/10] x86: Reindent _static_cpu_has

[Borislav Petkov]

On Tue, Jan 16, 2018 at 03:28:26PM +0100, Peter Zijlstra wrote:
> Because its daft..
> 
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/cpufeature.h |   78 +++++++++++++++++++-------------------
>  1 file changed, 39 insertions(+), 39 deletions(-)

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

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

[PATCH v2 02/10] x86: Update _static_cpu_has to use all named variables

[Peter Zijlstra]

[-- Attachment #1: peterz-fix-static_cpu_has_names.patch --]
[-- Type: text/plain, Size: 1491 bytes --]


Requested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/cpufeature.h |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -153,7 +153,7 @@ static __always_inline __pure bool _stat
 		 ".section .altinstructions,\"a\"\n"
 		 " .long 1b - .\n"		/* src offset */
 		 " .long 4f - .\n"		/* repl offset */
-		 " .word %P1\n"			/* always replace */
+		 " .word %P[always]\n"		/* always replace */
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 5f - 4f\n"		/* repl len */
 		 " .byte 3b - 2b\n"		/* pad len */
@@ -165,7 +165,7 @@ static __always_inline __pure bool _stat
 		 ".section .altinstructions,\"a\"\n"
 		 " .long 1b - .\n"		/* src offset */
 		 " .long 0\n"			/* no replacement */
-		 " .word %P0\n"			/* feature bit */
+		 " .word %P[feature]\n"		/* feature bit */
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 0\n"			/* repl len */
 		 " .byte 0\n"			/* pad len */
@@ -176,8 +176,9 @@ static __always_inline __pure bool _stat
 		 " jnz %l[t_yes]\n"
 		 " jmp %l[t_no]\n"
 		 ".previous\n"
-		 : : "i" (bit), "i" (X86_FEATURE_ALWAYS),
-		     [bitnum] "i" (1 << (bit & 7)),
+		 : : [feature]  "i" (bit),
+		     [always]   "i" (X86_FEATURE_ALWAYS),
+		     [bitnum]   "i" (1 << (bit & 7)),
 		     [cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
 		 : : t_yes, t_no);
 t_yes:

Re: [PATCH v2 02/10] x86: Update _static_cpu_has to use all named variables

[Borislav Petkov]

On Tue, Jan 16, 2018 at 03:28:27PM +0100, Peter Zijlstra wrote:

C'mon... you can do it... I believe in you..., you can give me that commit
message, even if it is a single sentence...

:-)))

> Requested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/cpufeature.h |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

With a commit msg:

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 02/10] x86: Update _static_cpu_has to use all named variables

[Peter Zijlstra]

On Thu, Jan 18, 2018 at 12:21:12PM +0100, Borislav Petkov wrote:
> On Tue, Jan 16, 2018 at 03:28:27PM +0100, Peter Zijlstra wrote:
> 
> C'mon... you can do it... I believe in you..., you can give me that commit
> message, even if it is a single sentence...

You mean the subject alone don't count?

Re: [PATCH v2 02/10] x86: Update _static_cpu_has to use all named variables

[Borislav Petkov]

On Thu, Jan 18, 2018 at 04:09:40PM +0100, Peter Zijlstra wrote:
> You mean the subject alone don't count?

Yeah. Those empty commit messages make it look like a private repo with
WIP patches. The crap I have here.

:-)

-- 
Regards/Gruss,
    Boris.

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

[PATCH v2 03/10] x86: Add a type field to alt_instr

[Peter Zijlstra]

[-- Attachment #1: peterz-alternative-type.patch --]
[-- Type: text/plain, Size: 3057 bytes --]

Add a type field to the alternative description. For now this will be
used to annotate static_cpu_has() but possible future uses include
using it to implement alternative alignment and special NOP handling.

Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/alternative-asm.h |    3 ++-
 arch/x86/include/asm/alternative.h     |    6 +++++-
 arch/x86/include/asm/cpufeature.h      |    2 ++
 tools/objtool/special.c                |    2 +-
 4 files changed, 10 insertions(+), 3 deletions(-)

--- a/arch/x86/include/asm/alternative-asm.h
+++ b/arch/x86/include/asm/alternative-asm.h
@@ -25,13 +25,14 @@
  * enough information for the alternatives patching code to patch an
  * instruction. See apply_alternatives().
  */
-.macro altinstruction_entry orig alt feature orig_len alt_len pad_len
+.macro altinstruction_entry orig alt feature orig_len alt_len pad_len type=0
 	.long \orig - .
 	.long \alt - .
 	.word \feature
 	.byte \orig_len
 	.byte \alt_len
 	.byte \pad_len
+	.byte \type
 .endm
 
 /*
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -45,6 +45,8 @@
 #define LOCK_PREFIX ""
 #endif
 
+#define ALT_TYPE_DEFAULT	0
+
 struct alt_instr {
 	s32 instr_offset;	/* original instruction */
 	s32 repl_offset;	/* offset to replacement instruction */
@@ -52,6 +54,7 @@ struct alt_instr {
 	u8  instrlen;		/* length of original instruction */
 	u8  replacementlen;	/* length of new instruction */
 	u8  padlen;		/* length of build-time padding */
+	u8  type;		/* type of alternative */
 } __packed;
 
 /*
@@ -127,7 +130,8 @@ static inline int alternatives_text_rese
 	" .word " __stringify(feature) "\n"		/* feature bit     */ \
 	" .byte " alt_total_slen "\n"			/* source len      */ \
 	" .byte " alt_rlen(num) "\n"			/* replacement len */ \
-	" .byte " alt_pad_len "\n"			/* pad len */
+	" .byte " alt_pad_len "\n"			/* pad len */	      \
+	" .byte 0 \n"					/* type */
 
 #define ALTINSTR_REPLACEMENT(newinstr, feature, num)	/* replacement */     \
 	b_replacement(num)":\n\t" newinstr "\n" e_replacement(num) ":\n\t"
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -157,6 +157,7 @@ static __always_inline __pure bool _stat
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 5f - 4f\n"		/* repl len */
 		 " .byte 3b - 2b\n"		/* pad len */
+		 " .byte 0\n"			/* type */
 		 ".previous\n"
 		 ".section .altinstr_replacement,\"ax\"\n"
 		 "4: jmp %l[t_no]\n"
@@ -169,6 +170,7 @@ static __always_inline __pure bool _stat
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 0\n"			/* repl len */
 		 " .byte 0\n"			/* pad len */
+		 " .byte 0\n"			/* type */
 		 ".previous\n"
 		 ".section .altinstr_aux,\"ax\"\n"
 		 "6:\n"
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -34,7 +34,7 @@
 #define JUMP_ORIG_OFFSET	0
 #define JUMP_NEW_OFFSET		8
 
-#define ALT_ENTRY_SIZE		13
+#define ALT_ENTRY_SIZE		14
 #define ALT_ORIG_OFFSET		0
 #define ALT_NEW_OFFSET		4
 #define ALT_FEATURE_OFFSET	8

Re: [PATCH v2 03/10] x86: Add a type field to alt_instr

[Josh Poimboeuf]

On Tue, Jan 16, 2018 at 03:28:28PM +0100, Peter Zijlstra wrote:
> Add a type field to the alternative description. For now this will be
> used to annotate static_cpu_has() but possible future uses include
> using it to implement alternative alignment and special NOP handling.
> 
> Suggested-by: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/alternative-asm.h |    3 ++-
>  arch/x86/include/asm/alternative.h     |    6 +++++-
>  arch/x86/include/asm/cpufeature.h      |    2 ++
>  tools/objtool/special.c                |    2 +-
>  4 files changed, 10 insertions(+), 3 deletions(-)
> 
> --- a/arch/x86/include/asm/alternative-asm.h
> +++ b/arch/x86/include/asm/alternative-asm.h
> @@ -25,13 +25,14 @@
>   * enough information for the alternatives patching code to patch an
>   * instruction. See apply_alternatives().
>   */
> -.macro altinstruction_entry orig alt feature orig_len alt_len pad_len
> +.macro altinstruction_entry orig alt feature orig_len alt_len pad_len type=0
>  	.long \orig - .
>  	.long \alt - .
>  	.word \feature
>  	.byte \orig_len
>  	.byte \alt_len
>  	.byte \pad_len
> +	.byte \type
>  .endm
>  
>  /*
> --- a/arch/x86/include/asm/alternative.h
> +++ b/arch/x86/include/asm/alternative.h
> @@ -45,6 +45,8 @@
>  #define LOCK_PREFIX ""
>  #endif
>  
> +#define ALT_TYPE_DEFAULT	0
> +
>  struct alt_instr {
>  	s32 instr_offset;	/* original instruction */
>  	s32 repl_offset;	/* offset to replacement instruction */
> @@ -52,6 +54,7 @@ struct alt_instr {
>  	u8  instrlen;		/* length of original instruction */
>  	u8  replacementlen;	/* length of new instruction */
>  	u8  padlen;		/* length of build-time padding */
> +	u8  type;		/* type of alternative */
>  } __packed;

Should we put a comment above the struct with a reminder to also update
objtool ALT_ENTRY_SIZE (and possibly the ALT_*_OFFSET macros) when they
change the struct?

Unfortunately it's against policy to include kernel headers from the
tools subdirectory, so we generally have to either hard code things like
this, or have a duplicated header file which needs to be kept in sync.

-- 
Josh

Re: [PATCH v2 03/10] x86: Add a type field to alt_instr

[Borislav Petkov]

On Tue, Jan 16, 2018 at 04:49:55PM -0600, Josh Poimboeuf wrote:
> Unfortunately it's against policy to include kernel headers from the
> tools subdirectory, so we generally have to either hard code things like
> this, or have a duplicated header file which needs to be kept in sync.

Yeah, and AFAIR, perf tool guys already have machinery that does check
whether stuff went out of sync and they resync the required definitions.

You could reuse that...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 03/10] x86: Add a type field to alt_instr

[Josh Poimboeuf]

On Tue, Jan 16, 2018 at 11:53:25PM +0100, Borislav Petkov wrote:
> On Tue, Jan 16, 2018 at 04:49:55PM -0600, Josh Poimboeuf wrote:
> > Unfortunately it's against policy to include kernel headers from the
> > tools subdirectory, so we generally have to either hard code things like
> > this, or have a duplicated header file which needs to be kept in sync.
> 
> Yeah, and AFAIR, perf tool guys already have machinery that does check
> whether stuff went out of sync and they resync the required definitions.
> 
> You could reuse that...

We could, but it can be a real PITA to keep those files in sync, as they
often change for minor changes which don't even matter.  So it might not
be worth the trouble.  Objtool seems to complain when the alternatives
struct size changes anyway, so it's probably not a big deal.

-- 
Josh

Re: [PATCH v2 03/10] x86: Add a type field to alt_instr

[Borislav Petkov]

On Tue, Jan 16, 2018 at 03:28:28PM +0100, Peter Zijlstra wrote:
> Add a type field to the alternative description. For now this will be
> used to annotate static_cpu_has() but possible future uses include
> using it to implement alternative alignment and special NOP handling.
> 
> Suggested-by: Borislav Petkov <bp@alien8.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/alternative-asm.h |    3 ++-
>  arch/x86/include/asm/alternative.h     |    6 +++++-

There's a

	DPRINTK("feat: %d*32+%d, old: (%p, len: %d), repl: (%p, len: %d), pad: %d",

in arch/x86/kernel/alternative.c which dumps the whole alt_instr struct
for debugging. Please add the type there too.

With that and Josh's suggestion added:

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

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

[PATCH v2 04/10] objtool: Implement base jump_assert support

[Peter Zijlstra]

[-- Attachment #1: peter_zijlstra-q-objtool_vs_jump_label.patch --]
[-- Type: text/plain, Size: 4017 bytes --]

Implement a jump_label assertion that asserts that the code location
is indeed only reachable through a static_branch. Because if GCC is
absolutely retaded it could generate code like:

	xor rax,rax
	NOP/JMP 1f
	mov $1, rax
1:
	test rax,rax
	jz 2f
	<do-code>
2:

instead of the sensible:

	NOP/JMP 1f
	<do-code>
1:

This implements objtool infrastructure for ensuring the code ends up
sane, since we'll rely on that for correctness and security.

We tag the instructions after the static branch with static_jump_dest=true;
that is the instruction after the NOP and the instruction at the
JMP+disp site.

Then, when we read the .discard.jump_assert section, we assert that
each entry points to an instruction that has static_jump_dest set.

With this we can assert that the code emitted for the if statement
ends up at the static jump location and nothing untowards happened.

Cc: Borislav Petkov <bp@alien8.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++--
 tools/objtool/check.h |    1 
 2 files changed, 69 insertions(+), 2 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -687,8 +687,17 @@ static int handle_jump_alt(struct objtoo
 			   struct instruction *orig_insn,
 			   struct instruction **new_insn)
 {
-	if (orig_insn->type == INSN_NOP)
+	struct instruction *next_insn = list_next_entry(orig_insn, list);
+
+	if (orig_insn->type == INSN_NOP) {
+		/*
+		 * If orig_insn is a NOP, then new_insn is the branch target
+		 * for when it would've been a JMP.
+		 */
+		next_insn->static_jump_dest = true;
+		(*new_insn)->static_jump_dest = true;
 		return 0;
+	}
 
 	if (orig_insn->type != INSN_JUMP_UNCONDITIONAL) {
 		WARN_FUNC("unsupported instruction at jump label",
@@ -696,7 +705,16 @@ static int handle_jump_alt(struct objtoo
 		return -1;
 	}
 
-	*new_insn = list_next_entry(orig_insn, list);
+	/*
+	 * Otherwise, orig_insn is a JMP and it will have orig_insn->jump_dest.
+	 * In this case we'll effectively NOP the alt by pointing new_insn at
+	 * next_insn.
+	 */
+	orig_insn->jump_dest->static_jump_dest = true;
+	next_insn->static_jump_dest = true;
+
+	*new_insn = next_insn;
+
 	return 0;
 }
 
@@ -1067,6 +1085,50 @@ static int read_unwind_hints(struct objt
 	return 0;
 }
 
+static int assert_static_jumps(struct objtool_file *file)
+{
+	struct section *sec, *relasec;
+	struct instruction *insn;
+	struct rela *rela;
+	int i;
+
+	sec = find_section_by_name(file->elf, ".discard.jump_assert");
+	if (!sec)
+		return 0;
+
+	relasec = sec->rela;
+	if (!relasec) {
+		WARN("missing .rela.discard.jump_assert section");
+		return -1;
+	}
+
+	if (sec->len % sizeof(unsigned long)) {
+		WARN("jump_assert size mismatch: %d %ld", sec->len, sizeof(unsigned long));
+		return -1;
+	}
+
+	for (i = 0; i < sec->len / sizeof(unsigned long); i++) {
+		rela = find_rela_by_dest(sec, i * sizeof(unsigned long));
+		if (!rela) {
+			WARN("can't find rela for jump_assert[%d]", i);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("can't find insn for jump_assert[%d]", i);
+			return -1;
+		}
+
+		if (!insn->static_jump_dest) {
+			WARN_FUNC("static assert FAIL", insn->sec, insn->offset);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 static int decode_sections(struct objtool_file *file)
 {
 	int ret;
@@ -2000,6 +2062,10 @@ int check(const char *_objname, bool _no
 		goto out;
 	warnings += ret;
 
+	ret = assert_static_jumps(&file);
+	if (ret < 0)
+		return ret;
+
 	if (list_empty(&file.insn_list))
 		goto out;
 
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -45,6 +45,7 @@ struct instruction {
 	unsigned char type;
 	unsigned long immediate;
 	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
+	bool static_jump_dest;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct list_head alts;

[PATCH v2 05/10] x86: Annotate static_cpu_has alternative

[Peter Zijlstra]

[-- Attachment #1: peterz-x86-static_cpu_has-annotate.patch --]
[-- Type: text/plain, Size: 1696 bytes --]

In order to recognise static_cpu_has() alternatives from any other
alternative without dodgy heuristics, we need to explicitly mark them.
Use the new type field for this.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/alternative.h |    1 +
 arch/x86/include/asm/cpufeature.h  |    5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -46,6 +46,7 @@
 #endif
 
 #define ALT_TYPE_DEFAULT	0
+#define ALT_TYPE_STATIC_CPU_HAS	1 /* objtool, static_cpu_has */
 
 struct alt_instr {
 	s32 instr_offset;	/* original instruction */
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -157,7 +157,7 @@ static __always_inline __pure bool _stat
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 5f - 4f\n"		/* repl len */
 		 " .byte 3b - 2b\n"		/* pad len */
-		 " .byte 0\n"			/* type */
+		 " .byte %P[type]\n"		/* type */
 		 ".previous\n"
 		 ".section .altinstr_replacement,\"ax\"\n"
 		 "4: jmp %l[t_no]\n"
@@ -170,7 +170,7 @@ static __always_inline __pure bool _stat
 		 " .byte 3b - 1b\n"		/* src len */
 		 " .byte 0\n"			/* repl len */
 		 " .byte 0\n"			/* pad len */
-		 " .byte 0\n"			/* type */
+		 " .byte %P[type]\n"		/* type */
 		 ".previous\n"
 		 ".section .altinstr_aux,\"ax\"\n"
 		 "6:\n"
@@ -181,6 +181,7 @@ static __always_inline __pure bool _stat
 		 : : [feature]  "i" (bit),
 		     [always]   "i" (X86_FEATURE_ALWAYS),
 		     [bitnum]   "i" (1 << (bit & 7)),
+		     [type]	"i" (ALT_TYPE_STATIC_CPU_HAS),
 		     [cap_byte] "m" (((const char *)boot_cpu_data.x86_capability)[bit >> 3])
 		 : : t_yes, t_no);
 t_yes:

Re: [PATCH v2 05/10] x86: Annotate static_cpu_has alternative

[Borislav Petkov]

On Tue, Jan 16, 2018 at 03:28:30PM +0100, Peter Zijlstra wrote:
> In order to recognise static_cpu_has() alternatives from any other
> alternative without dodgy heuristics, we need to explicitly mark them.
> Use the new type field for this.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/alternative.h |    1 +
>  arch/x86/include/asm/cpufeature.h  |    5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> --- a/arch/x86/include/asm/alternative.h
> +++ b/arch/x86/include/asm/alternative.h
> @@ -46,6 +46,7 @@
>  #endif
>  
>  #define ALT_TYPE_DEFAULT	0
> +#define ALT_TYPE_STATIC_CPU_HAS	1 /* objtool, static_cpu_has */

Just a nitpick: let's be more verbose in that comment:

/*
 * This alt_instr descriptor is part of a static_cpu_has() construct. Use it to
 * detect its type when processing with other tools, like objtool, for example.
 */
#define ALT_TYPE_STATIC_CPU_HAS	1

With that:

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

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

[PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()

[Peter Zijlstra]

[-- Attachment #1: peterz-objtool-static_cpu_has.patch --]
[-- Type: text/plain, Size: 2867 bytes --]

Unlike the jump_label bits, static_cpu_has is implemented with
alternatives. We use the new type field to distinguish them from any
other alternatives

Like jump_labels, make static_cpu_has set static_jump_dest on the
instructions after the static branch such that we can assert on it.

Cc: Borislav Petkov <bp@alien8.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c   |   21 +++++++++++++++++++++
 tools/objtool/special.c |   11 +++++++++++
 tools/objtool/special.h |    1 +
 3 files changed, 33 insertions(+)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -636,6 +636,12 @@ static int handle_group_alt(struct objto
 	fake_jump->ignore = true;
 
 	if (!special_alt->new_len) {
+		/*
+		 * The NOP case for _static_cpu_has()
+		 */
+		if (special_alt->static_feat)
+			fake_jump->jump_dest->static_jump_dest = true;
+
 		*new_insn = fake_jump;
 		return 0;
 	}
@@ -664,6 +670,21 @@ static int handle_group_alt(struct objto
 				  insn->sec, insn->offset);
 			return -1;
 		}
+
+		if (special_alt->static_feat) {
+			if (insn->type != INSN_JUMP_UNCONDITIONAL) {
+				WARN_FUNC("not an unconditional jump in _static_cpu_has()",
+					  insn->sec, insn->offset);
+			}
+			if (insn->jump_dest == fake_jump) {
+				WARN_FUNC("jump inside alternative for _static_cpu_has()",
+					  insn->sec, insn->offset);
+			}
+			/*
+			 * The JMP+disp case for _static_cpu_has()
+			 */
+			insn->jump_dest->static_jump_dest = true;
+		}
 	}
 
 	if (!last_new_insn) {
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -40,6 +40,11 @@
 #define ALT_FEATURE_OFFSET	8
 #define ALT_ORIG_LEN_OFFSET	10
 #define ALT_NEW_LEN_OFFSET	11
+#define ALT_PADDING_OFFSET	12
+#define ALT_TYPE_OFFSET		13
+
+#define ALT_TYPE_DEFAULT	0
+#define ALT_TYPE_STATIC_CPU_HAS	1
 
 #define X86_FEATURE_POPCNT (4*32+23)
 
@@ -99,10 +104,13 @@ static int get_alt_entry(struct elf *elf
 
 	if (entry->feature) {
 		unsigned short feature;
+		unsigned char type;
 
 		feature = *(unsigned short *)(sec->data->d_buf + offset +
 					      entry->feature);
 
+		type = *(unsigned char *)(sec->data->d_buf + offset + ALT_TYPE_OFFSET);
+
 		/*
 		 * It has been requested that we don't validate the !POPCNT
 		 * feature path which is a "very very small percentage of
@@ -110,6 +118,9 @@ static int get_alt_entry(struct elf *elf
 		 */
 		if (feature == X86_FEATURE_POPCNT)
 			alt->skip_orig = true;
+
+		if (type == ALT_TYPE_STATIC_CPU_HAS)
+			alt->static_feat = true;
 	}
 
 	orig_rela = find_rela_by_dest(sec, offset + entry->orig);
--- a/tools/objtool/special.h
+++ b/tools/objtool/special.h
@@ -27,6 +27,7 @@ struct special_alt {
 	bool group;
 	bool skip_orig;
 	bool jump_or_nop;
+	bool static_feat;
 
 	struct section *orig_sec;
 	unsigned long orig_off;

Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()

[Josh Poimboeuf]

On Tue, Jan 16, 2018 at 03:28:31PM +0100, Peter Zijlstra wrote:
> Unlike the jump_label bits, static_cpu_has is implemented with
> alternatives. We use the new type field to distinguish them from any
> other alternatives
> 
> Like jump_labels, make static_cpu_has set static_jump_dest on the
> instructions after the static branch such that we can assert on it.
> 
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  tools/objtool/check.c   |   21 +++++++++++++++++++++
>  tools/objtool/special.c |   11 +++++++++++
>  tools/objtool/special.h |    1 +
>  3 files changed, 33 insertions(+)
> 
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -636,6 +636,12 @@ static int handle_group_alt(struct objto
>  	fake_jump->ignore = true;
>  
>  	if (!special_alt->new_len) {
> +		/*
> +		 * The NOP case for _static_cpu_has()
> +		 */
> +		if (special_alt->static_feat)

s/static_feat/static_cpu_has/ ?

> +			fake_jump->jump_dest->static_jump_dest = true;
> +
>  		*new_insn = fake_jump;
>  		return 0;
>  	}
> @@ -664,6 +670,21 @@ static int handle_group_alt(struct objto
>  				  insn->sec, insn->offset);
>  			return -1;
>  		}
> +
> +		if (special_alt->static_feat) {
> +			if (insn->type != INSN_JUMP_UNCONDITIONAL) {
> +				WARN_FUNC("not an unconditional jump in _static_cpu_has()",
> +					  insn->sec, insn->offset);
> +			}

So I think this is trying to assert the fact that you're only expecting
a single instruction which is an unconditional jump.  We already weeded
out non-jumps earlier in the loop, so would it make sense to do this
check before the INSN_JUMP_CONDITIONAL/INSN_JUMP_UNCONDITIONAL check a
little higher up?

> +			if (insn->jump_dest == fake_jump) {
> +				WARN_FUNC("jump inside alternative for _static_cpu_has()",
> +					  insn->sec, insn->offset);
> +			}

The error message doesn't seem to match the condition, so I'm not sure
which one you're trying to check, or why.

IIRC, 'insn->jump_dest == fake_jump' means we reached the end of the
alternative code block without hitting a jump.

But based on the loop exit condition, I don't think it's ever possible
for insn->jump_dest to ever point to the fake_jump at the end.

> +			/*
> +			 * The JMP+disp case for _static_cpu_has()
> +			 */
> +			insn->jump_dest->static_jump_dest = true;
> +		}
>  	}
>  
>  	if (!last_new_insn) {
> --- a/tools/objtool/special.c
> +++ b/tools/objtool/special.c
> @@ -40,6 +40,11 @@
>  #define ALT_FEATURE_OFFSET	8
>  #define ALT_ORIG_LEN_OFFSET	10
>  #define ALT_NEW_LEN_OFFSET	11
> +#define ALT_PADDING_OFFSET	12
> +#define ALT_TYPE_OFFSET		13

We don't need the ALT_PADDING_OFFSET define (notice we have gaps
already, there are only defines for the ones we use).

> +
> +#define ALT_TYPE_DEFAULT	0
> +#define ALT_TYPE_STATIC_CPU_HAS	1
>  
>  #define X86_FEATURE_POPCNT (4*32+23)
>  
> @@ -99,10 +104,13 @@ static int get_alt_entry(struct elf *elf
>  
>  	if (entry->feature) {

Since this block now uses more than entry->feature, and is now just a
general alternatives thing, can you change it to

	if (entry->feature == ALT_FEATURE_OFFSET)

so it's more clear and slightly more future proof?

>  		unsigned short feature;
> +		unsigned char type;
>  
>  		feature = *(unsigned short *)(sec->data->d_buf + offset +
>  					      entry->feature);
>  
> +		type = *(unsigned char *)(sec->data->d_buf + offset + ALT_TYPE_OFFSET);
> +
>  		/*
>  		 * It has been requested that we don't validate the !POPCNT
>  		 * feature path which is a "very very small percentage of
> @@ -110,6 +118,9 @@ static int get_alt_entry(struct elf *elf
>  		 */
>  		if (feature == X86_FEATURE_POPCNT)
>  			alt->skip_orig = true;
> +
> +		if (type == ALT_TYPE_STATIC_CPU_HAS)
> +			alt->static_feat = true;
>  	}
>  
>  	orig_rela = find_rela_by_dest(sec, offset + entry->orig);
> --- a/tools/objtool/special.h
> +++ b/tools/objtool/special.h
> @@ -27,6 +27,7 @@ struct special_alt {
>  	bool group;
>  	bool skip_orig;
>  	bool jump_or_nop;
> +	bool static_feat;
>  
>  	struct section *orig_sec;
>  	unsigned long orig_off;
> 
> 

-- 
Josh

Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()

[Peter Zijlstra]

On Tue, Jan 16, 2018 at 05:02:56PM -0600, Josh Poimboeuf wrote:
> On Tue, Jan 16, 2018 at 03:28:31PM +0100, Peter Zijlstra wrote:
> > +		if (special_alt->static_feat)
> 
> s/static_feat/static_cpu_has/ ?

done.

> > @@ -664,6 +670,21 @@ static int handle_group_alt(struct objto
> >  				  insn->sec, insn->offset);
> >  			return -1;
> >  		}
> > +
> > +		if (special_alt->static_feat) {
> > +			if (insn->type != INSN_JUMP_UNCONDITIONAL) {
> > +				WARN_FUNC("not an unconditional jump in _static_cpu_has()",
> > +					  insn->sec, insn->offset);
> > +			}
> 
> So I think this is trying to assert the fact that you're only expecting
> a single instruction which is an unconditional jump.

Correct.

>                                                      We already weeded
> out non-jumps earlier in the loop, so would it make sense to do this
> check before the INSN_JUMP_CONDITIONAL/INSN_JUMP_UNCONDITIONAL check a
> little higher up?

This way we keep all the special_alt->static_cpu_has bits together.

> > +			if (insn->jump_dest == fake_jump) {
> > +				WARN_FUNC("jump inside alternative for _static_cpu_has()",
> > +					  insn->sec, insn->offset);
> > +			}
> 
> The error message doesn't seem to match the condition, so I'm not sure
> which one you're trying to check, or why.
> 
> IIRC, 'insn->jump_dest == fake_jump' means we reached the end of the
> alternative code block without hitting a jump.
> 
> But based on the loop exit condition, I don't think it's ever possible
> for insn->jump_dest to ever point to the fake_jump at the end.

Oof, now what was I thinking again.. So that fake_jump is inserted at
the end of the alternative and jumps to the code after where the
alternative will be patched in to simulate the code flow.

If there is a jump inside the alternative that jumps to the end, it's
destination will be set to the fake jump, we have this clause for that:

	dest_off = insn->offset + insn->len + insn->immediate;
	if (dest_off == special_alt->new_off + special_alt->new_len)
		insn->jump_dest = fake_jump;

if that happens for static_cpu_has(), bad things happened.

So the only way for a jump to have fake_jump as destination is if the
jump is inside the alternative (but to the end) and we must assert this
didn't happen.

Unlikely, yes, but I figured we want to know about it if it ever does
happen.

> > --- a/tools/objtool/special.c
> > +++ b/tools/objtool/special.c
> > @@ -40,6 +40,11 @@
> >  #define ALT_FEATURE_OFFSET	8
> >  #define ALT_ORIG_LEN_OFFSET	10
> >  #define ALT_NEW_LEN_OFFSET	11
> > +#define ALT_PADDING_OFFSET	12
> > +#define ALT_TYPE_OFFSET		13
> 
> We don't need the ALT_PADDING_OFFSET define (notice we have gaps
> already, there are only defines for the ones we use).

Over zealous I suppose, I've taken it out again.

> > +
> > +#define ALT_TYPE_DEFAULT	0
> > +#define ALT_TYPE_STATIC_CPU_HAS	1
> >  
> >  #define X86_FEATURE_POPCNT (4*32+23)
> >  
> > @@ -99,10 +104,13 @@ static int get_alt_entry(struct elf *elf
> >  
> >  	if (entry->feature) {
> 
> Since this block now uses more than entry->feature, and is now just a
> general alternatives thing, can you change it to
> 
> 	if (entry->feature == ALT_FEATURE_OFFSET)
> 
> so it's more clear and slightly more future proof?

I've made it entry->group instead. How about the below on top?

---

Subject: objtool: Introduce special_type
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Jan 17 10:16:44 CET 2018

Use an enum for the special_alt entries instead of a collection of
booleans.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c   |   11 ++++++++---
 tools/objtool/special.c |   21 +++++++++------------
 tools/objtool/special.h |   10 ++++++++--
 3 files changed, 25 insertions(+), 17 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -773,7 +773,7 @@ static int add_special_section_alts(stru
 			continue;
 
 		new_insn = NULL;
-		if (!special_alt->group || special_alt->new_len) {
+		if (special_alt->type != alternative || special_alt->new_len) {
 			new_insn = find_insn(file, special_alt->new_sec,
 					     special_alt->new_off);
 			if (!new_insn) {
@@ -785,16 +785,21 @@ static int add_special_section_alts(stru
 			}
 		}
 
-		if (special_alt->group) {
+		switch(special_alt->type) {
+		case alternative:
 			ret = handle_group_alt(file, special_alt, orig_insn,
 					       &new_insn);
 			if (ret)
 				goto out;
-		} else if (special_alt->jump_or_nop) {
+			break;
+
+		case jump_label:
 			ret = handle_jump_alt(file, special_alt, orig_insn,
 					      &new_insn);
 			if (ret)
 				goto out;
+
+			break;
 		}
 
 		alt = malloc(sizeof(*alt));
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -49,7 +49,7 @@
 
 struct special_entry {
 	const char *sec;
-	bool group, jump_or_nop;
+	enum special_type type;
 	unsigned char size, orig, new;
 	unsigned char orig_len, new_len; /* group only */
 	unsigned char feature; /* ALTERNATIVE macro CPU feature */
@@ -58,7 +58,7 @@ struct special_entry {
 struct special_entry entries[] = {
 	{
 		.sec = ".altinstructions",
-		.group = true,
+		.type = alternative,
 		.size = ALT_ENTRY_SIZE,
 		.orig = ALT_ORIG_OFFSET,
 		.orig_len = ALT_ORIG_LEN_OFFSET,
@@ -68,13 +68,14 @@ struct special_entry entries[] = {
 	},
 	{
 		.sec = "__jump_table",
-		.jump_or_nop = true,
+		.type = jump_label,
 		.size = JUMP_ENTRY_SIZE,
 		.orig = JUMP_ORIG_OFFSET,
 		.new = JUMP_NEW_OFFSET,
 	},
 	{
 		.sec = "__ex_table",
+		.type = exception,
 		.size = EX_ENTRY_SIZE,
 		.orig = EX_ORIG_OFFSET,
 		.new = EX_NEW_OFFSET,
@@ -92,18 +93,14 @@ static int get_alt_entry(struct elf *elf
 	offset = idx * entry->size;
 	data = sec->data->d_buf + offset;
 
-	alt->group = entry->group;
-	alt->jump_or_nop = entry->jump_or_nop;
+	alt->type = entry->type;
 
-	if (alt->group) {
-		alt->orig_len = *(unsigned char *)(data + entry->orig_len);
-		alt->new_len = *(unsigned char *)(data + entry->new_len);
-	}
-
-	if (entry->group) {
+	if (alt->type == alternative) {
 		unsigned short feature;
 		unsigned char type;
 
+		alt->orig_len = *(unsigned char *)(data + entry->orig_len);
+		alt->new_len = *(unsigned char *)(data + entry->new_len);
 		feature = *(unsigned short *)(data + ALT_FEATURE_OFFSET);
 		type = *(unsigned char *)(data + ALT_TYPE_OFFSET);
 
@@ -133,7 +130,7 @@ static int get_alt_entry(struct elf *elf
 	alt->orig_sec = orig_rela->sym->sec;
 	alt->orig_off = orig_rela->addend;
 
-	if (!entry->group || alt->new_len) {
+	if (entry->type != alternative || alt->new_len) {
 		new_rela = find_rela_by_dest(sec, offset + entry->new);
 		if (!new_rela) {
 			WARN_FUNC("can't find new rela",
--- a/tools/objtool/special.h
+++ b/tools/objtool/special.h
@@ -21,12 +21,18 @@
 #include <stdbool.h>
 #include "elf.h"
 
+enum special_type {
+	alternative,
+	jump_label,
+	exception,
+};
+
 struct special_alt {
 	struct list_head list;
 
-	bool group;
+	enum special_type type;
+
 	bool skip_orig;
-	bool jump_or_nop;
 	bool static_cpu_has;
 
 	struct section *orig_sec;

Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()

[Josh Poimboeuf]

On Wed, Jan 17, 2018 at 10:19:45AM +0100, Peter Zijlstra wrote:
> > > @@ -664,6 +670,21 @@ static int handle_group_alt(struct objto
> > >  				  insn->sec, insn->offset);
> > >  			return -1;
> > >  		}
> > > +
> > > +		if (special_alt->static_feat) {
> > > +			if (insn->type != INSN_JUMP_UNCONDITIONAL) {
> > > +				WARN_FUNC("not an unconditional jump in _static_cpu_has()",
> > > +					  insn->sec, insn->offset);
> > > +			}
> > 
> > So I think this is trying to assert the fact that you're only expecting
> > a single instruction which is an unconditional jump.
> 
> Correct.
> 
> >                                                      We already weeded
> > out non-jumps earlier in the loop, so would it make sense to do this
> > check before the INSN_JUMP_CONDITIONAL/INSN_JUMP_UNCONDITIONAL check a
> > little higher up?
> 
> This way we keep all the special_alt->static_cpu_has bits together.

But if the instruction is something other than a conditional jump, you
won't get a warning.

> > > +			if (insn->jump_dest == fake_jump) {
> > > +				WARN_FUNC("jump inside alternative for _static_cpu_has()",
> > > +					  insn->sec, insn->offset);
> > > +			}
> > 
> > The error message doesn't seem to match the condition, so I'm not sure
> > which one you're trying to check, or why.
> > 
> > IIRC, 'insn->jump_dest == fake_jump' means we reached the end of the
> > alternative code block without hitting a jump.
> > 
> > But based on the loop exit condition, I don't think it's ever possible
> > for insn->jump_dest to ever point to the fake_jump at the end.
> 
> Oof, now what was I thinking again.. So that fake_jump is inserted at
> the end of the alternative and jumps to the code after where the
> alternative will be patched in to simulate the code flow.
> 
> If there is a jump inside the alternative that jumps to the end, it's
> destination will be set to the fake jump, we have this clause for that:
> 
> 	dest_off = insn->offset + insn->len + insn->immediate;
> 	if (dest_off == special_alt->new_off + special_alt->new_len)
> 		insn->jump_dest = fake_jump;
> 
> if that happens for static_cpu_has(), bad things happened.
> 
> So the only way for a jump to have fake_jump as destination is if the
> jump is inside the alternative (but to the end) and we must assert this
> didn't happen.
> 
> Unlikely, yes, but I figured we want to know about it if it ever does
> happen.

Ok.

> > > +
> > > +#define ALT_TYPE_DEFAULT	0
> > > +#define ALT_TYPE_STATIC_CPU_HAS	1
> > >  
> > >  #define X86_FEATURE_POPCNT (4*32+23)
> > >  
> > > @@ -99,10 +104,13 @@ static int get_alt_entry(struct elf *elf
> > >  
> > >  	if (entry->feature) {
> > 
> > Since this block now uses more than entry->feature, and is now just a
> > general alternatives thing, can you change it to
> > 
> > 	if (entry->feature == ALT_FEATURE_OFFSET)
> > 
> > so it's more clear and slightly more future proof?
> 
> I've made it entry->group instead. How about the below on top?
> 
> ---
> 
> Subject: objtool: Introduce special_type
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Jan 17 10:16:44 CET 2018
> 
> Use an enum for the special_alt entries instead of a collection of
> booleans.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  tools/objtool/check.c   |   11 ++++++++---
>  tools/objtool/special.c |   21 +++++++++------------
>  tools/objtool/special.h |   10 ++++++++--
>  3 files changed, 25 insertions(+), 17 deletions(-)
> 
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -773,7 +773,7 @@ static int add_special_section_alts(stru
>  			continue;
>  
>  		new_insn = NULL;
> -		if (!special_alt->group || special_alt->new_len) {
> +		if (special_alt->type != alternative || special_alt->new_len) {
>  			new_insn = find_insn(file, special_alt->new_sec,
>  					     special_alt->new_off);
>  			if (!new_insn) {
> @@ -785,16 +785,21 @@ static int add_special_section_alts(stru
>  			}
>  		}
>  
> -		if (special_alt->group) {
> +		switch(special_alt->type) {
> +		case alternative:
>  			ret = handle_group_alt(file, special_alt, orig_insn,
>  					       &new_insn);
>  			if (ret)
>  				goto out;
> -		} else if (special_alt->jump_or_nop) {
> +			break;
> +
> +		case jump_label:
>  			ret = handle_jump_alt(file, special_alt, orig_insn,
>  					      &new_insn);
>  			if (ret)
>  				goto out;
> +
> +			break;
>  		}
>  
>  		alt = malloc(sizeof(*alt));
> --- a/tools/objtool/special.c
> +++ b/tools/objtool/special.c
> @@ -49,7 +49,7 @@
>  
>  struct special_entry {
>  	const char *sec;
> -	bool group, jump_or_nop;
> +	enum special_type type;
>  	unsigned char size, orig, new;
>  	unsigned char orig_len, new_len; /* group only */
>  	unsigned char feature; /* ALTERNATIVE macro CPU feature */
> @@ -58,7 +58,7 @@ struct special_entry {
>  struct special_entry entries[] = {
>  	{
>  		.sec = ".altinstructions",
> -		.group = true,
> +		.type = alternative,
>  		.size = ALT_ENTRY_SIZE,
>  		.orig = ALT_ORIG_OFFSET,
>  		.orig_len = ALT_ORIG_LEN_OFFSET,
> @@ -68,13 +68,14 @@ struct special_entry entries[] = {
>  	},
>  	{
>  		.sec = "__jump_table",
> -		.jump_or_nop = true,
> +		.type = jump_label,
>  		.size = JUMP_ENTRY_SIZE,
>  		.orig = JUMP_ORIG_OFFSET,
>  		.new = JUMP_NEW_OFFSET,
>  	},
>  	{
>  		.sec = "__ex_table",
> +		.type = exception,
>  		.size = EX_ENTRY_SIZE,
>  		.orig = EX_ORIG_OFFSET,
>  		.new = EX_NEW_OFFSET,
> @@ -92,18 +93,14 @@ static int get_alt_entry(struct elf *elf
>  	offset = idx * entry->size;
>  	data = sec->data->d_buf + offset;
>  
> -	alt->group = entry->group;
> -	alt->jump_or_nop = entry->jump_or_nop;
> +	alt->type = entry->type;
>  
> -	if (alt->group) {
> -		alt->orig_len = *(unsigned char *)(data + entry->orig_len);
> -		alt->new_len = *(unsigned char *)(data + entry->new_len);
> -	}
> -
> -	if (entry->group) {
> +	if (alt->type == alternative) {
>  		unsigned short feature;
>  		unsigned char type;
>  
> +		alt->orig_len = *(unsigned char *)(data + entry->orig_len);
> +		alt->new_len = *(unsigned char *)(data + entry->new_len);
>  		feature = *(unsigned short *)(data + ALT_FEATURE_OFFSET);
>  		type = *(unsigned char *)(data + ALT_TYPE_OFFSET);
>  
> @@ -133,7 +130,7 @@ static int get_alt_entry(struct elf *elf
>  	alt->orig_sec = orig_rela->sym->sec;
>  	alt->orig_off = orig_rela->addend;
>  
> -	if (!entry->group || alt->new_len) {
> +	if (entry->type != alternative || alt->new_len) {
>  		new_rela = find_rela_by_dest(sec, offset + entry->new);
>  		if (!new_rela) {
>  			WARN_FUNC("can't find new rela",
> --- a/tools/objtool/special.h
> +++ b/tools/objtool/special.h
> @@ -21,12 +21,18 @@
>  #include <stdbool.h>
>  #include "elf.h"
>  
> +enum special_type {
> +	alternative,
> +	jump_label,
> +	exception,
> +};
> +
>  struct special_alt {
>  	struct list_head list;
>  
> -	bool group;
> +	enum special_type type;
> +
>  	bool skip_orig;
> -	bool jump_or_nop;
>  	bool static_cpu_has;
>  
>  	struct section *orig_sec;

Yes, nice improvement on the existing code.

-- 
Josh

Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()

[Josh Poimboeuf]

On Wed, Jan 17, 2018 at 08:27:59AM -0600, Josh Poimboeuf wrote:
> > > > +			if (insn->jump_dest == fake_jump) {
> > > > +				WARN_FUNC("jump inside alternative for _static_cpu_has()",
> > > > +					  insn->sec, insn->offset);
> > > > +			}
> > > 
> > > The error message doesn't seem to match the condition, so I'm not sure
> > > which one you're trying to check, or why.
> > > 
> > > IIRC, 'insn->jump_dest == fake_jump' means we reached the end of the
> > > alternative code block without hitting a jump.
> > > 
> > > But based on the loop exit condition, I don't think it's ever possible
> > > for insn->jump_dest to ever point to the fake_jump at the end.
> > 
> > Oof, now what was I thinking again.. So that fake_jump is inserted at
> > the end of the alternative and jumps to the code after where the
> > alternative will be patched in to simulate the code flow.
> > 
> > If there is a jump inside the alternative that jumps to the end, it's
> > destination will be set to the fake jump, we have this clause for that:
> > 
> > 	dest_off = insn->offset + insn->len + insn->immediate;
> > 	if (dest_off == special_alt->new_off + special_alt->new_len)
> > 		insn->jump_dest = fake_jump;
> > 
> > if that happens for static_cpu_has(), bad things happened.
> > 
> > So the only way for a jump to have fake_jump as destination is if the
> > jump is inside the alternative (but to the end) and we must assert this
> > didn't happen.
> > 
> > Unlikely, yes, but I figured we want to know about it if it ever does
> > happen.

So the case you're worried about, is it an unconditional jump?  As that
would be the only possibility based on the other warning.

-- 
Josh

Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()

[Peter Zijlstra]

On Wed, Jan 17, 2018 at 08:30:21AM -0600, Josh Poimboeuf wrote:
> On Wed, Jan 17, 2018 at 08:27:59AM -0600, Josh Poimboeuf wrote:
> > > > > +			if (insn->jump_dest == fake_jump) {
> > > > > +				WARN_FUNC("jump inside alternative for _static_cpu_has()",
> > > > > +					  insn->sec, insn->offset);
> > > > > +			}
> > > > 
> > > > The error message doesn't seem to match the condition, so I'm not sure
> > > > which one you're trying to check, or why.
> > > > 
> > > > IIRC, 'insn->jump_dest == fake_jump' means we reached the end of the
> > > > alternative code block without hitting a jump.
> > > > 
> > > > But based on the loop exit condition, I don't think it's ever possible
> > > > for insn->jump_dest to ever point to the fake_jump at the end.
> > > 
> > > Oof, now what was I thinking again.. So that fake_jump is inserted at
> > > the end of the alternative and jumps to the code after where the
> > > alternative will be patched in to simulate the code flow.
> > > 
> > > If there is a jump inside the alternative that jumps to the end, it's
> > > destination will be set to the fake jump, we have this clause for that:
> > > 
> > > 	dest_off = insn->offset + insn->len + insn->immediate;
> > > 	if (dest_off == special_alt->new_off + special_alt->new_len)
> > > 		insn->jump_dest = fake_jump;
> > > 
> > > if that happens for static_cpu_has(), bad things happened.
> > > 
> > > So the only way for a jump to have fake_jump as destination is if the
> > > jump is inside the alternative (but to the end) and we must assert this
> > > didn't happen.
> > > 
> > > Unlikely, yes, but I figured we want to know about it if it ever does
> > > happen.
> 
> So the case you're worried about, is it an unconditional jump?  As that
> would be the only possibility based on the other warning.

Right, the code up to that point would allow (if something really weird
happened) to have fake_jump as destination there. We want to flag if
that happens because bad.

[PATCH v2 07/10] x86/jump_label: Implement arch_static_assert()

[Peter Zijlstra]

[-- Attachment #1: peterz-x86-asm-goto-assert.patch --]
[-- Type: text/plain, Size: 1422 bytes --]

Implement the static (branch) assertion. It simply emits the address
of the next instruction into a special section which objtool will read
and validate against either __jump_table or .altinstructions.

Use like:

	if (static_branch_likely(_key)) {
		arch_static_assert();
		/* do stuff */
	}

Or

	if (static_cpu_has(_feat)) {
		arch_static_assert();
		/* do stuff */
	}

Cc: Borislav Petkov <bp@alien8.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/jump_label.h |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -62,6 +62,29 @@ static __always_inline bool arch_static_
 	return true;
 }
 
+/*
+ * Annotation for objtool; asserts that the previous instruction is the
+ * jump_label patch site. Or rather, that the next instruction is a static
+ * branch target.
+ *
+ * Use like:
+ *
+ *	if (static_branch_likely(key)) {
+ *		arch_static_assert();
+ *		do_code();
+ *	}
+ *
+ * Also works with static_cpu_has().
+ */
+static __always_inline void arch_static_assert(void)
+{
+	asm volatile ("1:\n\t"
+		      ".pushsection .discard.jump_assert \n\t"
+		      _ASM_ALIGN  "\n\t"
+		      _ASM_PTR "1b \n\t"
+		      ".popsection \n\t");
+}
+
 #ifdef CONFIG_X86_64
 typedef u64 jump_label_t;
 #else

Re: [PATCH v2 07/10] x86/jump_label: Implement arch_static_assert()

[Borislav Petkov]

On Tue, Jan 16, 2018 at 03:28:32PM +0100, Peter Zijlstra wrote:
> Implement the static (branch) assertion. It simply emits the address
> of the next instruction into a special section which objtool will read
> and validate against either __jump_table or .altinstructions.
> 
> Use like:
> 
> 	if (static_branch_likely(_key)) {
> 		arch_static_assert();
> 		/* do stuff */
> 	}
> 
> Or
> 
> 	if (static_cpu_has(_feat)) {
> 		arch_static_assert();

It looks to me like the asserts can be moved into the macros and the
assertion checking can happen by default ...?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v2 07/10] x86/jump_label: Implement arch_static_assert()

[Peter Zijlstra]

On Thu, Jan 18, 2018 at 02:33:22PM +0100, Borislav Petkov wrote:
> On Tue, Jan 16, 2018 at 03:28:32PM +0100, Peter Zijlstra wrote:
> > Implement the static (branch) assertion. It simply emits the address
> > of the next instruction into a special section which objtool will read
> > and validate against either __jump_table or .altinstructions.
> > 
> > Use like:
> > 
> > 	if (static_branch_likely(_key)) {
> > 		arch_static_assert();
> > 		/* do stuff */
> > 	}
> > 
> > Or
> > 
> > 	if (static_cpu_has(_feat)) {
> > 		arch_static_assert();
> 
> It looks to me like the asserts can be moved into the macros and the
> assertion checking can happen by default ...?

Right until the point where someone writes:

	if (static_cpu_has(_feat) || ponies)

at which point it, rightfully, unconditionally asserts.

Not sure someone does that with static_cpu_has(), but there's
static_branch*() users that do exactly that.

[PATCH v2 08/10] objtool: Add retpoline validation

[Peter Zijlstra]

[-- Attachment #1: peterz-objtool-indirect.patch --]
[-- Type: text/plain, Size: 7107 bytes --]

David requested a objtool validation pass for RETPOLINE enabled
builds, where it validates no unannotated dynamic jumps or calls are
left.

Add an additional .discard.retpoline_safe section to allow annotating
the few dynamic sites that are required and safe.

Requested-by: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 scripts/Makefile.build        |    4 +
 tools/objtool/builtin-check.c |    5 +-
 tools/objtool/builtin-orc.c   |    4 -
 tools/objtool/check.c         |   92 +++++++++++++++++++++++++++++++++++++++---
 tools/objtool/check.h         |    4 -
 5 files changed, 98 insertions(+), 11 deletions(-)

--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -264,6 +264,10 @@ objtool_args += --no-unreachable
 else
 objtool_args += $(call cc-ifversion, -lt, 0405, --no-unreachable)
 endif
+ifdef CONFIG_RETPOLINE
+objtool_args += --retpoline
+endif
+
 
 # 'OBJECT_FILES_NON_STANDARD := y': skip objtool checking for a directory
 # 'OBJECT_FILES_NON_STANDARD_foo.o := 'y': skip objtool checking for a file
--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -29,7 +29,7 @@
 #include "builtin.h"
 #include "check.h"
 
-bool no_fp, no_unreachable;
+bool no_fp, no_unreachable, retpoline;
 
 static const char * const check_usage[] = {
 	"objtool check [<options>] file.o",
@@ -39,6 +39,7 @@ static const char * const check_usage[]
 const struct option check_options[] = {
 	OPT_BOOLEAN('f', "no-fp", &no_fp, "Skip frame pointer validation"),
 	OPT_BOOLEAN('u', "no-unreachable", &no_unreachable, "Skip 'unreachable instruction' warnings"),
+	OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"),
 	OPT_END(),
 };
 
@@ -53,5 +54,5 @@ int cmd_check(int argc, const char **arg
 
 	objname = argv[0];
 
-	return check(objname, no_fp, no_unreachable, false);
+	return check(objname, no_fp, no_unreachable, retpoline, false);
 }
--- a/tools/objtool/builtin-orc.c
+++ b/tools/objtool/builtin-orc.c
@@ -37,7 +37,7 @@ static const char *orc_usage[] = {
 };
 
 extern const struct option check_options[];
-extern bool no_fp, no_unreachable;
+extern bool no_fp, no_unreachable, retpoline;
 
 int cmd_orc(int argc, const char **argv)
 {
@@ -54,7 +54,7 @@ int cmd_orc(int argc, const char **argv)
 
 		objname = argv[0];
 
-		return check(objname, no_fp, no_unreachable, true);
+		return check(objname, no_fp, no_unreachable, retpoline, true);
 	}
 
 	if (!strcmp(argv[0], "dump")) {
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -33,7 +33,7 @@ struct alternative {
 };
 
 const char *objname;
-static bool no_fp;
+static bool no_fp, retpoline;
 struct cfi_state initial_func_cfi;
 
 struct instruction *find_insn(struct objtool_file *file,
@@ -496,6 +496,7 @@ static int add_jump_destinations(struct
 			 * disguise, so convert them accordingly.
 			 */
 			insn->type = INSN_JUMP_DYNAMIC;
+			insn->retpoline_safe = true;
 			continue;
 		} else {
 			/* sibling call */
@@ -548,13 +549,11 @@ static int add_call_destinations(struct
 			 * normal for a function to call within itself.  So
 			 * disable this warning for now.
 			 */
-#if 0
-			if (!insn->call_dest) {
+			if (retpoline && !insn->call_dest) {
 				WARN_FUNC("can't find call dest symbol at offset 0x%lx",
 					  insn->sec, insn->offset, dest_off);
 				return -1;
 			}
-#endif
 		} else if (rela->sym->type == STT_SECTION) {
 			insn->call_dest = find_symbol_by_offset(rela->sym->sec,
 								rela->addend+4);
@@ -1150,6 +1149,54 @@ static int assert_static_jumps(struct ob
 	return 0;
 }
 
+static int read_retpoline_hints(struct objtool_file *file)
+{
+	struct section *sec, *relasec;
+	struct instruction *insn;
+	struct rela *rela;
+	int i;
+
+	sec = find_section_by_name(file->elf, ".discard.retpoline_safe");
+	if (!sec)
+		return 0;
+
+	relasec = sec->rela;
+	if (!relasec) {
+		WARN("missing .rela.discard.retpoline_safe section");
+		return -1;
+	}
+
+	if (sec->len % sizeof(unsigned long)) {
+		WARN("retpoline_safe size mismatch: %d %ld", sec->len, sizeof(unsigned long));
+		return -1;
+	}
+
+	for (i = 0; i < sec->len / sizeof(unsigned long); i++) {
+		rela = find_rela_by_dest(sec, i * sizeof(unsigned long));
+		if (!rela) {
+			WARN("can't find rela for retpoline_safe[%d]", i);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("can't find insn for retpoline_safe[%d]", i);
+			return -1;
+		}
+
+		if (insn->type != INSN_JUMP_DYNAMIC &&
+		    insn->type != INSN_CALL_DYNAMIC) {
+			WARN_FUNC("retpoline_safe hint not a dynamic jump/call",
+				  insn->sec, insn->offset);
+			return -1;
+		}
+
+		insn->retpoline_safe = true;
+	}
+
+	return 0;
+}
+
 static int decode_sections(struct objtool_file *file)
 {
 	int ret;
@@ -1188,6 +1235,10 @@ static int decode_sections(struct objtoo
 	if (ret)
 		return ret;
 
+	ret = read_retpoline_hints(file);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
@@ -1931,6 +1982,29 @@ static int validate_unwind_hints(struct
 	return warnings;
 }
 
+static int validate_retpoline(struct objtool_file *file)
+{
+	struct instruction *insn;
+	int warnings = 0;
+
+	for_each_insn(file, insn) {
+		if (insn->type != INSN_JUMP_DYNAMIC &&
+		    insn->type != INSN_CALL_DYNAMIC)
+			continue;
+
+		if (insn->retpoline_safe)
+			continue;
+
+		WARN_FUNC("dynamic %s found in RETPOLINE build",
+			  insn->sec, insn->offset,
+			  insn->type == INSN_JUMP_DYNAMIC ? "jump" : "call");
+
+		warnings++;
+	}
+
+	return warnings;
+}
+
 static bool is_kasan_insn(struct instruction *insn)
 {
 	return (insn->type == INSN_CALL &&
@@ -2056,13 +2130,14 @@ static void cleanup(struct objtool_file
 	elf_close(file->elf);
 }
 
-int check(const char *_objname, bool _no_fp, bool no_unreachable, bool orc)
+int check(const char *_objname, bool _no_fp, bool no_unreachable, bool _retpoline, bool orc)
 {
 	struct objtool_file file;
 	int ret, warnings = 0;
 
 	objname = _objname;
 	no_fp = _no_fp;
+	retpoline = _retpoline;
 
 	file.elf = elf_open(objname, orc ? O_RDWR : O_RDONLY);
 	if (!file.elf)
@@ -2090,6 +2165,13 @@ int check(const char *_objname, bool _no
 	if (list_empty(&file.insn_list))
 		goto out;
 
+	if (retpoline) {
+		ret = validate_retpoline(&file);
+		if (ret < 0)
+			return ret;
+		warnings += ret;
+	}
+
 	ret = validate_functions(&file);
 	if (ret < 0)
 		goto out;
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -45,7 +45,7 @@ struct instruction {
 	unsigned char type;
 	unsigned long immediate;
 	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
-	bool static_jump_dest;
+	bool static_jump_dest, retpoline_safe;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct list_head alts;
@@ -63,7 +63,7 @@ struct objtool_file {
 	bool ignore_unreachables, c_file, hints;
 };
 
-int check(const char *objname, bool no_fp, bool no_unreachable, bool orc);
+int check(const char *objname, bool no_fp, bool no_unreachable, bool retpoline, bool orc);
 
 struct instruction *find_insn(struct objtool_file *file,
 			      struct section *sec, unsigned long offset);

[PATCH v2 09/10] x86: Annotate dynamic jump in head_64.S

[Peter Zijlstra]

[-- Attachment #1: peterz-objtool-retpoline-annotate.patch --]
[-- Type: text/plain, Size: 601 bytes --]

The objtool retpoline validation found this indirect jump. Seeing how
its on CPU bringup before we run userspace it should be safe, annotate
it.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/head_64.S |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -134,7 +134,10 @@ ENTRY(secondary_startup_64)
 
 	/* Ensure I am executing from virtual addresses */
 	movq	$1f, %rax
-	jmp	*%rax
+2:	jmp	*%rax
+	.pushsection .discard.retpoline_safe
+	.quad 2b
+	.popsection
 1:
 	UNWIND_HINT_EMPTY
 

[PATCH v2 10/10] objtool: More complex static jump implementation

[Peter Zijlstra]

[-- Attachment #1: peterz-objtool-fancy.patch --]
[-- Type: text/plain, Size: 3272 bytes --]

When using something like:

  -#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x]))
  +#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x]) && \
  +			(arch_static_assert(), true))

we get an objtool assertion fail like:

kernel/sched/fair.o: warning: objtool: hrtick_update()+0xd: static assert FAIL

where:

0000000000001140 <hrtick_update>:
    1140:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
    1145:       c3                      retq
    1146:       48 8b b7 30 09 00 00    mov    0x930(%rdi),%rsi
    114d:       8b 87 d8 09 00 00       mov    0x9d8(%rdi),%eax
    1153:       48 0f a3 05 00 00 00    bt     %rax,0x0(%rip)        # 115b <hrtick_update+0x1b>
    115a:       00
                        1157: R_X86_64_PC32     __cpu_active_mask-0x4

and:

RELOCATION RECORDS FOR [__jump_table]:
0000000000000150 R_X86_64_64       .text+0x0000000000001140
0000000000000158 R_X86_64_64       .text+0x0000000000001146

RELOCATION RECORDS FOR [.discard.jump_assert]:
0000000000000028 R_X86_64_64       .text+0x000000000000114d

IOW, GCC managed to place the assertion 1 instruction _after_ the
static jump target.

So while the code generation is fine, the assertion gets placed wrong.
We can 'fix' this by not only considering the immediate static jump
locations but also all the unconditional code after it, terminating
the basic block on any unconditional instruction or branch entry
point.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   40 ++++++++++++++++++++++++++++++++++++++++
 tools/objtool/check.h |    2 +-
 2 files changed, 41 insertions(+), 1 deletion(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -520,6 +520,8 @@ static int add_jump_destinations(struct
 				  dest_off);
 			return -1;
 		}
+
+		insn->jump_dest->branch_target = true;
 	}
 
 	return 0;
@@ -1197,6 +1199,40 @@ static int read_retpoline_hints(struct o
 	return 0;
 }
 
+static int grow_static_blocks(struct objtool_file *file)
+{
+	struct instruction *insn;
+	bool static_block = false;
+
+	for_each_insn(file, insn) {
+		if (!static_block && !insn->static_jump_dest)
+			continue;
+
+		if (insn->static_jump_dest) {
+			static_block = true;
+			continue;
+		}
+
+		if (insn->branch_target) {
+			static_block = false;
+			continue;
+		} else switch (insn->type) {
+		case INSN_JUMP_CONDITIONAL:
+		case INSN_JUMP_DYNAMIC:
+		case INSN_CALL:
+		case INSN_CALL_DYNAMIC:
+		case INSN_RETURN:
+		case INSN_BUG:
+			static_block = false;
+			continue;
+		}
+
+		insn->static_jump_dest = static_block;
+	}
+
+	return 0;
+}
+
 static int decode_sections(struct objtool_file *file)
 {
 	int ret;
@@ -1239,6 +1275,10 @@ static int decode_sections(struct objtoo
 	if (ret)
 		return ret;
 
+	ret = grow_static_blocks(file);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -45,7 +45,7 @@ struct instruction {
 	unsigned char type;
 	unsigned long immediate;
 	bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts;
-	bool static_jump_dest, retpoline_safe;
+	bool static_jump_dest, retpoline_safe, branch_target;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct list_head alts;

Re: [PATCH v2 10/10] objtool: More complex static jump implementation

[Peter Zijlstra]

On Tue, Jan 16, 2018 at 03:28:35PM +0100, Peter Zijlstra wrote:
> +static int grow_static_blocks(struct objtool_file *file)
> +{
> +	struct instruction *insn;
> +	bool static_block = false;
> +
> +	for_each_insn(file, insn) {
> +		if (!static_block && !insn->static_jump_dest)
> +			continue;
> +
> +		if (insn->static_jump_dest) {
> +			static_block = true;
> +			continue;
> +		}
> +
> +		if (insn->branch_target) {
> +			static_block = false;
> +			continue;
> +		} else switch (insn->type) {
> +		case INSN_JUMP_CONDITIONAL:
> +		case INSN_JUMP_DYNAMIC:

Hmm, I think I also should have added INSN_JUMP_UNCONDITIONAL here,
because the for_each_insn() iteration simply iterates through the entire
file in linear order, it doesn't actually _follow_ the jumps.

So this would result in code after the unconditional jump also being
marked static (although very likely it ends up being a branch target and
thus stops it through that).

/me updates.

> +		case INSN_CALL:
> +		case INSN_CALL_DYNAMIC:
> +		case INSN_RETURN:
> +		case INSN_BUG:
> +			static_block = false;
> +			continue;
> +		}
> +
> +		insn->static_jump_dest = static_block;
> +	}
> +
> +	return 0;
> +}

Re: [PATCH v2 10/10] objtool: More complex static jump implementation

[Josh Poimboeuf]

On Tue, Jan 16, 2018 at 03:28:35PM +0100, Peter Zijlstra wrote:
> When using something like:
> 
>   -#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x]))
>   +#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x]) && \
>   +			(arch_static_assert(), true))
> 
> we get an objtool assertion fail like:
> 
> kernel/sched/fair.o: warning: objtool: hrtick_update()+0xd: static assert FAIL
> 
> where:
> 
> 0000000000001140 <hrtick_update>:
>     1140:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
>     1145:       c3                      retq
>     1146:       48 8b b7 30 09 00 00    mov    0x930(%rdi),%rsi
>     114d:       8b 87 d8 09 00 00       mov    0x9d8(%rdi),%eax
>     1153:       48 0f a3 05 00 00 00    bt     %rax,0x0(%rip)        # 115b <hrtick_update+0x1b>
>     115a:       00
>                         1157: R_X86_64_PC32     __cpu_active_mask-0x4
> 
> and:
> 
> RELOCATION RECORDS FOR [__jump_table]:
> 0000000000000150 R_X86_64_64       .text+0x0000000000001140
> 0000000000000158 R_X86_64_64       .text+0x0000000000001146
> 
> RELOCATION RECORDS FOR [.discard.jump_assert]:
> 0000000000000028 R_X86_64_64       .text+0x000000000000114d
> 
> IOW, GCC managed to place the assertion 1 instruction _after_ the
> static jump target.
> 
> So while the code generation is fine, the assertion gets placed wrong.
> We can 'fix' this by not only considering the immediate static jump
> locations but also all the unconditional code after it, terminating
> the basic block on any unconditional instruction or branch entry
> point.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

This is pretty similar to something I've been wanting to do, which is to
track all basic blocks.  But this is fine enough for now.

One nit, can you rename "branch_target" to "jump_dest" for consistency
with the existing naming?

-- 
Josh

Re: [PATCH v2 10/10] objtool: More complex static jump implementation

[Peter Zijlstra]

On Tue, Jan 16, 2018 at 09:05:31PM -0600, Josh Poimboeuf wrote:
> On Tue, Jan 16, 2018 at 03:28:35PM +0100, Peter Zijlstra wrote:
> > When using something like:
> > 
> >   -#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x]))
> >   +#define sched_feat(x) (static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x]) && \
> >   +			(arch_static_assert(), true))
> > 
> > we get an objtool assertion fail like:
> > 
> > kernel/sched/fair.o: warning: objtool: hrtick_update()+0xd: static assert FAIL
> > 
> > where:
> > 
> > 0000000000001140 <hrtick_update>:
> >     1140:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
> >     1145:       c3                      retq
> >     1146:       48 8b b7 30 09 00 00    mov    0x930(%rdi),%rsi
> >     114d:       8b 87 d8 09 00 00       mov    0x9d8(%rdi),%eax
> >     1153:       48 0f a3 05 00 00 00    bt     %rax,0x0(%rip)        # 115b <hrtick_update+0x1b>
> >     115a:       00
> >                         1157: R_X86_64_PC32     __cpu_active_mask-0x4
> > 
> > and:
> > 
> > RELOCATION RECORDS FOR [__jump_table]:
> > 0000000000000150 R_X86_64_64       .text+0x0000000000001140
> > 0000000000000158 R_X86_64_64       .text+0x0000000000001146
> > 
> > RELOCATION RECORDS FOR [.discard.jump_assert]:
> > 0000000000000028 R_X86_64_64       .text+0x000000000000114d
> > 
> > IOW, GCC managed to place the assertion 1 instruction _after_ the
> > static jump target.
> > 
> > So while the code generation is fine, the assertion gets placed wrong.
> > We can 'fix' this by not only considering the immediate static jump
> > locations but also all the unconditional code after it, terminating
> > the basic block on any unconditional instruction or branch entry
> > point.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> This is pretty similar to something I've been wanting to do, which is to
> track all basic blocks.  But this is fine enough for now.

Right, if we'd have that, we could just mark the entire block as
'static' and be done with it.

> One nit, can you rename "branch_target" to "jump_dest" for consistency
> with the existing naming?

Can't, insn->jump_dest already exists.

[PATCH v2 11/10] objtool: Even more complex static block checks

[Peter Zijlstra]

Subject: objtool: Even more complex static block checks
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Jan 16 20:17:01 CET 2018

I've observed GCC transform:

  f()
  {
	  if (!static_branch_unlikely())
		  return;

	  static_assert();
	  A;
  }

  g()
  {
	  f();
  }

Into:

  f()
  {
	  static_assert();
	  A;
  }

  g()
  {
	  if (static_branch_unlikely())
		  f();
  }

Which results in the assertion landing at f+0. The transformation is
valid and useful; it avoids a pointless CALL+RET sequence, so we'll
have to teach objtool how to deal with this.

Do this by marking all CALL destinations with static_call when called
from a static_block and non_static_call when called outside a
static_block. This allows us to identify functions called exclusively
from a static_block and start them with a static_block.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   77 ++++++++++++++++++++++++++++++++++++--------------
 tools/objtool/elf.h   |    1 
 2 files changed, 57 insertions(+), 21 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1199,36 +1199,71 @@ static int read_retpoline_hints(struct o
 	return 0;
 }
 
+static void __grow_static_block(struct instruction *insn, bool *state)
+{
+	if (!*state && !insn->static_jump_dest)
+		return;
+
+	if (insn->static_jump_dest) {
+		*state = true;
+		return;
+	}
+
+	if (insn->branch_target) {
+		*state = false;
+		return;
+
+	} else switch (insn->type) {
+	case INSN_JUMP_CONDITIONAL:
+	case INSN_JUMP_UNCONDITIONAL:
+	case INSN_JUMP_DYNAMIC:
+	case INSN_CALL_DYNAMIC:
+	case INSN_RETURN:
+	case INSN_BUG:
+		*state = false;
+		return;
+	}
+
+	insn->static_jump_dest = *state;
+}
+
 static int grow_static_blocks(struct objtool_file *file)
 {
-	struct instruction *insn;
 	bool static_block = false;
+	struct symbol *func, *tmp;
+	struct instruction *insn;
+	struct section *sec;
 
 	for_each_insn(file, insn) {
-		if (!static_block && !insn->static_jump_dest)
-			continue;
+		__grow_static_block(insn, &static_block);
 
-		if (insn->static_jump_dest) {
-			static_block = true;
-			continue;
-		}
+		if (insn->type == INSN_CALL) {
+			func = insn->call_dest;
 
-		if (insn->branch_target) {
-			static_block = false;
-			continue;
-		} else switch (insn->type) {
-		case INSN_JUMP_CONDITIONAL:
-		case INSN_JUMP_UNCONDITIONAL:
-		case INSN_JUMP_DYNAMIC:
-		case INSN_CALL:
-		case INSN_CALL_DYNAMIC:
-		case INSN_RETURN:
-		case INSN_BUG:
-			static_block = false;
-			continue;
+			if (static_block)
+				func->static_call = true;
+			else
+				func->non_static_call = true;
 		}
+	}
+
+	for_each_sec(file, sec) {
+		list_for_each_entry_safe(func, tmp, &sec->symbol_list, list) {
+			if (!func->static_call)
+				continue;
 
-		insn->static_jump_dest = static_block;
+			if (func->non_static_call)
+				continue;
+
+			/* static && !non_static -- only static callers */
+
+			static_block = true;
+			func_for_each_insn(file, func, insn) {
+				__grow_static_block(insn, &static_block);
+				if (!static_block)
+					break;
+			}
+		}
 	}
 
 	return 0;
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -61,6 +61,7 @@ struct symbol {
 	unsigned char bind, type;
 	unsigned long offset;
 	unsigned int len;
+	bool static_call, non_static_call;
 };
 
 struct rela {

Re: [PATCH v2 11/10] objtool: Even more complex static block checks

[Josh Poimboeuf]

On Tue, Jan 16, 2018 at 08:49:17PM +0100, Peter Zijlstra wrote:
> Subject: objtool: Even more complex static block checks
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue Jan 16 20:17:01 CET 2018
> 
> I've observed GCC transform:
> 
>   f()
>   {
> 	  if (!static_branch_unlikely())
> 		  return;
> 
> 	  static_assert();
> 	  A;
>   }
> 
>   g()
>   {
> 	  f();
>   }
> 
> Into:
> 
>   f()
>   {
> 	  static_assert();
> 	  A;
>   }
> 
>   g()
>   {
> 	  if (static_branch_unlikely())
> 		  f();
>   }
> 
> Which results in the assertion landing at f+0. The transformation is
> valid and useful; it avoids a pointless CALL+RET sequence, so we'll
> have to teach objtool how to deal with this.
> 
> Do this by marking all CALL destinations with static_call when called
> from a static_block and non_static_call when called outside a
> static_block. This allows us to identify functions called exclusively
> from a static_block and start them with a static_block.

Ew... where'd you place the assertion to trigger this?

It's late and my brain has already clocked out, so I'll need to revisit
this tomorrow.  But now I'm wondering if my basic block idea would be a
better way to solve this.

-- 
Josh

Re: [PATCH v2 11/10] objtool: Even more complex static block checks

[Peter Zijlstra]

On Tue, Jan 16, 2018 at 09:12:32PM -0600, Josh Poimboeuf wrote:
> On Tue, Jan 16, 2018 at 08:49:17PM +0100, Peter Zijlstra wrote:
> > Subject: objtool: Even more complex static block checks
> > From: Peter Zijlstra <peterz@infradead.org>
> > Date: Tue Jan 16 20:17:01 CET 2018
> > 
> > I've observed GCC transform:
> > 
> >   f()
> >   {
> > 	  if (!static_branch_unlikely())
> > 		  return;
> > 
> > 	  static_assert();
> > 	  A;
> >   }
> > 
> >   g()
> >   {
> > 	  f();
> >   }
> > 
> > Into:
> > 
> >   f()
> >   {
> > 	  static_assert();
> > 	  A;
> >   }
> > 
> >   g()
> >   {
> > 	  if (static_branch_unlikely())
> > 		  f();
> >   }
> > 
> > Which results in the assertion landing at f+0. The transformation is
> > valid and useful; it avoids a pointless CALL+RET sequence, so we'll
> > have to teach objtool how to deal with this.
> > 
> > Do this by marking all CALL destinations with static_call when called
> > from a static_block and non_static_call when called outside a
> > static_block. This allows us to identify functions called exclusively
> > from a static_block and start them with a static_block.
> 
> Ew... where'd you place the assertion to trigger this?

Its the patch I pastebin'ed you earlier, also see below.

> It's late and my brain has already clocked out, so I'll need to revisit
> this tomorrow.  But now I'm wondering if my basic block idea would be a
> better way to solve this.

I would think basic-blocks are inside functions, and this patch goes
across functions, something you'd still need even if you had basic
blocks.

Also, basic blocks are non-trivial because they can overlap. I've
implemented something like that before for perf, see commit:

  70fbe0574558 ("perf annotate: Add branch stack / basic block")

We could probably lift that code fairly easily.

---
Subject: jump_label: Add static assertion to every static_branch
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Jan 16 15:27:36 CET 2018

for testing.. not sure we wants this in general

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/jump_label.h |    1 +
 include/linux/jump_label.h        |    8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -76,6 +76,7 @@ static __always_inline bool arch_static_
  *
  * Also works with static_cpu_has().
  */
+#define arch_static_assert arch_static_assert
 static __always_inline void arch_static_assert(void)
 {
 	asm volatile ("1:\n\t"
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -323,6 +323,10 @@ extern bool ____wrong_branch_error(void)
 
 #ifdef HAVE_JUMP_LABEL
 
+#ifndef arch_static_assert
+#define arch_static_assert (void)
+#endif
+
 /*
  * Combine the right initial value (type) with the right branch order
  * to generate the desired result.
@@ -388,7 +392,7 @@ extern bool ____wrong_branch_error(void)
 		branch = !arch_static_branch_jump(&(x)->key, true);		\
 	else									\
 		branch = ____wrong_branch_error();				\
-	branch;									\
+	branch && (arch_static_assert(), true);					\
 })
 
 #define static_branch_unlikely(x)						\
@@ -400,7 +404,7 @@ extern bool ____wrong_branch_error(void)
 		branch = arch_static_branch(&(x)->key, false);			\
 	else									\
 		branch = ____wrong_branch_error();				\
-	branch;									\
+	branch && (arch_static_assert(), true);					\
 })
 
 #else /* !HAVE_JUMP_LABEL */
> Josh

Re: [PATCH v2 11/10] objtool: Even more complex static block checks

[Josh Poimboeuf]

On Wed, Jan 17, 2018 at 09:13:09AM +0100, Peter Zijlstra wrote:
> On Tue, Jan 16, 2018 at 09:12:32PM -0600, Josh Poimboeuf wrote:
> > On Tue, Jan 16, 2018 at 08:49:17PM +0100, Peter Zijlstra wrote:
> > > Subject: objtool: Even more complex static block checks
> > > From: Peter Zijlstra <peterz@infradead.org>
> > > Date: Tue Jan 16 20:17:01 CET 2018
> > > 
> > > I've observed GCC transform:
> > > 
> > >   f()
> > >   {
> > > 	  if (!static_branch_unlikely())
> > > 		  return;
> > > 
> > > 	  static_assert();
> > > 	  A;
> > >   }
> > > 
> > >   g()
> > >   {
> > > 	  f();
> > >   }
> > > 
> > > Into:
> > > 
> > >   f()
> > >   {
> > > 	  static_assert();
> > > 	  A;
> > >   }
> > > 
> > >   g()
> > >   {
> > > 	  if (static_branch_unlikely())
> > > 		  f();
> > >   }
> > > 
> > > Which results in the assertion landing at f+0. The transformation is
> > > valid and useful; it avoids a pointless CALL+RET sequence, so we'll
> > > have to teach objtool how to deal with this.
> > > 
> > > Do this by marking all CALL destinations with static_call when called
> > > from a static_block and non_static_call when called outside a
> > > static_block. This allows us to identify functions called exclusively
> > > from a static_block and start them with a static_block.
> > 
> > Ew... where'd you place the assertion to trigger this?
> 
> Its the patch I pastebin'ed you earlier, also see below.

Ah, I remembered you mentioning the problem, just forgot you showed me
the patch.

> > It's late and my brain has already clocked out, so I'll need to revisit
> > this tomorrow.  But now I'm wondering if my basic block idea would be a
> > better way to solve this.
> 
> I would think basic-blocks are inside functions, and this patch goes
> across functions, something you'd still need even if you had basic
> blocks.

Right, but I was thinking the patch would be a lot simpler with basic
blocks.

> Also, basic blocks are non-trivial because they can overlap.

Hm, I thought a basic block only has one entry point and one exit point.
How could they overlap?

-- 
Josh