[PATCH v4 00/13] objtool: vmlinux.o and moinstr validation

[PATCH v4 00/13] objtool: vmlinux.o and moinstr validation

[Peter Zijlstra]

Hi all,

This lacks the first 17 patches of the v3 posting which are en-route to
tip/core/objtool and should show up there somewhere later this evening.

As should be familiar by now; these patches implement the noinstr
(no-instrument) validation in objtool as requested by Thomas, to ensure
critical code (entry for now, idle later) run no unexpected code.

Functions are marked with: noinstr, which implies notrace, noinline and sticks
things in the .noinstr.text section. Such functions can then use instr_begin()
and instr_end() to allow calls to code outside of this section in sanctioned
areas.

[PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

There is a special case in the UNWIND_HINT_RESTORE code. When, upon
looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
the instruction hasn't been visited yet, it normally issues a WARN,
except when this HINT_SAVE instruction is the first instruction of
this branch.

This special case is of dubious correctness and is certainly unused
(verified with an allmodconfig build), the two sites that employ
UNWIND_HINT_SAVE/RESTORE (sync_core() and ftrace_regs_caller()) have
the SAVE on unconditional instructions at the start of the function.
It is therefore impossible for the save_insn not to have been visited
when we do hit the RESTORE.

Remove this.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c |   15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2040,15 +2040,14 @@ static int validate_return(struct symbol
  * tools/objtool/Documentation/stack-validation.txt.
  */
 static int validate_branch(struct objtool_file *file, struct symbol *func,
-			   struct instruction *first, struct insn_state state)
+			   struct instruction *insn, struct insn_state state)
 {
+	struct instruction *next_insn;
 	struct alternative *alt;
-	struct instruction *insn, *next_insn;
 	struct section *sec;
 	u8 visited;
 	int ret;
 
-	insn = first;
 	sec = insn->sec;
 
 	if (insn->alt_group && list_empty(&insn->alts)) {
@@ -2101,16 +2100,6 @@ static int validate_branch(struct objtoo
 				}
 
 				if (!save_insn->visited) {
-					/*
-					 * Oops, no state to copy yet.
-					 * Hopefully we can reach this
-					 * instruction from another branch
-					 * after the save insn has been
-					 * visited.
-					 */
-					if (insn == first)
-						return 0;
-
 					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
 						  sec, insn->offset);
 					return 1;

[PATCH v4 02/13] objtool: Factor out CFI hints

[Peter Zijlstra]

Move the application of CFI hints into it's own function.
No functional changes intended.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c |   67 ++++++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 29 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2033,6 +2033,41 @@ static int validate_return(struct symbol
 	return 0;
 }
 
+static int apply_insn_hint(struct objtool_file *file, struct section *sec,
+			   struct symbol *func, struct instruction *insn,
+			   struct insn_state *state)
+{
+	if (insn->restore) {
+		struct instruction *save_insn, *i;
+
+		i = insn;
+		save_insn = NULL;
+		sym_for_each_insn_continue_reverse(file, func, i) {
+			if (i->save) {
+				save_insn = i;
+				break;
+			}
+		}
+
+		if (!save_insn) {
+			WARN_FUNC("no corresponding CFI save for CFI restore",
+				  sec, insn->offset);
+			return 1;
+		}
+
+		if (!save_insn->visited) {
+			WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
+				  sec, insn->offset);
+			return 1;
+		}
+
+		insn->state = save_insn->state;
+	}
+
+	state = insn->state;
+	return 0;
+}
+
 /*
  * Follow the branch starting at the given instruction, and recursively follow
  * any other branches (jumps).  Meanwhile, track the frame pointer state at
@@ -2081,35 +2116,9 @@ static int validate_branch(struct objtoo
 		}
 
 		if (insn->hint) {
-			if (insn->restore) {
-				struct instruction *save_insn, *i;
-
-				i = insn;
-				save_insn = NULL;
-				sym_for_each_insn_continue_reverse(file, func, i) {
-					if (i->save) {
-						save_insn = i;
-						break;
-					}
-				}
-
-				if (!save_insn) {
-					WARN_FUNC("no corresponding CFI save for CFI restore",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				if (!save_insn->visited) {
-					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				insn->state = save_insn->state;
-			}
-
-			state = insn->state;
-
+			ret = apply_insn_hint(file, sec, func, insn, &state);
+			if (ret)
+				return ret;
 		} else
 			insn->state = state;
 

[PATCH v4 03/13] objtool: Rename struct cfi_state

[Peter Zijlstra]

There's going to be a new struct cfi_state, rename this one to make
place.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/arch.h            |    2 +-
 tools/objtool/arch/x86/decode.c |    2 +-
 tools/objtool/cfi.h             |    2 +-
 tools/objtool/check.c           |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -66,7 +66,7 @@ struct stack_op {
 	struct op_src src;
 };
 
-void arch_initial_func_cfi_state(struct cfi_state *state);
+void arch_initial_func_cfi_state(struct cfi_init_state *state);
 
 int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned long offset, unsigned int maxlen,
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -480,7 +480,7 @@ int arch_decode_instruction(struct elf *
 	return 0;
 }
 
-void arch_initial_func_cfi_state(struct cfi_state *state)
+void arch_initial_func_cfi_state(struct cfi_init_state *state)
 {
 	int i;
 
--- a/tools/objtool/cfi.h
+++ b/tools/objtool/cfi.h
@@ -35,7 +35,7 @@ struct cfi_reg {
 	int offset;
 };
 
-struct cfi_state {
+struct cfi_init_state {
 	struct cfi_reg cfa;
 	struct cfi_reg regs[CFI_NUM_REGS];
 };
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -27,7 +27,7 @@ struct alternative {
 };
 
 const char *objname;
-struct cfi_state initial_func_cfi;
+struct cfi_init_state initial_func_cfi;
 
 struct instruction *find_insn(struct objtool_file *file,
 			      struct section *sec, unsigned long offset)

[PATCH v4 04/13] objtool: Fix !CFI insn_state propagation

[Peter Zijlstra]

Objtool keeps per instruction CFI state in struct insn_state and will
save/restore this where required. However, insn_state has grown some
!CFI state, and this must not be saved/restored (that would
loose/destroy state).

Fix this by moving the CFI specific parts of insn_state into struct
cfi_state.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/cfi.h     |   12 ++
 tools/objtool/check.c   |  270 ++++++++++++++++++++++++------------------------
 tools/objtool/check.h   |   13 --
 tools/objtool/orc_gen.c |    8 -
 4 files changed, 160 insertions(+), 143 deletions(-)

--- a/tools/objtool/cfi.h
+++ b/tools/objtool/cfi.h
@@ -36,8 +36,20 @@ struct cfi_reg {
 };
 
 struct cfi_init_state {
+	struct cfi_reg regs[CFI_NUM_REGS];
 	struct cfi_reg cfa;
+};
+
+struct cfi_state {
 	struct cfi_reg regs[CFI_NUM_REGS];
+	struct cfi_reg vals[CFI_NUM_REGS];
+	struct cfi_reg cfa;
+	int stack_size;
+	int drap_reg, drap_offset;
+	unsigned char type;
+	bool bp_scratch;
+	bool drap;
+	bool end;
 };
 
 #endif /* _OBJTOOL_CFI_H */
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -215,18 +215,23 @@ static bool dead_end_function(struct obj
 	return __dead_end_function(file, func, 0);
 }
 
-static void clear_insn_state(struct insn_state *state)
+static void init_cfi_state(struct cfi_state *cfi)
 {
 	int i;
 
-	memset(state, 0, sizeof(*state));
-	state->cfa.base = CFI_UNDEFINED;
 	for (i = 0; i < CFI_NUM_REGS; i++) {
-		state->regs[i].base = CFI_UNDEFINED;
-		state->vals[i].base = CFI_UNDEFINED;
+		cfi->regs[i].base = CFI_UNDEFINED;
+		cfi->vals[i].base = CFI_UNDEFINED;
 	}
-	state->drap_reg = CFI_UNDEFINED;
-	state->drap_offset = -1;
+	cfi->cfa.base = CFI_UNDEFINED;
+	cfi->drap_reg = CFI_UNDEFINED;
+	cfi->drap_offset = -1;
+}
+
+static void clear_insn_state(struct insn_state *state)
+{
+	memset(state, 0, sizeof(*state));
+	init_cfi_state(&state->cfi);
 }
 
 /*
@@ -260,7 +265,7 @@ static int decode_instructions(struct ob
 			}
 			memset(insn, 0, sizeof(*insn));
 			INIT_LIST_HEAD(&insn->alts);
-			clear_insn_state(&insn->state);
+			init_cfi_state(&insn->cfi);
 
 			insn->sec = sec;
 			insn->offset = offset;
@@ -769,7 +774,7 @@ static int handle_group_alt(struct objto
 		}
 		memset(fake_jump, 0, sizeof(*fake_jump));
 		INIT_LIST_HEAD(&fake_jump->alts);
-		clear_insn_state(&fake_jump->state);
+		init_cfi_state(&fake_jump->cfi);
 
 		fake_jump->sec = special_alt->new_sec;
 		fake_jump->offset = FAKE_JUMP_OFFSET;
@@ -1262,7 +1267,7 @@ static int read_unwind_hints(struct objt
 			return -1;
 		}
 
-		cfa = &insn->state.cfa;
+		cfa = &insn->cfi.cfa;
 
 		if (hint->type == UNWIND_HINT_TYPE_SAVE) {
 			insn->save = true;
@@ -1308,8 +1313,8 @@ static int read_unwind_hints(struct objt
 		}
 
 		cfa->offset = hint->sp_offset;
-		insn->state.type = hint->type;
-		insn->state.end = hint->end;
+		insn->cfi.type = hint->type;
+		insn->cfi.end = hint->end;
 	}
 
 	return 0;
@@ -1436,17 +1441,18 @@ static bool is_fentry_call(struct instru
 
 static bool has_modified_stack_frame(struct insn_state *state)
 {
+	struct cfi_state *cfi = &state->cfi;
 	int i;
 
-	if (state->cfa.base != initial_func_cfi.cfa.base ||
-	    state->cfa.offset != initial_func_cfi.cfa.offset ||
-	    state->stack_size != initial_func_cfi.cfa.offset ||
-	    state->drap)
+	if (cfi->cfa.base != initial_func_cfi.cfa.base ||
+	    cfi->cfa.offset != initial_func_cfi.cfa.offset ||
+	    cfi->stack_size != initial_func_cfi.cfa.offset ||
+	    cfi->drap)
 		return true;
 
 	for (i = 0; i < CFI_NUM_REGS; i++)
-		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
-		    state->regs[i].offset != initial_func_cfi.regs[i].offset)
+		if (cfi->regs[i].base != initial_func_cfi.regs[i].base ||
+		    cfi->regs[i].offset != initial_func_cfi.regs[i].offset)
 			return true;
 
 	return false;
@@ -1454,19 +1460,21 @@ static bool has_modified_stack_frame(str
 
 static bool has_valid_stack_frame(struct insn_state *state)
 {
-	if (state->cfa.base == CFI_BP && state->regs[CFI_BP].base == CFI_CFA &&
-	    state->regs[CFI_BP].offset == -16)
+	struct cfi_state *cfi = &state->cfi;
+
+	if (cfi->cfa.base == CFI_BP && cfi->regs[CFI_BP].base == CFI_CFA &&
+	    cfi->regs[CFI_BP].offset == -16)
 		return true;
 
-	if (state->drap && state->regs[CFI_BP].base == CFI_BP)
+	if (cfi->drap && cfi->regs[CFI_BP].base == CFI_BP)
 		return true;
 
 	return false;
 }
 
-static int update_insn_state_regs(struct instruction *insn, struct insn_state *state)
+static int update_cfi_state_regs(struct instruction *insn, struct cfi_state *cfi)
 {
-	struct cfi_reg *cfa = &state->cfa;
+	struct cfi_reg *cfa = &cfi->cfa;
 	struct stack_op *op = &insn->stack_op;
 
 	if (cfa->base != CFI_SP)
@@ -1488,20 +1496,19 @@ static int update_insn_state_regs(struct
 	return 0;
 }
 
-static void save_reg(struct insn_state *state, unsigned char reg, int base,
-		     int offset)
+static void save_reg(struct cfi_state *cfi, unsigned char reg, int base, int offset)
 {
 	if (arch_callee_saved_reg(reg) &&
-	    state->regs[reg].base == CFI_UNDEFINED) {
-		state->regs[reg].base = base;
-		state->regs[reg].offset = offset;
+	    cfi->regs[reg].base == CFI_UNDEFINED) {
+		cfi->regs[reg].base = base;
+		cfi->regs[reg].offset = offset;
 	}
 }
 
-static void restore_reg(struct insn_state *state, unsigned char reg)
+static void restore_reg(struct cfi_state *cfi, unsigned char reg)
 {
-	state->regs[reg].base = CFI_UNDEFINED;
-	state->regs[reg].offset = 0;
+	cfi->regs[reg].base = CFI_UNDEFINED;
+	cfi->regs[reg].offset = 0;
 }
 
 /*
@@ -1557,11 +1564,11 @@ static void restore_reg(struct insn_stat
  *   41 5d			pop    %r13
  *   c3				retq
  */
-static int update_insn_state(struct instruction *insn, struct insn_state *state)
+static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi)
 {
 	struct stack_op *op = &insn->stack_op;
-	struct cfi_reg *cfa = &state->cfa;
-	struct cfi_reg *regs = state->regs;
+	struct cfi_reg *cfa = &cfi->cfa;
+	struct cfi_reg *regs = cfi->regs;
 
 	/* stack operations don't make sense with an undefined CFA */
 	if (cfa->base == CFI_UNDEFINED) {
@@ -1572,8 +1579,8 @@ static int update_insn_state(struct inst
 		return 0;
 	}
 
-	if (state->type == ORC_TYPE_REGS || state->type == ORC_TYPE_REGS_IRET)
-		return update_insn_state_regs(insn, state);
+	if (cfi->type == ORC_TYPE_REGS || cfi->type == ORC_TYPE_REGS_IRET)
+		return update_cfi_state_regs(insn, cfi);
 
 	switch (op->dest.type) {
 
@@ -1588,16 +1595,16 @@ static int update_insn_state(struct inst
 
 				/* mov %rsp, %rbp */
 				cfa->base = op->dest.reg;
-				state->bp_scratch = false;
+				cfi->bp_scratch = false;
 			}
 
 			else if (op->src.reg == CFI_SP &&
-				 op->dest.reg == CFI_BP && state->drap) {
+				 op->dest.reg == CFI_BP && cfi->drap) {
 
 				/* drap: mov %rsp, %rbp */
 				regs[CFI_BP].base = CFI_BP;
-				regs[CFI_BP].offset = -state->stack_size;
-				state->bp_scratch = false;
+				regs[CFI_BP].offset = -cfi->stack_size;
+				cfi->bp_scratch = false;
 			}
 
 			else if (op->src.reg == CFI_SP && cfa->base == CFI_SP) {
@@ -1612,8 +1619,8 @@ static int update_insn_state(struct inst
 				 *   ...
 				 *   mov    %rax, %rsp
 				 */
-				state->vals[op->dest.reg].base = CFI_CFA;
-				state->vals[op->dest.reg].offset = -state->stack_size;
+				cfi->vals[op->dest.reg].base = CFI_CFA;
+				cfi->vals[op->dest.reg].offset = -cfi->stack_size;
 			}
 
 			else if (op->src.reg == CFI_BP && op->dest.reg == CFI_SP &&
@@ -1624,14 +1631,14 @@ static int update_insn_state(struct inst
 				 *
 				 * Restore the original stack pointer (Clang).
 				 */
-				state->stack_size = -state->regs[CFI_BP].offset;
+				cfi->stack_size = -cfi->regs[CFI_BP].offset;
 			}
 
 			else if (op->dest.reg == cfa->base) {
 
 				/* mov %reg, %rsp */
 				if (cfa->base == CFI_SP &&
-				    state->vals[op->src.reg].base == CFI_CFA) {
+				    cfi->vals[op->src.reg].base == CFI_CFA) {
 
 					/*
 					 * This is needed for the rare case
@@ -1641,8 +1648,8 @@ static int update_insn_state(struct inst
 					 *   ...
 					 *   mov    %rcx, %rsp
 					 */
-					cfa->offset = -state->vals[op->src.reg].offset;
-					state->stack_size = cfa->offset;
+					cfa->offset = -cfi->vals[op->src.reg].offset;
+					cfi->stack_size = cfa->offset;
 
 				} else {
 					cfa->base = CFI_UNDEFINED;
@@ -1656,7 +1663,7 @@ static int update_insn_state(struct inst
 			if (op->dest.reg == CFI_SP && op->src.reg == CFI_SP) {
 
 				/* add imm, %rsp */
-				state->stack_size -= op->src.offset;
+				cfi->stack_size -= op->src.offset;
 				if (cfa->base == CFI_SP)
 					cfa->offset -= op->src.offset;
 				break;
@@ -1665,14 +1672,14 @@ static int update_insn_state(struct inst
 			if (op->dest.reg == CFI_SP && op->src.reg == CFI_BP) {
 
 				/* lea disp(%rbp), %rsp */
-				state->stack_size = -(op->src.offset + regs[CFI_BP].offset);
+				cfi->stack_size = -(op->src.offset + regs[CFI_BP].offset);
 				break;
 			}
 
 			if (op->src.reg == CFI_SP && cfa->base == CFI_SP) {
 
 				/* drap: lea disp(%rsp), %drap */
-				state->drap_reg = op->dest.reg;
+				cfi->drap_reg = op->dest.reg;
 
 				/*
 				 * lea disp(%rsp), %reg
@@ -1684,25 +1691,25 @@ static int update_insn_state(struct inst
 				 *   ...
 				 *   mov    %rcx, %rsp
 				 */
-				state->vals[op->dest.reg].base = CFI_CFA;
-				state->vals[op->dest.reg].offset = \
-					-state->stack_size + op->src.offset;
+				cfi->vals[op->dest.reg].base = CFI_CFA;
+				cfi->vals[op->dest.reg].offset = \
+					-cfi->stack_size + op->src.offset;
 
 				break;
 			}
 
-			if (state->drap && op->dest.reg == CFI_SP &&
-			    op->src.reg == state->drap_reg) {
+			if (cfi->drap && op->dest.reg == CFI_SP &&
+			    op->src.reg == cfi->drap_reg) {
 
 				 /* drap: lea disp(%drap), %rsp */
 				cfa->base = CFI_SP;
-				cfa->offset = state->stack_size = -op->src.offset;
-				state->drap_reg = CFI_UNDEFINED;
-				state->drap = false;
+				cfa->offset = cfi->stack_size = -op->src.offset;
+				cfi->drap_reg = CFI_UNDEFINED;
+				cfi->drap = false;
 				break;
 			}
 
-			if (op->dest.reg == state->cfa.base) {
+			if (op->dest.reg == cfi->cfa.base) {
 				WARN_FUNC("unsupported stack register modification",
 					  insn->sec, insn->offset);
 				return -1;
@@ -1712,18 +1719,18 @@ static int update_insn_state(struct inst
 
 		case OP_SRC_AND:
 			if (op->dest.reg != CFI_SP ||
-			    (state->drap_reg != CFI_UNDEFINED && cfa->base != CFI_SP) ||
-			    (state->drap_reg == CFI_UNDEFINED && cfa->base != CFI_BP)) {
+			    (cfi->drap_reg != CFI_UNDEFINED && cfa->base != CFI_SP) ||
+			    (cfi->drap_reg == CFI_UNDEFINED && cfa->base != CFI_BP)) {
 				WARN_FUNC("unsupported stack pointer realignment",
 					  insn->sec, insn->offset);
 				return -1;
 			}
 
-			if (state->drap_reg != CFI_UNDEFINED) {
+			if (cfi->drap_reg != CFI_UNDEFINED) {
 				/* drap: and imm, %rsp */
-				cfa->base = state->drap_reg;
-				cfa->offset = state->stack_size = 0;
-				state->drap = true;
+				cfa->base = cfi->drap_reg;
+				cfa->offset = cfi->stack_size = 0;
+				cfi->drap = true;
 			}
 
 			/*
@@ -1735,57 +1742,57 @@ static int update_insn_state(struct inst
 
 		case OP_SRC_POP:
 		case OP_SRC_POPF:
-			if (!state->drap && op->dest.type == OP_DEST_REG &&
+			if (!cfi->drap && op->dest.type == OP_DEST_REG &&
 			    op->dest.reg == cfa->base) {
 
 				/* pop %rbp */
 				cfa->base = CFI_SP;
 			}
 
-			if (state->drap && cfa->base == CFI_BP_INDIRECT &&
+			if (cfi->drap && cfa->base == CFI_BP_INDIRECT &&
 			    op->dest.type == OP_DEST_REG &&
-			    op->dest.reg == state->drap_reg &&
-			    state->drap_offset == -state->stack_size) {
+			    op->dest.reg == cfi->drap_reg &&
+			    cfi->drap_offset == -cfi->stack_size) {
 
 				/* drap: pop %drap */
-				cfa->base = state->drap_reg;
+				cfa->base = cfi->drap_reg;
 				cfa->offset = 0;
-				state->drap_offset = -1;
+				cfi->drap_offset = -1;
 
-			} else if (regs[op->dest.reg].offset == -state->stack_size) {
+			} else if (regs[op->dest.reg].offset == -cfi->stack_size) {
 
 				/* pop %reg */
-				restore_reg(state, op->dest.reg);
+				restore_reg(cfi, op->dest.reg);
 			}
 
-			state->stack_size -= 8;
+			cfi->stack_size -= 8;
 			if (cfa->base == CFI_SP)
 				cfa->offset -= 8;
 
 			break;
 
 		case OP_SRC_REG_INDIRECT:
-			if (state->drap && op->src.reg == CFI_BP &&
-			    op->src.offset == state->drap_offset) {
+			if (cfi->drap && op->src.reg == CFI_BP &&
+			    op->src.offset == cfi->drap_offset) {
 
 				/* drap: mov disp(%rbp), %drap */
-				cfa->base = state->drap_reg;
+				cfa->base = cfi->drap_reg;
 				cfa->offset = 0;
-				state->drap_offset = -1;
+				cfi->drap_offset = -1;
 			}
 
-			if (state->drap && op->src.reg == CFI_BP &&
+			if (cfi->drap && op->src.reg == CFI_BP &&
 			    op->src.offset == regs[op->dest.reg].offset) {
 
 				/* drap: mov disp(%rbp), %reg */
-				restore_reg(state, op->dest.reg);
+				restore_reg(cfi, op->dest.reg);
 
 			} else if (op->src.reg == cfa->base &&
 			    op->src.offset == regs[op->dest.reg].offset + cfa->offset) {
 
 				/* mov disp(%rbp), %reg */
 				/* mov disp(%rsp), %reg */
-				restore_reg(state, op->dest.reg);
+				restore_reg(cfi, op->dest.reg);
 			}
 
 			break;
@@ -1800,78 +1807,78 @@ static int update_insn_state(struct inst
 
 	case OP_DEST_PUSH:
 	case OP_DEST_PUSHF:
-		state->stack_size += 8;
+		cfi->stack_size += 8;
 		if (cfa->base == CFI_SP)
 			cfa->offset += 8;
 
 		if (op->src.type != OP_SRC_REG)
 			break;
 
-		if (state->drap) {
-			if (op->src.reg == cfa->base && op->src.reg == state->drap_reg) {
+		if (cfi->drap) {
+			if (op->src.reg == cfa->base && op->src.reg == cfi->drap_reg) {
 
 				/* drap: push %drap */
 				cfa->base = CFI_BP_INDIRECT;
-				cfa->offset = -state->stack_size;
+				cfa->offset = -cfi->stack_size;
 
 				/* save drap so we know when to restore it */
-				state->drap_offset = -state->stack_size;
+				cfi->drap_offset = -cfi->stack_size;
 
-			} else if (op->src.reg == CFI_BP && cfa->base == state->drap_reg) {
+			} else if (op->src.reg == CFI_BP && cfa->base == cfi->drap_reg) {
 
 				/* drap: push %rbp */
-				state->stack_size = 0;
+				cfi->stack_size = 0;
 
 			} else if (regs[op->src.reg].base == CFI_UNDEFINED) {
 
 				/* drap: push %reg */
-				save_reg(state, op->src.reg, CFI_BP, -state->stack_size);
+				save_reg(cfi, op->src.reg, CFI_BP, -cfi->stack_size);
 			}
 
 		} else {
 
 			/* push %reg */
-			save_reg(state, op->src.reg, CFI_CFA, -state->stack_size);
+			save_reg(cfi, op->src.reg, CFI_CFA, -cfi->stack_size);
 		}
 
 		/* detect when asm code uses rbp as a scratch register */
 		if (!no_fp && insn->func && op->src.reg == CFI_BP &&
 		    cfa->base != CFI_BP)
-			state->bp_scratch = true;
+			cfi->bp_scratch = true;
 		break;
 
 	case OP_DEST_REG_INDIRECT:
 
-		if (state->drap) {
-			if (op->src.reg == cfa->base && op->src.reg == state->drap_reg) {
+		if (cfi->drap) {
+			if (op->src.reg == cfa->base && op->src.reg == cfi->drap_reg) {
 
 				/* drap: mov %drap, disp(%rbp) */
 				cfa->base = CFI_BP_INDIRECT;
 				cfa->offset = op->dest.offset;
 
 				/* save drap offset so we know when to restore it */
-				state->drap_offset = op->dest.offset;
+				cfi->drap_offset = op->dest.offset;
 			}
 
 			else if (regs[op->src.reg].base == CFI_UNDEFINED) {
 
 				/* drap: mov reg, disp(%rbp) */
-				save_reg(state, op->src.reg, CFI_BP, op->dest.offset);
+				save_reg(cfi, op->src.reg, CFI_BP, op->dest.offset);
 			}
 
 		} else if (op->dest.reg == cfa->base) {
 
 			/* mov reg, disp(%rbp) */
 			/* mov reg, disp(%rsp) */
-			save_reg(state, op->src.reg, CFI_CFA,
-				 op->dest.offset - state->cfa.offset);
+			save_reg(cfi, op->src.reg, CFI_CFA,
+				 op->dest.offset - cfi->cfa.offset);
 		}
 
 		break;
 
 	case OP_DEST_LEAVE:
-		if ((!state->drap && cfa->base != CFI_BP) ||
-		    (state->drap && cfa->base != state->drap_reg)) {
+		if ((!cfi->drap && cfa->base != CFI_BP) ||
+		    (cfi->drap && cfa->base != cfi->drap_reg)) {
 			WARN_FUNC("leave instruction with modified stack frame",
 				  insn->sec, insn->offset);
 			return -1;
@@ -1879,10 +1886,10 @@ static int update_insn_state(struct inst
 
 		/* leave (mov %rbp, %rsp; pop %rbp) */
 
-		state->stack_size = -state->regs[CFI_BP].offset - 8;
-		restore_reg(state, CFI_BP);
+		cfi->stack_size = -cfi->regs[CFI_BP].offset - 8;
+		restore_reg(cfi, CFI_BP);
 
-		if (!state->drap) {
+		if (!cfi->drap) {
 			cfa->base = CFI_SP;
 			cfa->offset -= 8;
 		}
@@ -1897,7 +1904,7 @@ static int update_insn_state(struct inst
 		}
 
 		/* pop mem */
-		state->stack_size -= 8;
+		cfi->stack_size -= 8;
 		if (cfa->base == CFI_SP)
 			cfa->offset -= 8;
 
@@ -1912,41 +1919,44 @@ static int update_insn_state(struct inst
 	return 0;
 }
 
-static bool insn_state_match(struct instruction *insn, struct insn_state *state)
+static bool insn_cfi_match(struct instruction *insn, struct cfi_state *cfi2)
 {
-	struct insn_state *state1 = &insn->state, *state2 = state;
+	struct cfi_state *cfi1 = &insn->cfi;
 	int i;
 
-	if (memcmp(&state1->cfa, &state2->cfa, sizeof(state1->cfa))) {
+	if (memcmp(&cfi1->cfa, &cfi2->cfa, sizeof(cfi1->cfa))) {
+
 		WARN_FUNC("stack state mismatch: cfa1=%d%+d cfa2=%d%+d",
 			  insn->sec, insn->offset,
-			  state1->cfa.base, state1->cfa.offset,
-			  state2->cfa.base, state2->cfa.offset);
+			  cfi1->cfa.base, cfi1->cfa.offset,
+			  cfi2->cfa.base, cfi2->cfa.offset);
 
-	} else if (memcmp(&state1->regs, &state2->regs, sizeof(state1->regs))) {
+	} else if (memcmp(&cfi1->regs, &cfi2->regs, sizeof(cfi1->regs))) {
 		for (i = 0; i < CFI_NUM_REGS; i++) {
-			if (!memcmp(&state1->regs[i], &state2->regs[i],
+			if (!memcmp(&cfi1->regs[i], &cfi2->regs[i],
 				    sizeof(struct cfi_reg)))
 				continue;
 
 			WARN_FUNC("stack state mismatch: reg1[%d]=%d%+d reg2[%d]=%d%+d",
 				  insn->sec, insn->offset,
-				  i, state1->regs[i].base, state1->regs[i].offset,
-				  i, state2->regs[i].base, state2->regs[i].offset);
+				  i, cfi1->regs[i].base, cfi1->regs[i].offset,
+				  i, cfi2->regs[i].base, cfi2->regs[i].offset);
 			break;
 		}
 
-	} else if (state1->type != state2->type) {
+	} else if (cfi1->type != cfi2->type) {
+
 		WARN_FUNC("stack state mismatch: type1=%d type2=%d",
-			  insn->sec, insn->offset, state1->type, state2->type);
+			  insn->sec, insn->offset, cfi1->type, cfi2->type);
+
+	} else if (cfi1->drap != cfi2->drap ||
+		   (cfi1->drap && cfi1->drap_reg != cfi2->drap_reg) ||
+		   (cfi1->drap && cfi1->drap_offset != cfi2->drap_offset)) {
 
-	} else if (state1->drap != state2->drap ||
-		 (state1->drap && state1->drap_reg != state2->drap_reg) ||
-		 (state1->drap && state1->drap_offset != state2->drap_offset)) {
 		WARN_FUNC("stack state mismatch: drap1=%d(%d,%d) drap2=%d(%d,%d)",
 			  insn->sec, insn->offset,
-			  state1->drap, state1->drap_reg, state1->drap_offset,
-			  state2->drap, state2->drap_reg, state2->drap_offset);
+			  cfi1->drap, cfi1->drap_reg, cfi1->drap_offset,
+			  cfi2->drap, cfi2->drap_reg, cfi2->drap_offset);
 
 	} else
 		return true;
@@ -2024,7 +2034,7 @@ static int validate_return(struct symbol
 		return 1;
 	}
 
-	if (state->bp_scratch) {
+	if (state->cfi.bp_scratch) {
 		WARN("%s uses BP as a scratch register",
 		     func->name);
 		return 1;
@@ -2035,7 +2045,7 @@ static int validate_return(struct symbol
 
 static int apply_insn_hint(struct objtool_file *file, struct section *sec,
 			   struct symbol *func, struct instruction *insn,
-			   struct insn_state *state)
+			   struct cfi_state *cfi)
 {
 	if (insn->restore) {
 		struct instruction *save_insn, *i;
@@ -2061,10 +2071,10 @@ static int apply_insn_hint(struct objtoo
 			return 1;
 		}
 
-		insn->state = save_insn->state;
+		insn->cfi = save_insn->cfi;
 	}
 
-	state = insn->state;
+	*cfi = insn->cfi;
 	return 0;
 }
 
@@ -2108,7 +2118,7 @@ static int validate_branch(struct objtoo
 
 		visited = 1 << state.uaccess;
 		if (insn->visited) {
-			if (!insn->hint && !insn_state_match(insn, &state))
+			if (!insn->hint && !insn_cfi_match(insn, &state.cfi))
 				return 1;
 
 			if (insn->visited & visited)
@@ -2116,11 +2126,11 @@ static int validate_branch(struct objtoo
 		}
 
 		if (insn->hint) {
-			ret = apply_insn_hint(file, sec, func, insn, &state);
+			ret = apply_insn_hint(file, sec, func, insn, &state.cfi);
 			if (ret)
 				return ret;
 		} else
-			insn->state = state;
+			insn->cfi = state.cfi;
 
 		insn->visited |= visited;
 
@@ -2210,7 +2220,7 @@ static int validate_branch(struct objtoo
 			return 0;
 
 		case INSN_STACK:
-			if (update_insn_state(insn, &state))
+			if (update_cfi_state(insn, &state.cfi))
 				return 1;
 
 			if (insn->stack_op.dest.type == OP_DEST_PUSHF) {
@@ -2280,7 +2290,7 @@ static int validate_branch(struct objtoo
 			return 0;
 
 		if (!next_insn) {
-			if (state.cfa.base == CFI_UNDEFINED)
+			if (state.cfi.cfa.base == CFI_UNDEFINED)
 				return 0;
 			WARN("%s: unexpected end of section", sec->name);
 			return 1;
@@ -2420,10 +2430,10 @@ static int validate_section(struct objto
 
 	clear_insn_state(&state);
 
-	state.cfa = initial_func_cfi.cfa;
-	memcpy(&state.regs, &initial_func_cfi.regs,
+	state.cfi.cfa = initial_func_cfi.cfa;
+	memcpy(&state.cfi.regs, &initial_func_cfi.regs,
 	       CFI_NUM_REGS * sizeof(struct cfi_reg));
-	state.stack_size = initial_func_cfi.cfa.offset;
+	state.cfi.stack_size = initial_func_cfi.cfa.offset;
 
 	list_for_each_entry(func, &sec->symbol_list, list) {
 		if (func->type != STT_FUNC)
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -14,15 +14,10 @@
 #include <linux/hashtable.h>
 
 struct insn_state {
-	struct cfi_reg cfa;
-	struct cfi_reg regs[CFI_NUM_REGS];
-	int stack_size;
-	unsigned char type;
-	bool bp_scratch;
-	bool drap, end, uaccess, df;
+	struct cfi_state cfi;
 	unsigned int uaccess_stack;
-	int drap_reg, drap_offset;
-	struct cfi_reg vals[CFI_NUM_REGS];
+	bool uaccess;
+	bool df;
 };
 
 struct instruction {
@@ -43,7 +38,7 @@ struct instruction {
 	struct list_head alts;
 	struct symbol *func;
 	struct stack_op stack_op;
-	struct insn_state state;
+	struct cfi_state cfi;
 	struct orc_entry orc;
 };
 
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -16,10 +16,10 @@ int create_orc(struct objtool_file *file
 
 	for_each_insn(file, insn) {
 		struct orc_entry *orc = &insn->orc;
-		struct cfi_reg *cfa = &insn->state.cfa;
-		struct cfi_reg *bp = &insn->state.regs[CFI_BP];
+		struct cfi_reg *cfa = &insn->cfi.cfa;
+		struct cfi_reg *bp = &insn->cfi.regs[CFI_BP];
 
-		orc->end = insn->state.end;
+		orc->end = insn->cfi.end;
 
 		if (cfa->base == CFI_UNDEFINED) {
 			orc->sp_reg = ORC_REG_UNDEFINED;
@@ -75,7 +75,7 @@ int create_orc(struct objtool_file *file
 
 		orc->sp_offset = cfa->offset;
 		orc->bp_offset = bp->offset;
-		orc->type = insn->state.type;
+		orc->type = insn->cfi.type;
 	}
 
 	return 0;

[PATCH v4 05/13] objtool: Implement noinstr validation

[Peter Zijlstra]

Validate that any call out of .noinstr.text is in between
instr_begin() and instr_end() annotations.

This annotation is useful to ensure correct behaviour wrt tracing
sensitive code like entry/exit and idle code. When we run code in a
sensitive context we want a guarantee no unknown code is ran.

Since this validation relies on knowing the section of call
destination symbols, we must run it on vmlinux.o instead of on
individual object files.

Add two options:

 -d/--duplicate "duplicate validation for vmlinux"
 -l/--vmlinux "vmlinux.o validation"

Where the latter auto-detects when objname ends with "vmlinux.o" and
the former will force all validations, also those already done on
!vmlinux object files.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/builtin-check.c |   11 +++-
 tools/objtool/builtin.h       |    2 
 tools/objtool/check.c         |   98 ++++++++++++++++++++++++++++++++++++++++++
 tools/objtool/check.h         |    3 +
 tools/objtool/elf.h           |    2 
 5 files changed, 112 insertions(+), 4 deletions(-)

--- a/tools/objtool/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -14,10 +14,11 @@
  */
 
 #include <subcmd/parse-options.h>
+#include <string.h>
 #include "builtin.h"
 #include "check.h"
 
-bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats;
+bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux;
 
 static const char * const check_usage[] = {
 	"objtool check [<options>] file.o",
@@ -32,12 +33,14 @@ const struct option check_options[] = {
 	OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"),
 	OPT_BOOLEAN('a', "uaccess", &uaccess, "enable uaccess checking"),
 	OPT_BOOLEAN('s', "stats", &stats, "print statistics"),
+	OPT_BOOLEAN('d', "duplicate", &validate_dup, "duplicate validation for vmlinux.o"),
+	OPT_BOOLEAN('l', "vmlinux", &vmlinux, "vmlinux.o validation"),
 	OPT_END(),
 };
 
 int cmd_check(int argc, const char **argv)
 {
-	const char *objname;
+	const char *objname, *s;
 
 	argc = parse_options(argc, argv, check_options, check_usage, 0);
 
@@ -46,5 +49,9 @@ int cmd_check(int argc, const char **arg
 
 	objname = argv[0];
 
+	s = strstr(objname, "vmlinux.o");
+	if (s && !s[9])
+		vmlinux = true;
+
 	return check(objname, false);
 }
--- a/tools/objtool/builtin.h
+++ b/tools/objtool/builtin.h
@@ -8,7 +8,7 @@
 #include <subcmd/parse-options.h>
 
 extern const struct option check_options[];
-extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats;
+extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, validate_dup, vmlinux;
 
 extern int cmd_check(int argc, const char **argv);
 extern int cmd_orc(int argc, const char **argv);
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -257,6 +257,9 @@ static int decode_instructions(struct ob
 		    strncmp(sec->name, ".discard.", 9))
 			sec->text = true;
 
+		if (!strcmp(sec->name, ".noinstr.text"))
+			sec->noinstr = true;
+
 		for (offset = 0; offset < sec->len; offset += insn->len) {
 			insn = malloc(sizeof(*insn));
 			if (!insn) {
@@ -1355,6 +1358,53 @@ static int read_retpoline_hints(struct o
 	return 0;
 }
 
+static int read_instr_hints(struct objtool_file *file)
+{
+	struct section *sec;
+	struct instruction *insn;
+	struct rela *rela;
+
+	sec = find_section_by_name(file->elf, ".rela.discard.instr_end");
+	if (!sec)
+		return 0;
+
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		if (rela->sym->type != STT_SECTION) {
+			WARN("unexpected relocation symbol type in %s", sec->name);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("bad .discard.instr_end entry");
+			return -1;
+		}
+
+		insn->instr--;
+	}
+
+	sec = find_section_by_name(file->elf, ".rela.discard.instr_begin");
+	if (!sec)
+		return 0;
+
+	list_for_each_entry(rela, &sec->rela_list, list) {
+		if (rela->sym->type != STT_SECTION) {
+			WARN("unexpected relocation symbol type in %s", sec->name);
+			return -1;
+		}
+
+		insn = find_insn(file, rela->sym->sec, rela->addend);
+		if (!insn) {
+			WARN("bad .discard.instr_begin entry");
+			return -1;
+		}
+
+		insn->instr++;
+	}
+
+	return 0;
+}
+
 static void mark_rodata(struct objtool_file *file)
 {
 	struct section *sec;
@@ -1426,6 +1476,10 @@ static int decode_sections(struct objtoo
 	if (ret)
 		return ret;
 
+	ret = read_instr_hints(file);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
@@ -1982,6 +2036,13 @@ static inline const char *call_dest_name
 
 static int validate_call(struct instruction *insn, struct insn_state *state)
 {
+	if (state->noinstr && state->instr <= 0 &&
+	    (!insn->call_dest || insn->call_dest->sec != insn->sec)) {
+		WARN_FUNC("call to %s() leaves .noinstr.text section",
+				insn->sec, insn->offset, call_dest_name(insn));
+		return 1;
+	}
+
 	if (state->uaccess && !func_uaccess_safe(insn->call_dest)) {
 		WARN_FUNC("call to %s() with UACCESS enabled",
 				insn->sec, insn->offset, call_dest_name(insn));
@@ -2010,6 +2071,12 @@ static int validate_sibling_call(struct
 
 static int validate_return(struct symbol *func, struct instruction *insn, struct insn_state *state)
 {
+	if (state->noinstr && state->instr > 0) {
+		WARN_FUNC("return with instrumentation enabled",
+			  insn->sec, insn->offset);
+		return 1;
+	}
+
 	if (state->uaccess && !func_uaccess_safe(func)) {
 		WARN_FUNC("return with UACCESS enabled",
 			  insn->sec, insn->offset);
@@ -2124,6 +2191,9 @@ static int validate_branch(struct objtoo
 				return 0;
 		}
 
+		if (state.noinstr)
+			state.instr += insn->instr;
+
 		if (insn->hint) {
 			ret = apply_insn_hint(file, sec, func, insn, &state.cfi);
 			if (ret)
@@ -2434,6 +2504,14 @@ static int validate_section(struct objto
 	       CFI_NUM_REGS * sizeof(struct cfi_reg));
 	state.cfi.stack_size = initial_func_cfi.cfa.offset;
 
+	/*
+	 * We need the full vmlinux for noinstr validation, otherwise we can
+	 * not correctly determine insn->call_dest->sec (external symbols do
+	 * not have a section).
+	 */
+	if (vmlinux)
+		state.noinstr = sec->noinstr;
+
 	list_for_each_entry(func, &sec->symbol_list, list) {
 		if (func->type != STT_FUNC)
 			continue;
@@ -2462,6 +2540,17 @@ static int validate_section(struct objto
 	return warnings;
 }
 
+static int validate_vmlinux_functions(struct objtool_file *file)
+{
+	struct section *sec;
+
+	sec = find_section_by_name(file->elf, ".noinstr.text");
+	if (!sec)
+		return 0;
+
+	return validate_section(file, sec);
+}
+
 static int validate_functions(struct objtool_file *file)
 {
 	struct section *sec;
@@ -2519,6 +2608,15 @@ int check(const char *_objname, bool orc
 	if (list_empty(&file.insn_list))
 		goto out;
 
+	if (vmlinux && !validate_dup) {
+		ret = validate_vmlinux_functions(&file);
+		if (ret < 0)
+			goto out;
+
+		warnings += ret;
+		goto out;
+	}
+
 	if (retpoline) {
 		ret = validate_retpoline(&file);
 		if (ret < 0)
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -18,6 +18,8 @@ struct insn_state {
 	unsigned int uaccess_stack;
 	bool uaccess;
 	bool df;
+	bool noinstr;
+	s8 instr;
 };
 
 struct instruction {
@@ -30,6 +32,7 @@ struct instruction {
 	unsigned long immediate;
 	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
 	bool retpoline_safe;
+	s8 instr;
 	u8 visited;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -39,7 +39,7 @@ struct section {
 	char *name;
 	int idx;
 	unsigned int len;
-	bool changed, text, rodata;
+	bool changed, text, rodata, noinstr;
 };
 
 struct symbol {

[PATCH v4 06/13] objtool: Optimize !vmlinux.o again

[Peter Zijlstra]

When doing kbuild tests to see if the objtool changes affected those I
found that there was a measurable regression:

          pre		  post

  real    1m13.594        1m16.488s
  user    34m58.246s      35m23.947s
  sys     4m0.393s        4m27.312s

Perf showed that for small files the increased hash-table sizes were a
measurable difference. Since we already have -l "vmlinux" to
distinguish between the modes, make it also use a smaller portion of
the hash-tables.

This flips it into a small win:

  real    1m14.143s
  user    34m49.292s
  sys     3m44.746s

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/elf.c     |   62 +++++++++++++++++++++++++++++++++---------------
 tools/objtool/elf.h     |   13 ++++++----
 tools/objtool/orc_gen.c |    3 --
 3 files changed, 52 insertions(+), 26 deletions(-)

--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -27,6 +27,22 @@ static inline u32 str_hash(const char *s
 	return jhash(str, strlen(str), 0);
 }
 
+static inline int elf_hash_bits(void)
+{
+	return vmlinux ? ELF_HASH_BITS : 16;
+}
+
+#define elf_hash_add(hashtable, node, key) \
+	hlist_add_head(node, &hashtable[hash_min(key, elf_hash_bits())])
+
+static void elf_hash_init(struct hlist_head *table)
+{
+	__hash_init(table, 1U << elf_hash_bits());
+}
+
+#define elf_hash_for_each_possible(name, obj, member, key)			\
+	hlist_for_each_entry(obj, &name[hash_min(key, elf_hash_bits())], member)
+
 static void rb_add(struct rb_root *tree, struct rb_node *node,
 		   int (*cmp)(struct rb_node *, const struct rb_node *))
 {
@@ -115,7 +131,7 @@ struct section *find_section_by_name(str
 {
 	struct section *sec;
 
-	hash_for_each_possible(elf->section_name_hash, sec, name_hash, str_hash(name))
+	elf_hash_for_each_possible(elf->section_name_hash, sec, name_hash, str_hash(name))
 		if (!strcmp(sec->name, name))
 			return sec;
 
@@ -127,7 +143,7 @@ static struct section *find_section_by_i
 {
 	struct section *sec;
 
-	hash_for_each_possible(elf->section_hash, sec, hash, idx)
+	elf_hash_for_each_possible(elf->section_hash, sec, hash, idx)
 		if (sec->idx == idx)
 			return sec;
 
@@ -138,7 +154,7 @@ static struct symbol *find_symbol_by_ind
 {
 	struct symbol *sym;
 
-	hash_for_each_possible(elf->symbol_hash, sym, hash, idx)
+	elf_hash_for_each_possible(elf->symbol_hash, sym, hash, idx)
 		if (sym->idx == idx)
 			return sym;
 
@@ -205,7 +221,7 @@ struct symbol *find_symbol_by_name(struc
 {
 	struct symbol *sym;
 
-	hash_for_each_possible(elf->symbol_name_hash, sym, name_hash, str_hash(name))
+	elf_hash_for_each_possible(elf->symbol_name_hash, sym, name_hash, str_hash(name))
 		if (!strcmp(sym->name, name))
 			return sym;
 
@@ -224,7 +240,7 @@ struct rela *find_rela_by_dest_range(str
 	sec = sec->rela;
 
 	for_offset_range(o, offset, offset + len) {
-		hash_for_each_possible(elf->rela_hash, rela, hash,
+		elf_hash_for_each_possible(elf->rela_hash, rela, hash,
 				       sec_offset_hash(sec, o)) {
 			if (rela->sec != sec)
 				continue;
@@ -309,8 +325,8 @@ static int read_sections(struct elf *elf
 		sec->len = sec->sh.sh_size;
 
 		list_add_tail(&sec->list, &elf->sections);
-		hash_add(elf->section_hash, &sec->hash, sec->idx);
-		hash_add(elf->section_name_hash, &sec->name_hash, str_hash(sec->name));
+		elf_hash_add(elf->section_hash, &sec->hash, sec->idx);
+		elf_hash_add(elf->section_name_hash, &sec->name_hash, str_hash(sec->name));
 	}
 
 	if (stats)
@@ -394,8 +410,8 @@ static int read_symbols(struct elf *elf)
 		else
 			entry = &sym->sec->symbol_list;
 		list_add(&sym->list, entry);
-		hash_add(elf->symbol_hash, &sym->hash, sym->idx);
-		hash_add(elf->symbol_name_hash, &sym->name_hash, str_hash(sym->name));
+		elf_hash_add(elf->symbol_hash, &sym->hash, sym->idx);
+		elf_hash_add(elf->symbol_name_hash, &sym->name_hash, str_hash(sym->name));
 	}
 
 	if (stats)
@@ -456,6 +472,14 @@ static int read_symbols(struct elf *elf)
 	return -1;
 }
 
+void elf_add_rela(struct elf *elf, struct rela *rela)
+{
+	struct section *sec = rela->sec;
+
+	list_add_tail(&rela->list, &sec->rela_list);
+	elf_hash_add(elf->rela_hash, &rela->hash, rela_hash(rela));
+}
+
 static int read_relas(struct elf *elf)
 {
 	struct section *sec;
@@ -503,8 +527,7 @@ static int read_relas(struct elf *elf)
 				return -1;
 			}
 
-			list_add_tail(&rela->list, &sec->rela_list);
-			hash_add(elf->rela_hash, &rela->hash, rela_hash(rela));
+			elf_add_rela(elf, rela);
 			nr_rela++;
 		}
 		max_rela = max(max_rela, nr_rela);
@@ -531,15 +554,16 @@ struct elf *elf_read(const char *name, i
 		perror("malloc");
 		return NULL;
 	}
-	memset(elf, 0, sizeof(*elf));
+	memset(elf, 0, offsetof(struct elf, sections));
 
-	hash_init(elf->symbol_hash);
-	hash_init(elf->symbol_name_hash);
-	hash_init(elf->section_hash);
-	hash_init(elf->section_name_hash);
-	hash_init(elf->rela_hash);
 	INIT_LIST_HEAD(&elf->sections);
 
+	elf_hash_init(elf->symbol_hash);
+	elf_hash_init(elf->symbol_name_hash);
+	elf_hash_init(elf->section_hash);
+	elf_hash_init(elf->section_name_hash);
+	elf_hash_init(elf->rela_hash);
+
 	elf->fd = open(name, flags);
 	if (elf->fd == -1) {
 		fprintf(stderr, "objtool: Can't open '%s': %s\n",
@@ -676,8 +700,8 @@ struct section *elf_create_section(struc
 	shstrtab->changed = true;
 
 	list_add_tail(&sec->list, &elf->sections);
-	hash_add(elf->section_hash, &sec->hash, sec->idx);
-	hash_add(elf->section_name_hash, &sec->name_hash, str_hash(sec->name));
+	elf_hash_add(elf->section_hash, &sec->hash, sec->idx);
+	elf_hash_add(elf->section_name_hash, &sec->name_hash, str_hash(sec->name));
 
 	return sec;
 }
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -70,17 +70,19 @@ struct rela {
 	bool jump_table_start;
 };
 
+#define ELF_HASH_BITS	20
+
 struct elf {
 	Elf *elf;
 	GElf_Ehdr ehdr;
 	int fd;
 	char *name;
 	struct list_head sections;
-	DECLARE_HASHTABLE(symbol_hash, 20);
-	DECLARE_HASHTABLE(symbol_name_hash, 20);
-	DECLARE_HASHTABLE(section_hash, 16);
-	DECLARE_HASHTABLE(section_name_hash, 16);
-	DECLARE_HASHTABLE(rela_hash, 20);
+	DECLARE_HASHTABLE(symbol_hash, ELF_HASH_BITS);
+	DECLARE_HASHTABLE(symbol_name_hash, ELF_HASH_BITS);
+	DECLARE_HASHTABLE(section_hash, ELF_HASH_BITS);
+	DECLARE_HASHTABLE(section_name_hash, ELF_HASH_BITS);
+	DECLARE_HASHTABLE(rela_hash, ELF_HASH_BITS);
 };
 
 #define OFFSET_STRIDE_BITS	4
@@ -127,6 +129,7 @@ struct section *elf_create_rela_section(
 int elf_rebuild_rela_section(struct section *sec);
 int elf_write(struct elf *elf);
 void elf_close(struct elf *elf);
+void elf_add_rela(struct elf *elf, struct rela *rela);
 
 #define for_each_sec(file, sec)						\
 	list_for_each_entry(sec, &file->elf->sections, list)
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -111,8 +111,7 @@ static int create_orc_entry(struct elf *
 	rela->offset = idx * sizeof(int);
 	rela->sec = ip_relasec;
 
-	list_add_tail(&rela->list, &ip_relasec->rela_list);
-	hash_add(elf->rela_hash, &rela->hash, rela_hash(rela));
+	elf_add_rela(elf, rela);
 
 	return 0;
 }

[PATCH v4 07/13] objtool: Use sec_offset_hash() for insn_hash

[Peter Zijlstra]

In preparation for find_insn_containing(), change insn_hash to use
sec_offset_hash().

This actually reduces runtime; probably because mixing in the section
index reduces the collisions due to text sections all starting their
instructions at offset 0.

Runtime on vmlinux.o from 3.1 to 2.5 seconds.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -34,9 +34,10 @@ struct instruction *find_insn(struct obj
 {
 	struct instruction *insn;
 
-	hash_for_each_possible(file->insn_hash, insn, hash, offset)
+	hash_for_each_possible(file->insn_hash, insn, hash, sec_offset_hash(sec, offset)) {
 		if (insn->sec == sec && insn->offset == offset)
 			return insn;
+	}
 
 	return NULL;
 }
@@ -276,7 +277,7 @@ static int decode_instructions(struct ob
 			if (ret)
 				goto err;
 
-			hash_add(file->insn_hash, &insn->hash, insn->offset);
+			hash_add(file->insn_hash, &insn->hash, sec_offset_hash(sec, insn->offset));
 			list_add_tail(&insn->list, &file->insn_list);
 			nr_insns++;
 		}

[PATCH v4 08/13] objtool: Detect loading function pointers across noinstr

[Peter Zijlstra]

Detect if noinstr text loads functions pointers from regular text,
doing so is a definite sign that indirect function calls are unsafe.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/arch.h  |    2 +
 tools/objtool/check.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/objtool/check.h |    2 -
 3 files changed, 74 insertions(+), 1 deletion(-)

--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -75,4 +75,6 @@ int arch_decode_instruction(struct elf *
 
 bool arch_callee_saved_reg(unsigned char reg);
 
+#define MAX_INSN_SIZE 15
+
 #endif /* _ARCH_H */
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -42,6 +42,25 @@ struct instruction *find_insn(struct obj
 	return NULL;
 }
 
+static struct instruction *find_insn_containing(struct objtool_file *file, struct section *sec,
+					 unsigned long offset)
+{
+	struct instruction *insn;
+	unsigned long o;
+
+	for_offset_range(o, offset - MAX_INSN_SIZE - 1, offset) {
+		hash_for_each_possible(file->insn_hash, insn, hash, sec_offset_hash(sec, o)) {
+			if (insn->sec != sec)
+				continue;
+
+			if (insn->offset <= offset && insn->offset + insn->len > offset)
+				return insn;
+		}
+	}
+
+	return NULL;
+}
+
 static struct instruction *next_insn_same_sec(struct objtool_file *file,
 					      struct instruction *insn)
 {
@@ -2146,6 +2165,32 @@ static int apply_insn_hint(struct objtoo
 	return 0;
 }
 
+static int validate_rela(struct instruction *insn, struct insn_state *state)
+{
+	/*
+	 * Assume that any text rela that's not a CALL or JMP is a load of a
+	 * function pointer.
+	 */
+
+	switch (insn->type) {
+	case INSN_CALL:
+	case INSN_CALL_DYNAMIC:
+	case INSN_JUMP_CONDITIONAL:
+	case INSN_JUMP_UNCONDITIONAL:
+		return 0;
+
+	default:
+		break;
+	}
+
+	if (state->noinstr && state->instr <= 0 && insn->has_text_rela) {
+		WARN_FUNC("loading non-noinstr function pointer", insn->sec, insn->offset);
+		return 1;
+	}
+
+	return 0;
+}
+
 /*
  * Follow the branch starting at the given instruction, and recursively follow
  * any other branches (jumps).  Meanwhile, track the frame pointer state at
@@ -2224,6 +2269,10 @@ static int validate_branch(struct objtoo
 				return 0;
 		}
 
+		ret = validate_rela(insn, &state);
+		if (ret)
+			return ret;
+
 		switch (insn->type) {
 
 		case INSN_RETURN:
@@ -2492,6 +2541,25 @@ static bool ignore_unreachable_insn(stru
 	return false;
 }
 
+static void prepare_insn_rela(struct objtool_file *file, struct section *sec)
+{
+	struct instruction *insn;
+	struct rela *rela;
+
+	if (!sec->rela)
+		return;
+
+	list_for_each_entry(rela, &sec->rela->rela_list, list) {
+		insn = find_insn_containing(file, sec, rela->offset);
+		if (!insn)
+			continue;
+
+		insn->has_text_rela = rela->sym && rela->sym->sec &&
+				      rela->sym->sec->text &&
+				      !rela->sym->sec->noinstr;
+	}
+}
+
 static int validate_section(struct objtool_file *file, struct section *sec)
 {
 	struct symbol *func;
@@ -2514,6 +2582,9 @@ static int validate_section(struct objto
 	if (vmlinux)
 		state.noinstr = sec->noinstr;
 
+	if (state.noinstr)
+		prepare_insn_rela(file, sec);
+
 	list_for_each_entry(func, &sec->symbol_list, list) {
 		if (func->type != STT_FUNC)
 			continue;
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -31,7 +31,7 @@ struct instruction {
 	enum insn_type type;
 	unsigned long immediate;
 	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
-	bool retpoline_safe;
+	bool retpoline_safe, has_text_rela;
 	s8 instr;
 	u8 visited;
 	struct symbol *call_dest;

[PATCH v4 09/13] kbuild/objtool: Add objtool-vmlinux.o pass

[Peter Zijlstra]

Now that objtool is capable of processing vmlinux.o and actually has
something useful to do there, (conditionally) add it to the final link
pass.

This will increase build time by a few seconds.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 lib/Kconfig.debug       |    5 +++++
 scripts/link-vmlinux.sh |   24 ++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -379,6 +379,11 @@ config STACK_VALIDATION
 	  For more information, see
 	  tools/objtool/Documentation/stack-validation.txt.
 
+config VMLINUX_VALIDATION
+	bool
+	depends on STACK_VALIDATION && DEBUG_ENTRY && !PARAVIRT
+	default y
+
 config DEBUG_FORCE_WEAK_PER_CPU
 	bool "Force weak per-cpu definitions"
 	depends on DEBUG_KERNEL
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -55,6 +55,29 @@ modpost_link()
 	${LD} ${KBUILD_LDFLAGS} -r -o ${1} ${objects}
 }
 
+objtool_link()
+{
+	local objtoolopt;
+
+	if [ -n "${CONFIG_VMLINUX_VALIDATION}" ]; then
+		objtoolopt="check"
+		if [ -z "${CONFIG_FRAME_POINTER}" ]; then
+			objtoolopt="${objtoolopt} --no-fp"
+		fi
+		if [ -n "${CONFIG_GCOV_KERNEL}" ]; then
+			objtoolopt="${objtoolopt} --no-unreachable"
+		fi
+		if [ -n "${CONFIG_RETPOLINE}" ]; then
+			objtoolopt="${objtoolopt} --retpoline"
+		fi
+		if [ -n "${CONFIG_X86_SMAP}" ]; then
+			objtoolopt="${objtoolopt} --uaccess"
+		fi
+		info OBJTOOL ${1}
+		tools/objtool/objtool ${objtoolopt} ${1}
+	fi
+}
+
 # Link of vmlinux
 # ${1} - output file
 # ${2}, ${3}, ... - optional extra .o files
@@ -244,6 +267,7 @@ ${MAKE} -f "${srctree}/scripts/Makefile.
 #link vmlinux.o
 info LD vmlinux.o
 modpost_link vmlinux.o
+objtool_link vmlinux.o
 
 # modpost vmlinux.o to check for section mismatches
 ${MAKE} -f "${srctree}/scripts/Makefile.modpost" MODPOST_VMLINUX=1

[PATCH v4 10/13] objtool: Avoid iterating !text section symbols

[Peter Zijlstra]

validate_functions() iterates all sections their symbols; this is
pointless to do for !text sections as they won't have instructions
anyway.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2621,8 +2621,12 @@ static int validate_functions(struct obj
 	struct section *sec;
 	int warnings = 0;
 
-	for_each_sec(file, sec)
+	for_each_sec(file, sec) {
+		if (!(sec->sh.sh_flags & SHF_EXECINSTR))
+			continue;
+
 		warnings += validate_section(file, sec);
+	}
 
 	return warnings;
 }

[PATCH v4 11/13] objtool: Rearrange validate_section()

[Peter Zijlstra]

In preparation of further changes, once again break out the loop body.
No functional changes intended.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c |   51 ++++++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 22 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2559,12 +2559,37 @@ static void prepare_insn_rela(struct obj
 	}
 }
 
-static int validate_section(struct objtool_file *file, struct section *sec)
+static int validate_symbol(struct objtool_file *file, struct section *sec,
+			   struct symbol *sym, struct insn_state *state)
 {
-	struct symbol *func;
 	struct instruction *insn;
+	int ret;
+
+	if (!sym->len) {
+		WARN("%s() is missing an ELF size annotation", sym->name);
+		return 1;
+	}
+
+	if (sym->pfunc != sym || sym->alias != sym)
+		return 0;
+
+	insn = find_insn(file, sec, sym->offset);
+	if (!insn || insn->ignore || insn->visited)
+		return 0;
+
+	state->uaccess = sym->uaccess_safe;
+
+	ret = validate_branch(file, insn->func, insn, *state);
+	if (ret && backtrace)
+		BT_FUNC("<=== (sym)", insn);
+	return ret;
+}
+
+static int validate_section(struct objtool_file *file, struct section *sec)
+{
 	struct insn_state state;
-	int ret, warnings = 0;
+	struct symbol *func;
+	int warnings = 0;
 
 	clear_insn_state(&state);
 
@@ -2588,25 +2613,7 @@ static int validate_section(struct objto
 		if (func->type != STT_FUNC)
 			continue;
 
-		if (!func->len) {
-			WARN("%s() is missing an ELF size annotation",
-			     func->name);
-			warnings++;
-		}
-
-		if (func->pfunc != func || func->alias != func)
-			continue;
-
-		insn = find_insn(file, sec, func->offset);
-		if (!insn || insn->ignore || insn->visited)
-			continue;
-
-		state.uaccess = func->uaccess_safe;
-
-		ret = validate_branch(file, func, insn, state);
-		if (ret && backtrace)
-			BT_FUNC("<=== (func)", insn);
-		warnings += ret;
+		warnings += validate_symbol(file, sec, func, &state);
 	}
 
 	return warnings;

[PATCH v4 12/13] objtool: Add STT_NOTYPE noinstr validation

[Peter Zijlstra]

Make sure to also check STT_NOTYPE symbols for noinstr violations.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -248,10 +248,18 @@ static void init_cfi_state(struct cfi_st
 	cfi->drap_offset = -1;
 }
 
-static void clear_insn_state(struct insn_state *state)
+static void init_insn_state(struct insn_state *state, struct section *sec)
 {
 	memset(state, 0, sizeof(*state));
 	init_cfi_state(&state->cfi);
+
+	/*
+	 * We need the full vmlinux for noinstr validation, otherwise we can
+	 * not correctly determine insn->call_dest->sec (external symbols do
+	 * not have a section).
+	 */
+	if (vmlinux && sec)
+		state->noinstr = sec->noinstr;
 }
 
 /*
@@ -2422,24 +2430,34 @@ static int validate_branch(struct objtoo
 	return 0;
 }
 
-static int validate_unwind_hints(struct objtool_file *file)
+static int validate_unwind_hints(struct objtool_file *file, struct section *sec)
 {
 	struct instruction *insn;
-	int ret, warnings = 0;
 	struct insn_state state;
+	int ret, warnings = 0;
 
 	if (!file->hints)
 		return 0;
 
-	clear_insn_state(&state);
+	init_insn_state(&state, sec);
 
-	for_each_insn(file, insn) {
+	if (sec) {
+		insn = find_insn(file, sec, 0);
+		if (!insn)
+			return 0;
+	} else {
+		insn = list_first_entry(&file->insn_list, typeof(*insn), list);
+	}
+
+	while (&insn->list != &file->insn_list && (!sec || insn->sec == sec)) {
 		if (insn->hint && !insn->visited) {
 			ret = validate_branch(file, insn->func, insn, state);
 			if (ret && backtrace)
 				BT_FUNC("<=== (hint)", insn);
 			warnings += ret;
 		}
+
+		insn = list_next_entry(insn, list);
 	}
 
 	return warnings;
@@ -2592,21 +2610,13 @@ static int validate_section(struct objto
 	struct symbol *func;
 	int warnings = 0;
 
-	clear_insn_state(&state);
+	init_insn_state(&state, sec);
 
 	state.cfi.cfa = initial_func_cfi.cfa;
 	memcpy(&state.cfi.regs, &initial_func_cfi.regs,
 	       CFI_NUM_REGS * sizeof(struct cfi_reg));
 	state.cfi.stack_size = initial_func_cfi.cfa.offset;
 
-	/*
-	 * We need the full vmlinux for noinstr validation, otherwise we can
-	 * not correctly determine insn->call_dest->sec (external symbols do
-	 * not have a section).
-	 */
-	if (vmlinux)
-		state.noinstr = sec->noinstr;
-
 	if (state.noinstr)
 		prepare_insn_rela(file, sec);
 
@@ -2623,12 +2633,16 @@ static int validate_section(struct objto
 static int validate_vmlinux_functions(struct objtool_file *file)
 {
 	struct section *sec;
+	int warnings = 0;
 
 	sec = find_section_by_name(file->elf, ".noinstr.text");
 	if (!sec)
 		return 0;
 
-	return validate_section(file, sec);
+	warnings += validate_section(file, sec);
+	warnings += validate_unwind_hints(file, sec);
+
+	return warnings;
 }
 
 static int validate_functions(struct objtool_file *file)
@@ -2713,7 +2727,7 @@ int check(const char *_objname, bool orc
 		goto out;
 	warnings += ret;
 
-	ret = validate_unwind_hints(&file);
+	ret = validate_unwind_hints(&file, NULL);
 	if (ret < 0)
 		goto out;
 	warnings += ret;

[PATCH v4 13/13] objtool: Also consider .entry.text as noinstr

[Peter Zijlstra]

From: Thomas Gleixner <tglx@linutronix.de>

Consider all of .entry.text as noinstr. This gets us coverage across
the PTI boundary. While we could add everything .noinstr.text into
.entry.text that would bloat the amount of code in the user mapping.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 tools/objtool/check.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -285,7 +285,8 @@ static int decode_instructions(struct ob
 		    strncmp(sec->name, ".discard.", 9))
 			sec->text = true;
 
-		if (!strcmp(sec->name, ".noinstr.text"))
+		if (!strcmp(sec->name, ".noinstr.text") ||
+		    !strcmp(sec->name, ".entry.text"))
 			sec->noinstr = true;
 
 		for (offset = 0; offset < sec->len; offset += insn->len) {
@@ -2065,7 +2066,7 @@ static inline const char *call_dest_name
 static int validate_call(struct instruction *insn, struct insn_state *state)
 {
 	if (state->noinstr && state->instr <= 0 &&
-	    (!insn->call_dest || insn->call_dest->sec != insn->sec)) {
+	    (!insn->call_dest || !insn->call_dest->sec->noinstr)) {
 		WARN_FUNC("call to %s() leaves .noinstr.text section",
 				insn->sec, insn->offset, call_dest_name(insn));
 		return 1;
@@ -2636,11 +2637,16 @@ static int validate_vmlinux_functions(st
 	int warnings = 0;
 
 	sec = find_section_by_name(file->elf, ".noinstr.text");
-	if (!sec)
-		return 0;
+	if (sec) {
+		warnings += validate_section(file, sec);
+		warnings += validate_unwind_hints(file, sec);
+	}
 
-	warnings += validate_section(file, sec);
-	warnings += validate_unwind_hints(file, sec);
+	sec = find_section_by_name(file->elf, ".entry.text");
+	if (sec) {
+		warnings += validate_section(file, sec);
+		warnings += validate_unwind_hints(file, sec);
+	}
 
 	return warnings;
 }

Re: [PATCH v4 02/13] objtool: Factor out CFI hints

[Miroslav Benes]

On Wed, 25 Mar 2020, Peter Zijlstra wrote:

> Move the application of CFI hints into it's own function.
> No functional changes intended.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  tools/objtool/check.c |   67 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 38 insertions(+), 29 deletions(-)
> 
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2033,6 +2033,41 @@ static int validate_return(struct symbol
>  	return 0;
>  }
>  
> +static int apply_insn_hint(struct objtool_file *file, struct section *sec,
> +			   struct symbol *func, struct instruction *insn,
> +			   struct insn_state *state)
> +{
> +	if (insn->restore) {
> +		struct instruction *save_insn, *i;
> +
> +		i = insn;
> +		save_insn = NULL;
> +		sym_for_each_insn_continue_reverse(file, func, i) {
> +			if (i->save) {
> +				save_insn = i;
> +				break;
> +			}
> +		}
> +
> +		if (!save_insn) {
> +			WARN_FUNC("no corresponding CFI save for CFI restore",
> +				  sec, insn->offset);
> +			return 1;
> +		}
> +
> +		if (!save_insn->visited) {
> +			WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> +				  sec, insn->offset);
> +			return 1;
> +		}
> +
> +		insn->state = save_insn->state;
> +	}
> +
> +	state = insn->state;

It does not matter, because it will change later again, but there should 
be

*state = insn->state;

here, right?

> +	return 0;
> +}
> +
>  /*
>   * Follow the branch starting at the given instruction, and recursively follow
>   * any other branches (jumps).  Meanwhile, track the frame pointer state at
> @@ -2081,35 +2116,9 @@ static int validate_branch(struct objtoo
>  		}
>  
>  		if (insn->hint) {
> -			if (insn->restore) {
> -				struct instruction *save_insn, *i;
> -
> -				i = insn;
> -				save_insn = NULL;
> -				sym_for_each_insn_continue_reverse(file, func, i) {
> -					if (i->save) {
> -						save_insn = i;
> -						break;
> -					}
> -				}
> -
> -				if (!save_insn) {
> -					WARN_FUNC("no corresponding CFI save for CFI restore",
> -						  sec, insn->offset);
> -					return 1;
> -				}
> -
> -				if (!save_insn->visited) {
> -					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> -						  sec, insn->offset);
> -					return 1;
> -				}
> -
> -				insn->state = save_insn->state;
> -			}
> -
> -			state = insn->state;
> -
> +			ret = apply_insn_hint(file, sec, func, insn, &state);
> +			if (ret)
> +				return ret;
>  		} else
>  			insn->state = state;
>  
> 
> 

Re: [PATCH v4 00/13] objtool: vmlinux.o and moinstr validation

[Miroslav Benes]

On Wed, 25 Mar 2020, Peter Zijlstra wrote:

> Hi all,
> 
> This lacks the first 17 patches of the v3 posting which are en-route to
> tip/core/objtool and should show up there somewhere later this evening.
> 
> As should be familiar by now; these patches implement the noinstr
> (no-instrument) validation in objtool as requested by Thomas, to ensure
> critical code (entry for now, idle later) run no unexpected code.
> 
> Functions are marked with: noinstr, which implies notrace, noinline and sticks
> things in the .noinstr.text section. Such functions can then use instr_begin()
> and instr_end() to allow calls to code outside of this section in sanctioned
> areas.

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M

Re: [PATCH v4 02/13] objtool: Factor out CFI hints

[Peter Zijlstra]

On Wed, Mar 25, 2020 at 07:26:06PM +0100, Miroslav Benes wrote:
> On Wed, 25 Mar 2020, Peter Zijlstra wrote:
> 
> > Move the application of CFI hints into it's own function.
> > No functional changes intended.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> >  tools/objtool/check.c |   67 ++++++++++++++++++++++++++++----------------------
> >  1 file changed, 38 insertions(+), 29 deletions(-)
> > 
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -2033,6 +2033,41 @@ static int validate_return(struct symbol
> >  	return 0;
> >  }
> >  
> > +static int apply_insn_hint(struct objtool_file *file, struct section *sec,
> > +			   struct symbol *func, struct instruction *insn,
> > +			   struct insn_state *state)
> > +{
> > +	if (insn->restore) {
> > +		struct instruction *save_insn, *i;
> > +
> > +		i = insn;
> > +		save_insn = NULL;
> > +		sym_for_each_insn_continue_reverse(file, func, i) {
> > +			if (i->save) {
> > +				save_insn = i;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (!save_insn) {
> > +			WARN_FUNC("no corresponding CFI save for CFI restore",
> > +				  sec, insn->offset);
> > +			return 1;
> > +		}
> > +
> > +		if (!save_insn->visited) {
> > +			WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> > +				  sec, insn->offset);
> > +			return 1;
> > +		}
> > +
> > +		insn->state = save_insn->state;
> > +	}
> > +
> > +	state = insn->state;
> 
> It does not matter, because it will change later again, but there should 
> be
> 
> *state = insn->state;
> 
> here, right?

Argh, yes.  Let me go edit the patches.

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

On Wed, Mar 25, 2020 at 06:45:26PM +0100, Peter Zijlstra wrote:
> There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> the instruction hasn't been visited yet, it normally issues a WARN,
> except when this HINT_SAVE instruction is the first instruction of
> this branch.
> 
> This special case is of dubious correctness and is certainly unused
> (verified with an allmodconfig build), the two sites that employ
> UNWIND_HINT_SAVE/RESTORE (sync_core() and ftrace_regs_caller()) have
> the SAVE on unconditional instructions at the start of the function.
> It is therefore impossible for the save_insn not to have been visited
> when we do hit the RESTORE.

Clearly I was too tired when I did that allmodconfig build, because it
very much does generate a warning :-/.

Thank you 0day:

  kernel/sched/core.o: warning: objtool: finish_task_switch()+0x1c0: objtool isn't smart enough to handle this CFI save/restore combo

At least this gives clue as to what it was trying to do.

---
Subject: objtool: Remove CFI save/restore special case
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Mar 25 12:58:16 CET 2020

There is a special case in the UNWIND_HINT_RESTORE code. When, upon
looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
the instruction hasn't been visited yet, it normally issues a WARN,
except when this HINT_SAVE instruction is the first instruction of
this branch.

The reason for this special case comes apparent when we remove it;
code like:

	if (cond) {
		UNWIND_HINT_SAVE
		// do stuff
		UNWIND_HINT_RESTORE
	}
	// more stuff

will now trigger the warning. This is because UNWIND_HINT_RESTORE is
just a label, and there is nothing keeping it inside the (extended)
basic block covered by @cond. It will attach itself to the first
instruction of 'more stuff' and we'll hit it outside of the @cond,
confusing things.

I don't much like this special case, it confuses things and will come
apart horribly if/when the annotation needs to support nesting.
Instead extend the affected code to at least form an extended basic
block.

In particular, of the 2 users of this annotation: ftrace_regs_caller()
and sync_core(), only the latter suffers this problem. Extend it's
code sequence with a NOP to make it an extended basic block.

This isn't ideal either; stuffing code with NOPs just to make
annotations work is certainly sub-optimal, but given that sync_core()
is stupid expensive in any case, one extra nop isn't going to be a
problem here.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/processor.h |    9 ++++++++-
 tools/objtool/check.c            |   15 ++-------------
 2 files changed, 10 insertions(+), 14 deletions(-)

--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -727,6 +727,13 @@ static inline void sync_core(void)
 #else
 	unsigned int tmp;
 
+	/*
+	 * The trailing NOP is required to make this an extended basic block,
+	 * such that we can argue about it locally. Specifically this is
+	 * important for the UNWIND_HINTs, without this the UNWIND_HINT_RESTORE
+	 * can fall outside our extended basic block and objtool gets
+	 * (rightfully) confused.
+	 */
 	asm volatile (
 		UNWIND_HINT_SAVE
 		"mov %%ss, %0\n\t"
@@ -739,7 +746,7 @@ static inline void sync_core(void)
 		"pushq $1f\n\t"
 		"iretq\n\t"
 		UNWIND_HINT_RESTORE
-		"1:"
+		"1: nop\n\t"
 		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
 #endif
 }
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2000,15 +2000,14 @@ static int validate_sibling_call(struct
  * tools/objtool/Documentation/stack-validation.txt.
  */
 static int validate_branch(struct objtool_file *file, struct symbol *func,
-			   struct instruction *first, struct insn_state state)
+			   struct instruction *insn, struct insn_state state)
 {
+	struct instruction *next_insn;
 	struct alternative *alt;
-	struct instruction *insn, *next_insn;
 	struct section *sec;
 	u8 visited;
 	int ret;
 
-	insn = first;
 	sec = insn->sec;
 
 	if (insn->alt_group && list_empty(&insn->alts)) {
@@ -2061,16 +2060,6 @@ static int validate_branch(struct objtoo
 				}
 
 				if (!save_insn->visited) {
-					/*
-					 * Oops, no state to copy yet.
-					 * Hopefully we can reach this
-					 * instruction from another branch
-					 * after the save insn has been
-					 * visited.
-					 */
-					if (insn == first)
-						return 0;
-
 					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
 						  sec, insn->offset);
 					return 1;

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:

> There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> the instruction hasn't been visited yet, it normally issues a WARN,
> except when this HINT_SAVE instruction is the first instruction of
> this branch.
> 
> The reason for this special case comes apparent when we remove it;
> code like:
> 
> 	if (cond) {
> 		UNWIND_HINT_SAVE
> 		// do stuff
> 		UNWIND_HINT_RESTORE
> 	}
> 	// more stuff
> 
> will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> just a label, and there is nothing keeping it inside the (extended)
> basic block covered by @cond. It will attach itself to the first
> instruction of 'more stuff' and we'll hit it outside of the @cond,
> confusing things.
> 
> I don't much like this special case, it confuses things and will come
> apart horribly if/when the annotation needs to support nesting.
> Instead extend the affected code to at least form an extended basic
> block.
> 
> In particular, of the 2 users of this annotation: ftrace_regs_caller()
> and sync_core(), only the latter suffers this problem. Extend it's
> code sequence with a NOP to make it an extended basic block.
> 
> This isn't ideal either; stuffing code with NOPs just to make
> annotations work is certainly sub-optimal, but given that sync_core()
> is stupid expensive in any case, one extra nop isn't going to be a
> problem here.

So instr_begin() / instr_end() have this exact problem, but worse. Those
actually do nest and I've ran into the following situation:

	if (cond1) {
		instr_begin();
		// code1
		instr_end();
	}
	// code

	if (cond2) {
		instr_begin();
		// code2
		instr_end();
	}
	// tail

Where objtool then finds the path: !cond1, cond2, which ends up at code2
with 0, instead of 1.

I've also seen:

	if (cond) {
		instr_begin();
		// code1
		instr_end();
	}
	instr_begin();
	// code2
	instr_end();

Where instr_end() and instr_begin() merge onto the same instruction of
code2 as a 0, and again code2 will issue a false warning.

You can also not make objtool lift the end marker to the previous
instruction, because then:

	if (cond1) {
		instr_begin();
		if (cond2) {
			// code2
		}
		instr_end();
	}

Suffers the reverse problem, instr_end() becomes part of the @cond2
block and cond1 grows a path that misses it entirely.

So far I've not had any actual solution except adding a NOP to anchor
the annotation on.

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:
> Subject: objtool: Remove CFI save/restore special case
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Mar 25 12:58:16 CET 2020
> 
> There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> the instruction hasn't been visited yet, it normally issues a WARN,
> except when this HINT_SAVE instruction is the first instruction of
> this branch.
> 
> The reason for this special case comes apparent when we remove it;
> code like:
> 
> 	if (cond) {
> 		UNWIND_HINT_SAVE
> 		// do stuff
> 		UNWIND_HINT_RESTORE
> 	}
> 	// more stuff
> 
> will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> just a label, and there is nothing keeping it inside the (extended)
> basic block covered by @cond. It will attach itself to the first
> instruction of 'more stuff' and we'll hit it outside of the @cond,
> confusing things.
> 
> I don't much like this special case, it confuses things and will come
> apart horribly if/when the annotation needs to support nesting.
> Instead extend the affected code to at least form an extended basic
> block.

> @@ -727,6 +727,13 @@ static inline void sync_core(void)
>  #else
>  	unsigned int tmp;
>  
> +	/*
> +	 * The trailing NOP is required to make this an extended basic block,
> +	 * such that we can argue about it locally. Specifically this is
> +	 * important for the UNWIND_HINTs, without this the UNWIND_HINT_RESTORE
> +	 * can fall outside our extended basic block and objtool gets
> +	 * (rightfully) confused.
> +	 */
>  	asm volatile (
>  		UNWIND_HINT_SAVE
>  		"mov %%ss, %0\n\t"
> @@ -739,7 +746,7 @@ static inline void sync_core(void)
>  		"pushq $1f\n\t"
>  		"iretq\n\t"
>  		UNWIND_HINT_RESTORE
> -		"1:"
> +		"1: nop\n\t"
>  		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");

Note that the special case very much relies on the HINT_SAVE being the
first instruction of the (extended) basic block, which is only true in
this one usage anyway.

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Josh Poimboeuf]

On Thu, Mar 26, 2020 at 01:58:44PM +0100, Peter Zijlstra wrote:
> So instr_begin() / instr_end() have this exact problem, but worse. Those
> actually do nest and I've ran into the following situation:
> 
> 	if (cond1) {
> 		instr_begin();
> 		// code1
> 		instr_end();
> 	}
> 	// code
> 
> 	if (cond2) {
> 		instr_begin();
> 		// code2
> 		instr_end();
> 	}
> 	// tail
> 
> Where objtool then finds the path: !cond1, cond2, which ends up at code2
> with 0, instead of 1.

Hm, I don't see the nesting in this example, can you clarify?

> I've also seen:
> 
> 	if (cond) {
> 		instr_begin();
> 		// code1
> 		instr_end();
> 	}
> 	instr_begin();
> 	// code2
> 	instr_end();
> 
> Where instr_end() and instr_begin() merge onto the same instruction of
> code2 as a 0, and again code2 will issue a false warning.
> 
> You can also not make objtool lift the end marker to the previous
> instruction, because then:
> 
> 	if (cond1) {
> 		instr_begin();
> 		if (cond2) {
> 			// code2
> 		}
> 		instr_end();
> 	}
> 
> Suffers the reverse problem, instr_end() becomes part of the @cond2
> block and cond1 grows a path that misses it entirely.
> 
> So far I've not had any actual solution except adding a NOP to anchor
> the annotation on.

Are you adding the NOP to the instr_end() annotation itself?  Seems like
that would be the cleanest/easiest.

Though it is sad that we have to change the code to make objtool happy
-- would be nice if we could come up with something less intrusive.

-- 
Josh

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Josh Poimboeuf]

On Thu, Mar 26, 2020 at 12:30:49PM +0100, Peter Zijlstra wrote:
> This isn't ideal either; stuffing code with NOPs just to make
> annotations work is certainly sub-optimal, but given that sync_core()
> is stupid expensive in any case, one extra nop isn't going to be a
> problem here.

/me puts his hardened objtool maintainer's glasses on...

The problem is, when you do this kind of change to somebody else's code
-- like adding a NOP to driver code -- there's a 90% chance they'll NACK
it and tell you to fix your shit.  Because they'll be happy to tell you
the code itself should never be changed just to "make objtool happy".

And honestly, they'd be right, and there's not much you can say in
reply.  And then we end up having to fix it in objtool again anyway.

The 'insn == first' check isn't ideal, but at least it works (I think?).

Maybe there's some cleaner approach.  I'll need to think about it later
after I make some progress on the massive oil well fire I call my INBOX.

-- 
Josh

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Miroslav Benes]

On Thu, 26 Mar 2020, Peter Zijlstra wrote:

> On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:
> 
> > There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> > looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> > the instruction hasn't been visited yet, it normally issues a WARN,
> > except when this HINT_SAVE instruction is the first instruction of
> > this branch.
> > 
> > The reason for this special case comes apparent when we remove it;
> > code like:
> > 
> > 	if (cond) {
> > 		UNWIND_HINT_SAVE
> > 		// do stuff
> > 		UNWIND_HINT_RESTORE
> > 	}
> > 	// more stuff
> > 
> > will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> > just a label, and there is nothing keeping it inside the (extended)
> > basic block covered by @cond. It will attach itself to the first
> > instruction of 'more stuff' and we'll hit it outside of the @cond,
> > confusing things.
> > 
> > I don't much like this special case, it confuses things and will come
> > apart horribly if/when the annotation needs to support nesting.
> > Instead extend the affected code to at least form an extended basic
> > block.
> > 
> > In particular, of the 2 users of this annotation: ftrace_regs_caller()
> > and sync_core(), only the latter suffers this problem. Extend it's
> > code sequence with a NOP to make it an extended basic block.
> > 
> > This isn't ideal either; stuffing code with NOPs just to make
> > annotations work is certainly sub-optimal, but given that sync_core()
> > is stupid expensive in any case, one extra nop isn't going to be a
> > problem here.
> 
> So instr_begin() / instr_end() have this exact problem, but worse. Those
> actually do nest and I've ran into the following situation:
> 
> 	if (cond1) {
> 		instr_begin();
> 		// code1
> 		instr_end();
> 	}
> 	// code
> 
> 	if (cond2) {
> 		instr_begin();
> 		// code2
> 		instr_end();
> 	}
> 	// tail
> 
> Where objtool then finds the path: !cond1, cond2, which ends up at code2
> with 0, instead of 1.
> 
> I've also seen:
> 
> 	if (cond) {
> 		instr_begin();
> 		// code1
> 		instr_end();
> 	}
> 	instr_begin();
> 	// code2
> 	instr_end();
> 
> Where instr_end() and instr_begin() merge onto the same instruction of
> code2 as a 0, and again code2 will issue a false warning.
> 
> You can also not make objtool lift the end marker to the previous
> instruction, because then:
> 
> 	if (cond1) {
> 		instr_begin();
> 		if (cond2) {
> 			// code2
> 		}
> 		instr_end();
> 	}
> 
> Suffers the reverse problem, instr_end() becomes part of the @cond2
> block and cond1 grows a path that misses it entirely.

One could argue that this is really nasty and the correct way should be

	if (cond1) {
		if (cond2) {
			instr_begin();
			// code2
			instr_end();
		}
	}

Then it should work if instr_begin() marks the next instruction and 
instr_end() marks the previous one, no? There is a corner case when code2 
is exactly one instruction, so instr counting would have to be updated.

Hopefully there is a better way.

Miroslav

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Miroslav Benes]

On Thu, 26 Mar 2020, Miroslav Benes wrote:

> On Thu, 26 Mar 2020, Peter Zijlstra wrote:
> 
> > On Thu, Mar 26, 2020 at 12:30:50PM +0100, Peter Zijlstra wrote:
> > 
> > > There is a special case in the UNWIND_HINT_RESTORE code. When, upon
> > > looking for the UNWIND_HINT_SAVE instruction to restore from, it finds
> > > the instruction hasn't been visited yet, it normally issues a WARN,
> > > except when this HINT_SAVE instruction is the first instruction of
> > > this branch.
> > > 
> > > The reason for this special case comes apparent when we remove it;
> > > code like:
> > > 
> > > 	if (cond) {
> > > 		UNWIND_HINT_SAVE
> > > 		// do stuff
> > > 		UNWIND_HINT_RESTORE
> > > 	}
> > > 	// more stuff
> > > 
> > > will now trigger the warning. This is because UNWIND_HINT_RESTORE is
> > > just a label, and there is nothing keeping it inside the (extended)
> > > basic block covered by @cond. It will attach itself to the first
> > > instruction of 'more stuff' and we'll hit it outside of the @cond,
> > > confusing things.
> > > 
> > > I don't much like this special case, it confuses things and will come
> > > apart horribly if/when the annotation needs to support nesting.
> > > Instead extend the affected code to at least form an extended basic
> > > block.
> > > 
> > > In particular, of the 2 users of this annotation: ftrace_regs_caller()
> > > and sync_core(), only the latter suffers this problem. Extend it's
> > > code sequence with a NOP to make it an extended basic block.
> > > 
> > > This isn't ideal either; stuffing code with NOPs just to make
> > > annotations work is certainly sub-optimal, but given that sync_core()
> > > is stupid expensive in any case, one extra nop isn't going to be a
> > > problem here.
> > 
> > So instr_begin() / instr_end() have this exact problem, but worse. Those
> > actually do nest and I've ran into the following situation:
> > 
> > 	if (cond1) {
> > 		instr_begin();
> > 		// code1
> > 		instr_end();
> > 	}
> > 	// code
> > 
> > 	if (cond2) {
> > 		instr_begin();
> > 		// code2
> > 		instr_end();
> > 	}
> > 	// tail
> > 
> > Where objtool then finds the path: !cond1, cond2, which ends up at code2
> > with 0, instead of 1.
> > 
> > I've also seen:
> > 
> > 	if (cond) {
> > 		instr_begin();
> > 		// code1
> > 		instr_end();
> > 	}
> > 	instr_begin();
> > 	// code2
> > 	instr_end();
> > 
> > Where instr_end() and instr_begin() merge onto the same instruction of
> > code2 as a 0, and again code2 will issue a false warning.
> > 
> > You can also not make objtool lift the end marker to the previous
> > instruction, because then:
> > 
> > 	if (cond1) {
> > 		instr_begin();
> > 		if (cond2) {
> > 			// code2
> > 		}
> > 		instr_end();
> > 	}
> > 
> > Suffers the reverse problem, instr_end() becomes part of the @cond2
> > block and cond1 grows a path that misses it entirely.
> 
> One could argue that this is really nasty and the correct way should be
> 
> 	if (cond1) {
> 		if (cond2) {
> 			instr_begin();
> 			// code2
> 			instr_end();
> 		}
> 	}
> 
> Then it should work if instr_begin() marks the next instruction and 
> instr_end() marks the previous one, no? There is a corner case when code2 
> is exactly one instruction, so instr counting would have to be updated.

	if (cond1) {
		instr_begin()
		if (cond2) {
			// code2
		}
		// code1
		instr_end();
	}

is a counter example though, so I take it back.

M

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

On Thu, Mar 26, 2020 at 08:44:48AM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 26, 2020 at 01:58:44PM +0100, Peter Zijlstra wrote:
> > So instr_begin() / instr_end() have this exact problem, but worse. Those
> > actually do nest and I've ran into the following situation:
> > 
> > 	if (cond1) {
> > 		instr_begin();
> > 		// code1
> > 		instr_end();
> > 	}
> > 	// code
> > 
> > 	if (cond2) {
> > 		instr_begin();
> > 		// code2
> > 		instr_end();
> > 	}
> > 	// tail
> > 
> > Where objtool then finds the path: !cond1, cond2, which ends up at code2
> > with 0, instead of 1.
> 
> Hm, I don't see the nesting in this example, can you clarify?

Indeed no nesting here, but because they can nest we have that begin as
+1, end as -1 and then we sum it over the code flow.

Then given that, the above, ends up as -1 + 1 in the !cond1,cond2 case,
because that -1 escapes the cond1 block.

> > I've also seen:
> > 
> > 	if (cond) {
> > 		instr_begin();
> > 		// code1
> > 		instr_end();
> > 	}
> > 	instr_begin();
> > 	// code2
> > 	instr_end();
> > 
> > Where instr_end() and instr_begin() merge onto the same instruction of
> > code2 as a 0, and again code2 will issue a false warning.
> > 
> > You can also not make objtool lift the end marker to the previous
> > instruction, because then:
> > 
> > 	if (cond1) {
> > 		instr_begin();
> > 		if (cond2) {
> > 			// code2
> > 		}
> > 		instr_end();
> > 	}
> > 
> > Suffers the reverse problem, instr_end() becomes part of the @cond2
> > block and cond1 grows a path that misses it entirely.
> > 
> > So far I've not had any actual solution except adding a NOP to anchor
> > the annotation on.
> 
> Are you adding the NOP to the instr_end() annotation itself?  Seems like
> that would be the cleanest/easiest.

That actually generates a whole bunch of 'stupid' unreachable warnings.
Also, in the hope of still coming up with something saner, we've been
carrying a minimal set of explicit nop()s.

> Though it is sad that we have to change the code to make objtool happy
> -- would be nice if we could come up with something less intrusive.

Very much yes, but so far that's been eluding me.

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

On Thu, Mar 26, 2020 at 08:56:20AM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 26, 2020 at 12:30:49PM +0100, Peter Zijlstra wrote:
> > This isn't ideal either; stuffing code with NOPs just to make
> > annotations work is certainly sub-optimal, but given that sync_core()
> > is stupid expensive in any case, one extra nop isn't going to be a
> > problem here.
> 
> /me puts his hardened objtool maintainer's glasses on...
> 
> The problem is, when you do this kind of change to somebody else's code
> -- like adding a NOP to driver code -- there's a 90% chance they'll NACK
> it and tell you to fix your shit.  Because they'll be happy to tell you
> the code itself should never be changed just to "make objtool happy".

So the only objtool annotation drivers tend to run into is that uaccess
crud. Drivers really had better not need the CFI annotations, otherwise
they doing massively dodgy things.

And the nice thing with the uaccess crud is that they're anchored to
actual instructions, as opposed to this.

> And honestly, they'd be right, and there's not much you can say in
> reply.  And then we end up having to fix it in objtool again anyway.

Well, I agree. I just haven't managed to come up with anything sensible.

> The 'insn == first' check isn't ideal, but at least it works (I think?).

It works, yes, for exactly this one case.

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

On Thu, Mar 26, 2020 at 04:49:38PM +0100, Peter Zijlstra wrote:
> > The 'insn == first' check isn't ideal, but at least it works (I think?).
> 
> It works, yes, for exactly this one case.

How's this? Ignore the ignore_cfi bits, that's a 'failed' experiment.

---
 arch/x86/include/asm/orc_types.h       |   2 +
 arch/x86/include/asm/processor.h       |   6 +-
 arch/x86/include/asm/unwind_hints.h    |   4 ++
 tools/arch/x86/include/asm/orc_types.h |   2 +
 tools/objtool/check.c                  | 109 +++++++++++++++++++--------------
 tools/objtool/check.h                  |   3 +-
 6 files changed, 75 insertions(+), 51 deletions(-)

diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index 6e060907c163..82b5c685341a 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -60,6 +60,8 @@
 #define ORC_TYPE_REGS_IRET		2
 #define UNWIND_HINT_TYPE_SAVE		3
 #define UNWIND_HINT_TYPE_RESTORE	4
+#define UNWIND_HINT_TYPE_IGNORE		5
+#define UNWIND_HINT_TYPE_IRET_CONT	6

 #ifndef __ASSEMBLY__
 /*
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 94789db550df..45c74cbc0a83 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -728,8 +728,8 @@ static inline void sync_core(void)
 	unsigned int tmp;

 	asm volatile (
-		UNWIND_HINT_SAVE
 		"mov %%ss, %0\n\t"
+		UNWIND_HINT_SAVE
 		"pushq %q0\n\t"
 		"pushq %%rsp\n\t"
 		"addq $8, (%%rsp)\n\t"
@@ -737,9 +737,9 @@ static inline void sync_core(void)
 		"mov %%cs, %0\n\t"
 		"pushq %q0\n\t"
 		"pushq $1f\n\t"
+		UNWIND_HINT_IRET_CONT
 		"iretq\n\t"
-		UNWIND_HINT_RESTORE
-		"1:"
+		"1:\n\t"
 		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
 #endif
 }
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index f5e2eb12cb71..d8a07749c323 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -112,6 +112,10 @@

 #define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)

+#define UNWIND_HINT_IGNORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_IGNORE, 0)
+
+#define UNWIND_HINT_IRET_CONT UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_IRET_CONT, 0)
+
 #endif /* __ASSEMBLY__ */

 #endif /* _ASM_X86_UNWIND_HINTS_H */
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index 6e060907c163..82b5c685341a 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -60,6 +60,8 @@
 #define ORC_TYPE_REGS_IRET		2
 #define UNWIND_HINT_TYPE_SAVE		3
 #define UNWIND_HINT_TYPE_RESTORE	4
+#define UNWIND_HINT_TYPE_IGNORE		5
+#define UNWIND_HINT_TYPE_IRET_CONT	6

 #ifndef __ASSEMBLY__
 /*
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e637a4a38d2a..03bac6cb313c 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1259,14 +1259,25 @@ static int read_unwind_hints(struct objtool_file *file)

 		cfa = &insn->state.cfa;

-		if (hint->type == UNWIND_HINT_TYPE_SAVE) {
+		switch (hint->type) {
+		case UNWIND_HINT_TYPE_SAVE:
 			insn->save = true;
 			continue;

-		} else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
+		case UNWIND_HINT_TYPE_RESTORE:
 			insn->restore = true;
-			insn->hint = true;
 			continue;
+
+		case UNWIND_HINT_TYPE_IGNORE:
+			insn->ignore_cfi = true;
+			continue;
+
+		case UNWIND_HINT_TYPE_IRET_CONT:
+			insn->iret_cont = true;
+			continue;
+
+		default:
+			break;
 		}

 		insn->hint = true;
@@ -1558,6 +1569,9 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 	struct cfi_reg *cfa = &state->cfa;
 	struct cfi_reg *regs = state->regs;

+	if (insn->ignore_cfi)
+		return 0;
+
 	/* stack operations don't make sense with an undefined CFA */
 	if (cfa->base == CFI_UNDEFINED) {
 		if (insn->func) {
@@ -1993,6 +2007,37 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
 	return validate_call(insn, state);
 }

+static int insn_hint_restore(struct objtool_file *file, struct section *sec,
+			     struct symbol *func, struct instruction *insn,
+			     struct insn_state *state)
+{
+	struct instruction *save_insn, *i;
+
+	i = insn;
+	save_insn = NULL;
+	func_for_each_insn_continue_reverse(file, func, i) {
+		if (i->save) {
+			save_insn = i;
+			break;
+		}
+	}
+
+	if (!save_insn) {
+		WARN_FUNC("no corresponding CFI save for CFI restore",
+			  sec, insn->offset);
+		return 1;
+	}
+
+	if (!save_insn->visited) {
+		WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
+			  sec, insn->offset);
+		return 1;
+	}
+
+	*state = save_insn->state;
+	return 0;
+}
+
 /*
  * Follow the branch starting at the given instruction, and recursively follow
  * any other branches (jumps).  Meanwhile, track the frame pointer state at
@@ -2000,15 +2045,14 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
  * tools/objtool/Documentation/stack-validation.txt.
  */
 static int validate_branch(struct objtool_file *file, struct symbol *func,
-			   struct instruction *first, struct insn_state state)
+			   struct instruction *insn, struct insn_state state)
 {
+	struct instruction *next_insn;
 	struct alternative *alt;
-	struct instruction *insn, *next_insn;
 	struct section *sec;
 	u8 visited;
 	int ret;

-	insn = first;
 	sec = insn->sec;

 	if (insn->alt_group && list_empty(&insn->alts)) {
@@ -2034,7 +2078,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,

 		visited = 1 << state.uaccess;
 		if (insn->visited) {
-			if (!insn->hint && !insn_state_match(insn, &state))
+			if ((!insn->hint && !insn->restore) && !insn_state_match(insn, &state))
 				return 1;

 			if (insn->visited & visited)
@@ -2042,47 +2086,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 		}

 		if (insn->hint) {
-			if (insn->restore) {
-				struct instruction *save_insn, *i;
-
-				i = insn;
-				save_insn = NULL;
-				func_for_each_insn_continue_reverse(file, func, i) {
-					if (i->save) {
-						save_insn = i;
-						break;
-					}
-				}
-
-				if (!save_insn) {
-					WARN_FUNC("no corresponding CFI save for CFI restore",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				if (!save_insn->visited) {
-					/*
-					 * Oops, no state to copy yet.
-					 * Hopefully we can reach this
-					 * instruction from another branch
-					 * after the save insn has been
-					 * visited.
-					 */
-					if (insn == first)
-						return 0;
-
-					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				insn->state = save_insn->state;
-			}
-
 			state = insn->state;
-
-		} else
+		} else {
+			if (insn->restore)
+				insn_hint_restore(file, sec, func, insn, &state);
 			insn->state = state;
+		}

 		insn->visited |= visited;

@@ -2191,11 +2200,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			break;

 		case INSN_CONTEXT_SWITCH:
-			if (func && (!next_insn || !next_insn->hint)) {
+			if (insn->iret_cont) {
+				insn_hint_restore(file, sec, func, insn, &state);
+				break;
+			}
+
+			if (func) {
 				WARN_FUNC("unsupported instruction in callable function",
 					  sec, insn->offset);
 				return 1;
 			}
+
 			return 0;

 		case INSN_STACK:
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 6d875ca6fce0..f2b6172e119b 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,7 +33,8 @@ struct instruction {
 	unsigned int len;
 	enum insn_type type;
 	unsigned long immediate;
-	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+	bool alt_group, dead_end, ignore, ignore_alts;
+	bool hint, save, restore, ignore_cfi, iret_cont;
 	bool retpoline_safe;
 	u8 visited;
 	struct symbol *call_dest;

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Josh Poimboeuf]

On Thu, Mar 26, 2020 at 08:57:18PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 26, 2020 at 04:49:38PM +0100, Peter Zijlstra wrote:
> > > The 'insn == first' check isn't ideal, but at least it works (I think?).
> > 
> > It works, yes, for exactly this one case.
>
> How's this? Ignore the ignore_cfi bits, that's a 'failed' experiment.

It still seems complex to me.

What do you think about this?  If we store save_insn in the state when
we see insn->save, the restore logic becomes a lot easier.  Then if we
get a restore without a save, we can just ignore the restore hint in
that path.  Later, when we see the restore insn again from the save
path, we can then compare the insn state with the saved state to make
sure they match.

This assumes no crazy save/restore scenarios.  It also means that the
restore path has to be reachable from the save path, for which I had to
make a change to make IRETQ *not* a dead end if there's a restore hint
immediately after it.

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e637a4a38d2a..e9becd50f148 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1265,7 +1265,6 @@ static int read_unwind_hints(struct objtool_file *file)
 
 		} else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
 			insn->restore = true;
-			insn->hint = true;
 			continue;
 		}
 
@@ -2003,7 +2002,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			   struct instruction *first, struct insn_state state)
 {
 	struct alternative *alt;
-	struct instruction *insn, *next_insn;
+	struct instruction *insn, *next_insn, *save_insn = NULL;
 	struct section *sec;
 	u8 visited;
 	int ret;
@@ -2034,54 +2033,32 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 
 		visited = 1 << state.uaccess;
 		if (insn->visited) {
-			if (!insn->hint && !insn_state_match(insn, &state))
+			if (!insn->hint && !insn->restore &&
+			    !insn_state_match(insn, &state)) {
 				return 1;
+			}
+
+			if (insn->restore && save_insn) {
+				if (!insn_state_match(insn, &save_insn->state))
+					return 1;
+				save_insn = NULL;
+			}
 
 			if (insn->visited & visited)
 				return 0;
 		}
 
-		if (insn->hint) {
-			if (insn->restore) {
-				struct instruction *save_insn, *i;
-
-				i = insn;
-				save_insn = NULL;
-				func_for_each_insn_continue_reverse(file, func, i) {
-					if (i->save) {
-						save_insn = i;
-						break;
-					}
-				}
-
-				if (!save_insn) {
-					WARN_FUNC("no corresponding CFI save for CFI restore",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				if (!save_insn->visited) {
-					/*
-					 * Oops, no state to copy yet.
-					 * Hopefully we can reach this
-					 * instruction from another branch
-					 * after the save insn has been
-					 * visited.
-					 */
-					if (insn == first)
-						return 0;
-
-					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
-						  sec, insn->offset);
-					return 1;
-				}
+		if (insn->save)
+			save_insn = insn;
 
-				insn->state = save_insn->state;
-			}
+		if (insn->restore && save_insn) {
+			insn->state = save_insn->state;
+			save_insn = NULL;
+		}
 
+		if (insn->hint)
 			state = insn->state;
-
-		} else
+		else
 			insn->state = state;
 
 		insn->visited |= visited;
@@ -2191,12 +2168,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 			break;
 
 		case INSN_CONTEXT_SWITCH:
-			if (func && (!next_insn || !next_insn->hint)) {
-				WARN_FUNC("unsupported instruction in callable function",
-					  sec, insn->offset);
-				return 1;
+			if (!next_insn || !next_insn->restore) {
+				if (func) {
+					WARN_FUNC("unsupported instruction in callable function",
+							sec, insn->offset);
+					return 1;
+				}
+
+				return 0;
 			}
-			return 0;
+
+			break;
 
 		case INSN_STACK:
 			if (update_insn_state(insn, &state))
@@ -2293,7 +2275,7 @@ static int validate_unwind_hints(struct objtool_file *file)
 	clear_insn_state(&state);
 
 	for_each_insn(file, insn) {
-		if (insn->hint && !insn->visited) {
+		if ((insn->hint || insn->save || insn->restore) && !insn->visited) {
 			ret = validate_branch(file, insn->func, insn, state);
 			if (ret && backtrace)
 				BT_FUNC("<=== (hint)", insn);

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Josh Poimboeuf]

On Thu, Mar 26, 2020 at 04:38:41PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 26, 2020 at 08:44:48AM -0500, Josh Poimboeuf wrote:
> > On Thu, Mar 26, 2020 at 01:58:44PM +0100, Peter Zijlstra wrote:
> > > So instr_begin() / instr_end() have this exact problem, but worse. Those
> > > actually do nest and I've ran into the following situation:
> > > 
> > > 	if (cond1) {
> > > 		instr_begin();
> > > 		// code1
> > > 		instr_end();
> > > 	}
> > > 	// code
> > > 
> > > 	if (cond2) {
> > > 		instr_begin();
> > > 		// code2
> > > 		instr_end();
> > > 	}
> > > 	// tail
> > > 
> > > Where objtool then finds the path: !cond1, cond2, which ends up at code2
> > > with 0, instead of 1.
> > 
> > Hm, I don't see the nesting in this example, can you clarify?
> 
> Indeed no nesting here, but because they can nest we have that begin as
> +1, end as -1 and then we sum it over the code flow.
> 
> Then given that, the above, ends up as -1 + 1 in the !cond1,cond2 case,
> because that -1 escapes the cond1 block.

Ok, I see now.  I wonder if you can take a similar approach to the
save/restore patch I sent out.  Though I guess the nesting adds an
additional wrinkle I'm too tired to think about.

-- 
Josh

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

On Thu, Mar 26, 2020 at 08:00:01PM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 26, 2020 at 08:57:18PM +0100, Peter Zijlstra wrote:
> > On Thu, Mar 26, 2020 at 04:49:38PM +0100, Peter Zijlstra wrote:
> > > > The 'insn == first' check isn't ideal, but at least it works (I think?).
> > > 
> > > It works, yes, for exactly this one case.
> >
> > How's this? Ignore the ignore_cfi bits, that's a 'failed' experiment.
> 
> It still seems complex to me.
> 
> What do you think about this? 

Perhaps, but I still don't really like it. After much thinking I have
yet another proposal, see below.

> If we store save_insn in the state when
> we see insn->save, the restore logic becomes a lot easier.

Yes, that does help.

> Then if we
> get a restore without a save, we can just ignore the restore hint in
> that path.  Later, when we see the restore insn again from the save
> path, we can then compare the insn state with the saved state to make
> sure they match.

That basically removes the error.

> This assumes no crazy save/restore scenarios.

This.

> It also means that the
> restore path has to be reachable from the save path, for which I had to
> make a change to make IRETQ *not* a dead end if there's a restore hint
> immediately after it.

Right, I did that too. But that then makes me wonder why you needed that
change to validate_unwind_hints(), because if the restore hint makes it
continue, you'll not have unreachable code after it.



---
Subject: objtool: Implement RET_TAIL hint

This replaces the SAVE/RESTORE hints with a RET_TAIL hint that applies to:

 - regular RETURN and sibling calls (which are also function exists)
   it allows the stack-frame to be off by one word, ie. it allows a
   return-tail-call.

 - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
   CONTEXT_SWITCH) and here it denotes a return to self by having it
   consume arch_exception_frame_size bytes off the stack and continuing.

Apply this hint to ftrace_64.S and sync_core(), the two existing users
of the SAVE/RESTORE hints.

For ftrace_64.S we split the return path and make sure the
ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
own function.

By splitting the return path every instruction has a unique stack setup
and ORC can generate correct unwinds (XXX check if/how the ftrace
trampolines map into the ORC). Then employ the RET_TAIL hint to the
tail-call exit that has the direct-call (orig_eax) return-tail-call on.

For sync_core() annotate the IRET with RET_TAIL to mark it as a
control-flow NOP that consumes the exception frame.

XXX this adds a tail-call to ftrace_caller()
XXX compile tested only

Possibly-signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/orc_types.h       |  3 +-
 arch/x86/include/asm/processor.h       |  3 +-
 arch/x86/include/asm/unwind_hints.h    | 12 ++----
 arch/x86/kernel/ftrace.c               | 12 +++++-
 arch/x86/kernel/ftrace_64.S            | 27 +++++--------
 tools/arch/x86/include/asm/orc_types.h |  3 +-
 tools/objtool/Makefile                 |  2 +-
 tools/objtool/arch.h                   |  3 ++
 tools/objtool/arch/x86/decode.c        |  5 ++-
 tools/objtool/check.c                  | 74 ++++++++++------------------------
 tools/objtool/check.h                  |  3 +-
 11 files changed, 59 insertions(+), 88 deletions(-)

diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index 6e060907c163..49536fdd29ce 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,7 @@
 #define ORC_TYPE_CALL			0
 #define ORC_TYPE_REGS			1
 #define ORC_TYPE_REGS_IRET		2
-#define UNWIND_HINT_TYPE_SAVE		3
-#define UNWIND_HINT_TYPE_RESTORE	4
+#define UNWIND_HINT_TYPE_RET_TAIL	3
 
 #ifndef __ASSEMBLY__
 /*
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 94789db550df..cb09ee18cbab 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -728,7 +728,6 @@ static inline void sync_core(void)
 	unsigned int tmp;
 
 	asm volatile (
-		UNWIND_HINT_SAVE
 		"mov %%ss, %0\n\t"
 		"pushq %q0\n\t"
 		"pushq %%rsp\n\t"
@@ -737,8 +736,8 @@ static inline void sync_core(void)
 		"mov %%cs, %0\n\t"
 		"pushq %q0\n\t"
 		"pushq $1f\n\t"
+		UNWIND_HINT_RET_TAIL
 		"iretq\n\t"
-		UNWIND_HINT_RESTORE
 		"1:"
 		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
 #endif
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index f5e2eb12cb71..64ec9c9a6674 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -86,12 +86,8 @@
 	UNWIND_HINT sp_offset=\sp_offset
 .endm
 
-.macro UNWIND_HINT_SAVE
-	UNWIND_HINT type=UNWIND_HINT_TYPE_SAVE
-.endm
-
-.macro UNWIND_HINT_RESTORE
-	UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
+.macro UNWIND_HINT_RET_TAIL
+	UNWIND_HINT type=UNWIND_HINT_TYPE_RET_TAIL
 .endm
 
 #else /* !__ASSEMBLY__ */
@@ -108,9 +104,7 @@
 	".balign 4 \n\t"					\
 	".popsection\n\t"
 
-#define UNWIND_HINT_SAVE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_SAVE, 0)
-
-#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
+#define UNWIND_HINT_RET_TAIL UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RET_TAIL, 0)
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 37a0aeaf89e7..3fda2ee7bf71 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -282,7 +282,8 @@ static inline void tramp_free(void *tramp) { }
 
 /* Defined as markers to the end of the ftrace default trampolines */
 extern void ftrace_regs_caller_end(void);
-extern void ftrace_epilogue(void);
+extern void ftrace_regs_caller_ret(void);
+extern void ftrace_caller_end(void);
 extern void ftrace_caller_op_ptr(void);
 extern void ftrace_regs_caller_op_ptr(void);
 
@@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 		call_offset = (unsigned long)ftrace_regs_call;
 	} else {
 		start_offset = (unsigned long)ftrace_caller;
-		end_offset = (unsigned long)ftrace_epilogue;
+		end_offset = (unsigned long)ftrace_caller_end;
 		op_offset = (unsigned long)ftrace_caller_op_ptr;
 		call_offset = (unsigned long)ftrace_call;
 	}
@@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	if (WARN_ON(ret < 0))
 		goto fail;
 
+	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
+		ip = ftrace_regs_caller_ret;
+		ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
+		if (WARN_ON(ret < 0))
+			goto fail;
+	}
+
 	/*
 	 * The address of the ftrace_ops that is used for this trampoline
 	 * is stored at the end of the trampoline. This will be used to
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 369e61faacfe..28834d715817 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -157,8 +157,12 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	 * think twice before adding any new code or changing the
 	 * layout here.
 	 */
-SYM_INNER_LABEL(ftrace_epilogue, SYM_L_GLOBAL)
+SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
 
+	jmp ftrace_epilogue
+SYM_FUNC_END(ftrace_caller);
+
+SYM_FUNC_START(ftrace_epilogue)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
 	jmp ftrace_stub
@@ -170,14 +174,12 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
  */
 SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK)
 	retq
-SYM_FUNC_END(ftrace_caller)
+SYM_FUNC_END(ftrace_epilogue)
 
 SYM_FUNC_START(ftrace_regs_caller)
 	/* Save the current flags before any operations that can change them */
 	pushfq
 
-	UNWIND_HINT_SAVE
-
 	/* added 8 bytes to save flags */
 	save_mcount_regs 8
 	/* save_mcount_regs fills in first two parameters */
@@ -244,20 +246,14 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
 	movq %rax, MCOUNT_REG_SIZE(%rsp)
 
 	restore_mcount_regs 8
+	/* Restore flags */
+	popfq
 
-	jmp	2f
+SYM_INNER_LABEL(ftrace_regs_caller_ret, SYM_L_GLOBAL);
+	UNWIND_HINT_RET_TAIL
+	jmp	ftrace_epilogue
 
 1:	restore_mcount_regs
-
-
-2:
-	/*
-	 * The stack layout is nondetermistic here, depending on which path was
-	 * taken.  This confuses objtool and ORC, rightfully so.  For now,
-	 * pretend the stack always looks like the non-direct case.
-	 */
-	UNWIND_HINT_RESTORE
-
 	/* Restore flags */
 	popfq
 
@@ -268,7 +264,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
 	 * to the return.
 	 */
 SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL)
-
 	jmp ftrace_epilogue
 
 SYM_FUNC_END(ftrace_regs_caller)
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index 6e060907c163..49536fdd29ce 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,7 @@
 #define ORC_TYPE_CALL			0
 #define ORC_TYPE_REGS			1
 #define ORC_TYPE_REGS_IRET		2
-#define UNWIND_HINT_TYPE_SAVE		3
-#define UNWIND_HINT_TYPE_RESTORE	4
+#define UNWIND_HINT_TYPE_RET_TAIL	3
 
 #ifndef __ASSEMBLY__
 /*
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index ee08aeff30a1..ebbf92a8836f 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
 	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
 	    -I$(srctree)/tools/arch/$(SRCARCH)/include
 WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
-CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
+CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
 LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
 
 # Allow old libelf to be used:
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index ced3765c4f44..6d04a683f8cc 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -19,6 +19,7 @@ enum insn_type {
 	INSN_CALL,
 	INSN_CALL_DYNAMIC,
 	INSN_RETURN,
+	INSN_EXCEPTION_RETURN,
 	INSN_CONTEXT_SWITCH,
 	INSN_STACK,
 	INSN_BUG,
@@ -73,6 +74,8 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 			    unsigned int *len, enum insn_type *type,
 			    unsigned long *immediate, struct stack_op *op);
 
+static const int arch_exception_frame_size = 5*8;
+
 bool arch_callee_saved_reg(unsigned char reg);
 
 #endif /* _ARCH_H */
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index a62e032863a8..1f9dd97e4a50 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -431,10 +431,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 
 	case 0xca: /* retf */
 	case 0xcb: /* retf */
-	case 0xcf: /* iret */
 		*type = INSN_CONTEXT_SWITCH;
 		break;
 
+	case 0xcf: /* iret */
+		*type = INSN_EXCEPTION_RETURN;
+		break;
+
 	case 0xe8:
 		*type = INSN_CALL;
 		break;
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e637a4a38d2a..6eb9ae48c94f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1259,13 +1259,8 @@ static int read_unwind_hints(struct objtool_file *file)
 
 		cfa = &insn->state.cfa;
 
-		if (hint->type == UNWIND_HINT_TYPE_SAVE) {
-			insn->save = true;
-			continue;
-
-		} else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
-			insn->restore = true;
-			insn->hint = true;
+		if (hint->type == UNWIND_HINT_TYPE_RET_TAIL) {
+			insn->ret_tail = true;
 			continue;
 		}
 
@@ -1429,16 +1424,22 @@ static bool is_fentry_call(struct instruction *insn)
 	return false;
 }
 
-static bool has_modified_stack_frame(struct insn_state *state)
+static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
 {
 	int i;
 
 	if (state->cfa.base != initial_func_cfi.cfa.base ||
-	    state->cfa.offset != initial_func_cfi.cfa.offset ||
-	    state->stack_size != initial_func_cfi.cfa.offset ||
 	    state->drap)
 		return true;
 
+	if (state->cfa.offset != initial_func_cfi.cfa.offset ||
+	    !(insn->ret_tail && state->cfa.offset == initial_func_cfi.cfa.offset + 8))
+
+
+	if (state->stack_size != initial_func_cfi.cfa.offset &&
+	    !(insn->ret_tail && state->stack_size == initial_func_cfi.cfa.offset + 8))
+		return true;
+
 	for (i = 0; i < CFI_NUM_REGS; i++)
 		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
 		    state->regs[i].offset != initial_func_cfi.regs[i].offset)
@@ -1984,7 +1985,7 @@ static int validate_call(struct instruction *insn, struct insn_state *state)
 
 static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
 {
-	if (has_modified_stack_frame(state)) {
+	if (has_modified_stack_frame(insn, state)) {
 		WARN_FUNC("sibling call from callable instruction with modified stack frame",
 				insn->sec, insn->offset);
 		return 1;
@@ -2041,47 +2042,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				return 0;
 		}
 
-		if (insn->hint) {
-			if (insn->restore) {
-				struct instruction *save_insn, *i;
-
-				i = insn;
-				save_insn = NULL;
-				func_for_each_insn_continue_reverse(file, func, i) {
-					if (i->save) {
-						save_insn = i;
-						break;
-					}
-				}
-
-				if (!save_insn) {
-					WARN_FUNC("no corresponding CFI save for CFI restore",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				if (!save_insn->visited) {
-					/*
-					 * Oops, no state to copy yet.
-					 * Hopefully we can reach this
-					 * instruction from another branch
-					 * after the save insn has been
-					 * visited.
-					 */
-					if (insn == first)
-						return 0;
-
-					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				insn->state = save_insn->state;
-			}
-
+		if (insn->hint)
 			state = insn->state;
-
-		} else
+		else
 			insn->state = state;
 
 		insn->visited |= visited;
@@ -2123,7 +2086,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				return 1;
 			}
 
-			if (func && has_modified_stack_frame(&state)) {
+			if (func && has_modified_stack_frame(insn, &state)) {
 				WARN_FUNC("return with modified stack frame",
 					  sec, insn->offset);
 				return 1;
@@ -2190,6 +2153,13 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 
 			break;
 
+		case INSN_EXCEPTION_RETURN:
+			if (insn->ret_tail) {
+				state.stack_size -= arch_exception_frame_size;
+				break;
+			}
+
+			/* fallthrough */
 		case INSN_CONTEXT_SWITCH:
 			if (func && (!next_insn || !next_insn->hint)) {
 				WARN_FUNC("unsupported instruction in callable function",
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 6d875ca6fce0..138533ac23e2 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,7 +33,8 @@ struct instruction {
 	unsigned int len;
 	enum insn_type type;
 	unsigned long immediate;
-	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+	bool alt_group, dead_end, ignore, ignore_alts;
+	bool hint, ret_tail;
 	bool retpoline_safe;
 	u8 visited;
 	struct symbol *call_dest;

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Josh Poimboeuf]

On Mon, Mar 30, 2020 at 07:02:00PM +0200, Peter Zijlstra wrote:
> Subject: objtool: Implement RET_TAIL hint
> 
> This replaces the SAVE/RESTORE hints with a RET_TAIL hint that applies to:
> 
>  - regular RETURN and sibling calls (which are also function exists)
>    it allows the stack-frame to be off by one word, ie. it allows a
>    return-tail-call.
> 
>  - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
>    CONTEXT_SWITCH) and here it denotes a return to self by having it
>    consume arch_exception_frame_size bytes off the stack and continuing.
> 
> Apply this hint to ftrace_64.S and sync_core(), the two existing users
> of the SAVE/RESTORE hints.
> 
> For ftrace_64.S we split the return path and make sure the
> ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
> own function.
> 
> By splitting the return path every instruction has a unique stack setup
> and ORC can generate correct unwinds (XXX check if/how the ftrace
> trampolines map into the ORC). Then employ the RET_TAIL hint to the
> tail-call exit that has the direct-call (orig_eax) return-tail-call on.
> 
> For sync_core() annotate the IRET with RET_TAIL to mark it as a
> control-flow NOP that consumes the exception frame.

I do like the idea to get rid of SAVE/RESTORE altogether.  And it's nice
to make that ftrace code unwinder-deterministic.

However sync_core() and ftrace_regs_caller() are very different from
each other and I find the RET_TAIL hint usage to be extremely confusing.

For example, IRETQ isn't even a tail cail.

And the need for the hint to come *before* the insn which changes the
state is different from the other hints.

And now objtool has to know the arch exception stack size because of a
single code site.

And for a proper tail call, the stack should be empty.  I don't
understand the +8 thing in has_modified_stack_frame().  It seems
hard-coded for the weird ftrace case, rather than for tail calls in
general (which should already work as designed).

How about a more general hint like UNWIND_HINT_ADJUST?

For sync_core(), after the IRETQ:

  UNWIND_HINT_ADJUST sp_add=40

And ftrace_regs_caller_ret could have:

  UNWIND_HINT_ADJUST sp_add=8
  
-- 
Josh

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

On Mon, Mar 30, 2020 at 02:02:05PM -0500, Josh Poimboeuf wrote:
> On Mon, Mar 30, 2020 at 07:02:00PM +0200, Peter Zijlstra wrote:
> > Subject: objtool: Implement RET_TAIL hint
> > 
> > This replaces the SAVE/RESTORE hints with a RET_TAIL hint that applies to:
> > 
> >  - regular RETURN and sibling calls (which are also function exists)
> >    it allows the stack-frame to be off by one word, ie. it allows a
> >    return-tail-call.
> > 
> >  - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
> >    CONTEXT_SWITCH) and here it denotes a return to self by having it
> >    consume arch_exception_frame_size bytes off the stack and continuing.
> > 
> > Apply this hint to ftrace_64.S and sync_core(), the two existing users
> > of the SAVE/RESTORE hints.
> > 
> > For ftrace_64.S we split the return path and make sure the
> > ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
> > own function.
> > 
> > By splitting the return path every instruction has a unique stack setup
> > and ORC can generate correct unwinds (XXX check if/how the ftrace
> > trampolines map into the ORC). Then employ the RET_TAIL hint to the
> > tail-call exit that has the direct-call (orig_eax) return-tail-call on.
> > 
> > For sync_core() annotate the IRET with RET_TAIL to mark it as a
> > control-flow NOP that consumes the exception frame.
> 
> I do like the idea to get rid of SAVE/RESTORE altogether.  And it's nice
> to make that ftrace code unwinder-deterministic.
> 
> However sync_core() and ftrace_regs_caller() are very different from
> each other and I find the RET_TAIL hint usage to be extremely confusing.

I was going with the pattern:

	push target
	ret

which is an indirect tail-call that doesn't need a register. We use it
in various places. We use it here exactly because it preserves all
registers, but we use it in function-graph tracer and retprobes to
insert the return handler. But also in retpoline, because it uses the
return stack predictor, which by happy accident isn't the indirect
branch predictor.

> For example, IRETQ isn't even a tail cail.

It's the same indirect call, except with a bigger frame ;-)

	push # ss
	push # rsp
	push # flags
	push # cs
	push # ip
	iret

> And the need for the hint to come *before* the insn which changes the
> state is different from the other hints.

makes sense to me... but yah.

> And now objtool has to know the arch exception stack size because of a
> single code site.

Agreed.

> And for a proper tail call, the stack should be empty. 

All depends what you call proper :-)

> I don't
> understand the +8 thing in has_modified_stack_frame().

	push target
	ret

means we hit ret with one extra word on the stack.

> It seems
> hard-coded for the weird ftrace case, rather than for tail calls in
> general (which should already work as designed).

Like I said, we have it all over the place, but I suspect they're all
mostly hidden from objtool.

> How about a more general hint like UNWIND_HINT_ADJUST?
> 
> For sync_core(), after the IRETQ:
> 
>   UNWIND_HINT_ADJUST sp_add=40
> 
> And ftrace_regs_caller_ret could have:
> 
>   UNWIND_HINT_ADJUST sp_add=8

I like, I'll make it happen in the morning.

Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case

[Peter Zijlstra]

On Mon, Mar 30, 2020 at 10:02:54PM +0200, Peter Zijlstra wrote:
> On Mon, Mar 30, 2020 at 02:02:05PM -0500, Josh Poimboeuf wrote:
> > However sync_core() and ftrace_regs_caller() are very different from
> > each other and I find the RET_TAIL hint usage to be extremely confusing.
> 
> I was going with the pattern:
> 
> 	push target
> 	ret
> 
> which is an indirect tail-call that doesn't need a register. We use it
> in various places. We use it here exactly because it preserves all
> registers, but we use it in function-graph tracer and retprobes to
> insert the return handler. But also in retpoline, because it uses the
> return stack predictor, which by happy accident isn't the indirect
> branch predictor.
> 
> > For example, IRETQ isn't even a tail cail.
> 
> It's the same indirect call, except with a bigger frame ;-)
> 
> 	push # ss
> 	push # rsp
> 	push # flags
> 	push # cs
> 	push # ip
> 	iret
> 
> > And the need for the hint to come *before* the insn which changes the
> > state is different from the other hints.
> 
> makes sense to me... but yah.

Also, naturally, there are no instructions after RET to stick the
annotation to.

[RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Mon, Mar 30, 2020 at 10:02:54PM +0200, Peter Zijlstra wrote:
> On Mon, Mar 30, 2020 at 02:02:05PM -0500, Josh Poimboeuf wrote:

> > How about a more general hint like UNWIND_HINT_ADJUST?
> > 
> > For sync_core(), after the IRETQ:
> > 
> >   UNWIND_HINT_ADJUST sp_add=40
> > 
> > And ftrace_regs_caller_ret could have:
> > 
> >   UNWIND_HINT_ADJUST sp_add=8
> 
> I like, I'll make it happen in the morning.

Still compile tested only. But I did find orc_ftrace_find() which should
indeed allow ORC unwinding of the ftrace trampolines.

---
Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that applies
to the following instructions:

 - any instruction that terminates a function, like: RETURN and sibling
   calls. It allows the stack-frame to be off by @sp_offset, ie. it
   allows stuffing the return stack.

 - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
   CONTEXT_SWITCH) and here it denotes a @sp_offset sized POP and makes
   the instruction continue.

Apply this hint to ftrace_64.S and sync_core(), the two existing users
of the SAVE/RESTORE hints.

For ftrace_64.S we split the return path and make sure the
ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
own function.

By splitting the return path every instruction has a unique stack setup
and ORC can generate correct unwinds. Then employ the RET_OFFSET hint to
the tail-call exit that has the direct-call (orig_eax) stuffed on the
return stack.

For sync_core() annotate the IRET with RET_OFFSET to mark it as a
control-flow NOP that consumes the exception frame.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/orc_types.h       | 11 ++++-
 arch/x86/include/asm/processor.h       |  3 +-
 arch/x86/include/asm/unwind_hints.h    | 12 ++----
 arch/x86/kernel/ftrace.c               | 12 +++++-
 arch/x86/kernel/ftrace_64.S            | 31 ++++++--------
 tools/arch/x86/include/asm/orc_types.h | 11 ++++-
 tools/objtool/Makefile                 |  2 +-
 tools/objtool/arch.h                   |  1 +
 tools/objtool/arch/x86/decode.c        |  5 ++-
 tools/objtool/check.c                  | 75 +++++++++++-----------------------
 tools/objtool/check.h                  |  4 +-
 11 files changed, 77 insertions(+), 90 deletions(-)

diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h
index 6e060907c163..0133ec8d19b6 100644
--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,15 @@
 #define ORC_TYPE_CALL			0
 #define ORC_TYPE_REGS			1
 #define ORC_TYPE_REGS_IRET		2
-#define UNWIND_HINT_TYPE_SAVE		3
-#define UNWIND_HINT_TYPE_RESTORE	4
+
+/*
+ * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
+ * and sibling calls. On these, sp_offset denotes the expected offset from
+ * initial_func_cfi. It can also be used on EXCEPTION_RETURN instructions
+ * where sp_offset will denote the exception frame size consumed and will
+ * make objtool assume the instruction is a fall-through.
+ */
+#define UNWIND_HINT_TYPE_RET_OFFSET	3
 
 #ifndef __ASSEMBLY__
 /*
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 94789db550df..c8e0496e25dd 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -728,7 +728,6 @@ static inline void sync_core(void)
 	unsigned int tmp;
 
 	asm volatile (
-		UNWIND_HINT_SAVE
 		"mov %%ss, %0\n\t"
 		"pushq %q0\n\t"
 		"pushq %%rsp\n\t"
@@ -737,8 +736,8 @@ static inline void sync_core(void)
 		"mov %%cs, %0\n\t"
 		"pushq %q0\n\t"
 		"pushq $1f\n\t"
+		UNWIND_HINT_RET_OFFSET(5*8)
 		"iretq\n\t"
-		UNWIND_HINT_RESTORE
 		"1:"
 		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
 #endif
diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index f5e2eb12cb71..c4acd99716c1 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -86,12 +86,8 @@
 	UNWIND_HINT sp_offset=\sp_offset
 .endm
 
-.macro UNWIND_HINT_SAVE
-	UNWIND_HINT type=UNWIND_HINT_TYPE_SAVE
-.endm
-
-.macro UNWIND_HINT_RESTORE
-	UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
+.macro UNWIND_HINT_RET_OFFSET sp_offset=8
+	UNWIND_HINT type=UNWIND_HINT_TYPE_RET_OFFSET sp_offset=\sp_offset
 .endm
 
 #else /* !__ASSEMBLY__ */
@@ -108,9 +104,7 @@
 	".balign 4 \n\t"					\
 	".popsection\n\t"
 
-#define UNWIND_HINT_SAVE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_SAVE, 0)
-
-#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
+#define UNWIND_HINT_RET_OFFSET(offset) UNWIND_HINT(0, (offset), UNWIND_HINT_TYPE_RET_OFFSET, 0)
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 37a0aeaf89e7..3fda2ee7bf71 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -282,7 +282,8 @@ static inline void tramp_free(void *tramp) { }
 
 /* Defined as markers to the end of the ftrace default trampolines */
 extern void ftrace_regs_caller_end(void);
-extern void ftrace_epilogue(void);
+extern void ftrace_regs_caller_ret(void);
+extern void ftrace_caller_end(void);
 extern void ftrace_caller_op_ptr(void);
 extern void ftrace_regs_caller_op_ptr(void);
 
@@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 		call_offset = (unsigned long)ftrace_regs_call;
 	} else {
 		start_offset = (unsigned long)ftrace_caller;
-		end_offset = (unsigned long)ftrace_epilogue;
+		end_offset = (unsigned long)ftrace_caller_end;
 		op_offset = (unsigned long)ftrace_caller_op_ptr;
 		call_offset = (unsigned long)ftrace_call;
 	}
@@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	if (WARN_ON(ret < 0))
 		goto fail;
 
+	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
+		ip = ftrace_regs_caller_ret;
+		ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
+		if (WARN_ON(ret < 0))
+			goto fail;
+	}
+
 	/*
 	 * The address of the ftrace_ops that is used for this trampoline
 	 * is stored at the end of the trampoline. This will be used to
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 369e61faacfe..1e4a82ff97ea 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -157,8 +157,12 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	 * think twice before adding any new code or changing the
 	 * layout here.
 	 */
-SYM_INNER_LABEL(ftrace_epilogue, SYM_L_GLOBAL)
+SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
 
+	jmp ftrace_epilogue
+SYM_FUNC_END(ftrace_caller);
+
+SYM_FUNC_START(ftrace_epilogue)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
 	jmp ftrace_stub
@@ -170,14 +174,12 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
  */
 SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK)
 	retq
-SYM_FUNC_END(ftrace_caller)
+SYM_FUNC_END(ftrace_epilogue)
 
 SYM_FUNC_START(ftrace_regs_caller)
 	/* Save the current flags before any operations that can change them */
 	pushfq
 
-	UNWIND_HINT_SAVE
-
 	/* added 8 bytes to save flags */
 	save_mcount_regs 8
 	/* save_mcount_regs fills in first two parameters */
@@ -235,8 +237,8 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
 
 	/* If ORIG_RAX is anything but zero, make this a call to that */
 	movq ORIG_RAX(%rsp), %rax
-	cmpq	$0, %rax
-	je	1f
+	testq	%rax, %rax
+	jz	1f
 
 	/* Swap the flags with orig_rax */
 	movq MCOUNT_REG_SIZE(%rsp), %rdi
@@ -244,20 +246,14 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
 	movq %rax, MCOUNT_REG_SIZE(%rsp)
 
 	restore_mcount_regs 8
+	/* Restore flags */
+	popfq
 
-	jmp	2f
+SYM_INNER_LABEL(ftrace_regs_caller_ret, SYM_L_GLOBAL);
+	UNWIND_HINT_RET_OFFSET
+	jmp	ftrace_epilogue
 
 1:	restore_mcount_regs
-
-
-2:
-	/*
-	 * The stack layout is nondetermistic here, depending on which path was
-	 * taken.  This confuses objtool and ORC, rightfully so.  For now,
-	 * pretend the stack always looks like the non-direct case.
-	 */
-	UNWIND_HINT_RESTORE
-
 	/* Restore flags */
 	popfq
 
@@ -268,7 +264,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
 	 * to the return.
 	 */
 SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL)
-
 	jmp ftrace_epilogue
 
 SYM_FUNC_END(ftrace_regs_caller)
diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h
index 6e060907c163..0133ec8d19b6 100644
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,15 @@
 #define ORC_TYPE_CALL			0
 #define ORC_TYPE_REGS			1
 #define ORC_TYPE_REGS_IRET		2
-#define UNWIND_HINT_TYPE_SAVE		3
-#define UNWIND_HINT_TYPE_RESTORE	4
+
+/*
+ * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
+ * and sibling calls. On these, sp_offset denotes the expected offset from
+ * initial_func_cfi. It can also be used on EXCEPTION_RETURN instructions
+ * where sp_offset will denote the exception frame size consumed and will
+ * make objtool assume the instruction is a fall-through.
+ */
+#define UNWIND_HINT_TYPE_RET_OFFSET	3
 
 #ifndef __ASSEMBLY__
 /*
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index ee08aeff30a1..ebbf92a8836f 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
 	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
 	    -I$(srctree)/tools/arch/$(SRCARCH)/include
 WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
-CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
+CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
 LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
 
 # Allow old libelf to be used:
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index ced3765c4f44..cba72e1c47ce 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -19,6 +19,7 @@ enum insn_type {
 	INSN_CALL,
 	INSN_CALL_DYNAMIC,
 	INSN_RETURN,
+	INSN_EXCEPTION_RETURN,
 	INSN_CONTEXT_SWITCH,
 	INSN_STACK,
 	INSN_BUG,
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index a62e032863a8..1f9dd97e4a50 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -431,10 +431,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
 
 	case 0xca: /* retf */
 	case 0xcb: /* retf */
-	case 0xcf: /* iret */
 		*type = INSN_CONTEXT_SWITCH;
 		break;
 
+	case 0xcf: /* iret */
+		*type = INSN_EXCEPTION_RETURN;
+		break;
+
 	case 0xe8:
 		*type = INSN_CALL;
 		break;
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index e637a4a38d2a..502062803cce 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1259,13 +1259,8 @@ static int read_unwind_hints(struct objtool_file *file)
 
 		cfa = &insn->state.cfa;
 
-		if (hint->type == UNWIND_HINT_TYPE_SAVE) {
-			insn->save = true;
-			continue;
-
-		} else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
-			insn->restore = true;
-			insn->hint = true;
+		if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
+			insn->ret_offset = hint->sp_offset;
 			continue;
 		}
 
@@ -1429,16 +1424,23 @@ static bool is_fentry_call(struct instruction *insn)
 	return false;
 }
 
-static bool has_modified_stack_frame(struct insn_state *state)
+static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
 {
+	u8 ret_offset = insn->ret_offset;
 	int i;
 
 	if (state->cfa.base != initial_func_cfi.cfa.base ||
-	    state->cfa.offset != initial_func_cfi.cfa.offset ||
-	    state->stack_size != initial_func_cfi.cfa.offset ||
 	    state->drap)
 		return true;
 
+	if (state->cfa.offset != initial_func_cfi.cfa.offset &&
+	    !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
+		return true;
+
+	if (state->stack_size != initial_func_cfi.cfa.offset &&
+	    !(ret_offset && state->stack_size == initial_func_cfi.cfa.offset + ret_offset))
+		return true;
+
 	for (i = 0; i < CFI_NUM_REGS; i++)
 		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
 		    state->regs[i].offset != initial_func_cfi.regs[i].offset)
@@ -1984,7 +1986,7 @@ static int validate_call(struct instruction *insn, struct insn_state *state)
 
 static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
 {
-	if (has_modified_stack_frame(state)) {
+	if (has_modified_stack_frame(insn, state)) {
 		WARN_FUNC("sibling call from callable instruction with modified stack frame",
 				insn->sec, insn->offset);
 		return 1;
@@ -2041,47 +2043,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				return 0;
 		}
 
-		if (insn->hint) {
-			if (insn->restore) {
-				struct instruction *save_insn, *i;
-
-				i = insn;
-				save_insn = NULL;
-				func_for_each_insn_continue_reverse(file, func, i) {
-					if (i->save) {
-						save_insn = i;
-						break;
-					}
-				}
-
-				if (!save_insn) {
-					WARN_FUNC("no corresponding CFI save for CFI restore",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				if (!save_insn->visited) {
-					/*
-					 * Oops, no state to copy yet.
-					 * Hopefully we can reach this
-					 * instruction from another branch
-					 * after the save insn has been
-					 * visited.
-					 */
-					if (insn == first)
-						return 0;
-
-					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				insn->state = save_insn->state;
-			}
-
+		if (insn->hint)
 			state = insn->state;
-
-		} else
+		else
 			insn->state = state;
 
 		insn->visited |= visited;
@@ -2123,7 +2087,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 				return 1;
 			}
 
-			if (func && has_modified_stack_frame(&state)) {
+			if (func && has_modified_stack_frame(insn, &state)) {
 				WARN_FUNC("return with modified stack frame",
 					  sec, insn->offset);
 				return 1;
@@ -2190,6 +2154,13 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 
 			break;
 
+		case INSN_EXCEPTION_RETURN:
+			if (insn->ret_offset) {
+				state.stack_size -= insn->ret_offset;
+				break;
+			}
+
+			/* fallthrough */
 		case INSN_CONTEXT_SWITCH:
 			if (func && (!next_insn || !next_insn->hint)) {
 				WARN_FUNC("unsupported instruction in callable function",
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index 6d875ca6fce0..aaccaef8f074 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,9 +33,11 @@ struct instruction {
 	unsigned int len;
 	enum insn_type type;
 	unsigned long immediate;
-	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+	bool alt_group, dead_end, ignore, ignore_alts;
+	bool hint;
 	bool retpoline_safe;
 	u8 visited;
+	u8 ret_offset;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct instruction *first_jump_src;

Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Steven Rostedt]

On Tue, 31 Mar 2020 13:16:52 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> @@ -235,8 +237,8 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
>  
>  	/* If ORIG_RAX is anything but zero, make this a call to that */
>  	movq ORIG_RAX(%rsp), %rax
> -	cmpq	$0, %rax
> -	je	1f
> +	testq	%rax, %rax
> +	jz	1f
>  
>  	/* Swap the flags with orig_rax */
>  	movq MCOUNT_REG_SIZE(%rsp), %rdi

Hi Peter,

Can you send this change as a separate patch as it has nothing to do with
this current change, and is a clean up patch that stands on its own.

Thanks,

-- Steve

[RFC][PATCH] x86,ftrace: Shrink ftrace_regs_caller() by one byte

[Peter Zijlstra]

On Tue, Mar 31, 2020 at 11:31:36AM -0400, Steven Rostedt wrote:
> On Tue, 31 Mar 2020 13:16:52 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > @@ -235,8 +237,8 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> >  
> >  	/* If ORIG_RAX is anything but zero, make this a call to that */
> >  	movq ORIG_RAX(%rsp), %rax
> > -	cmpq	$0, %rax
> > -	je	1f
> > +	testq	%rax, %rax
> > +	jz	1f
> >  
> >  	/* Swap the flags with orig_rax */
> >  	movq MCOUNT_REG_SIZE(%rsp), %rdi
> 
> Hi Peter,
> 
> Can you send this change as a separate patch as it has nothing to do with
> this current change, and is a clean up patch that stands on its own.

Sure. But then I have to like write a Changelog for it... :/

---
Subject: x86,ftrace: Shrink ftrace_regs_caller() by one byte

'Optimize' ftrace_regs_caller. Instead of comparing against an
immediate, the more natural way to test for zero on x86 is: 'test
%r,%r'.

  48 83 f8 00             cmp    $0x0,%rax
  74 49                   je     226 <ftrace_regs_call+0xa3>

  48 85 c0                test   %rax,%rax
  74 49                   je     225 <ftrace_regs_call+0xa2>

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

diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 369e61faacfe..8e71c492d623 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -235,8 +235,8 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
 
 	/* If ORIG_RAX is anything but zero, make this a call to that */
 	movq ORIG_RAX(%rsp), %rax
-	cmpq	$0, %rax
-	je	1f
+	testq	%rax, %rax
+	jz	1f
 
 	/* Swap the flags with orig_rax */
 	movq MCOUNT_REG_SIZE(%rsp), %rdi

Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Tue, Mar 31, 2020 at 11:31:36AM -0400, Steven Rostedt wrote:

> Can you send this change as a separate patch as it has nothing to do with
> this current change, and is a clean up patch that stands on its own.

I also found this.. should I write it up?

---
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 369e61faacfe..0f108096f278 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -23,7 +23,7 @@
 #endif /* CONFIG_FRAME_POINTER */

 /* Size of stack used to save mcount regs in save_mcount_regs */
-#define MCOUNT_REG_SIZE		(SS+8 + MCOUNT_FRAME_SIZE)
+#define MCOUNT_REG_SIZE		(SIZEOF_PTREGS + MCOUNT_FRAME_SIZE)

 /*
  * gcc -pg option adds a call to 'mcount' in most functions.
@@ -77,7 +77,7 @@
 	/*
 	 * We add enough stack to save all regs.
 	 */
-	subq $(MCOUNT_REG_SIZE - MCOUNT_FRAME_SIZE), %rsp
+	subq $(SIZEOF_PTREGS), %rsp
 	movq %rax, RAX(%rsp)
 	movq %rcx, RCX(%rsp)
 	movq %rdx, RDX(%rsp)

Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Josh Poimboeuf]

On Tue, Mar 31, 2020 at 01:16:52PM +0200, Peter Zijlstra wrote:
> Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
> 
> This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that applies
> to the following instructions:
> 
>  - any instruction that terminates a function, like: RETURN and sibling
>    calls. It allows the stack-frame to be off by @sp_offset, ie. it
>    allows stuffing the return stack.
> 
>  - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
>    CONTEXT_SWITCH) and here it denotes a @sp_offset sized POP and makes
>    the instruction continue.

Looking closer, I see how my UNWIND_HINT_ADJUST idea doesn't work for
the ftrace_regs_caller() case.  The ORC data is actually correct there.
So basically we need a way to tell objtool to be quiet.

I now understand what you're trying to do with the RET_TAIL thing, and I
guess it's ok for the ftrace case.  But I'd rather an UNWIND_HINT_IGNORE
before the tail cail, which would tell objtool to just silence the tail
call warning.  It's simpler for the user to understand, it's simpler
logic in objtool, and I think an "ignore warnings for the next insn"
hint would be more generally applicable anyway.

But also... the RET_OFFSET usage for sync_core() *really* bugs me.

I know you said it's like an indirect tail call with a bigger frame, but
that's kind of stretching it because the function frame is still there.

And objtool doesn't treat it like a tail call at all.  In fact, it
handles it *completely* differently from the normal ret-tail-call case.
Instead of silencing a tail call warning, it adjusts the stack offset
and continues the code path.

This basically adds *two* new hint types, while trying to call them the
same thing.  There's no overlapping functionality between them in
objtool, other than the use of the same insn->ret_offset variable.  But
it's two distinct functionalities, depending on the context (return/tail
vs IRETQ).

I'll try to work up some patches with a different approach in a bit.

-- 
Josh

Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Josh Poimboeuf]

On Tue, Mar 31, 2020 at 09:58:41PM +0200, Peter Zijlstra wrote:
> On Tue, Mar 31, 2020 at 11:31:36AM -0400, Steven Rostedt wrote:
> 
> > Can you send this change as a separate patch as it has nothing to do with
> > this current change, and is a clean up patch that stands on its own.
> 
> I also found this.. should I write it up?
> 
> ---
> diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> index 369e61faacfe..0f108096f278 100644
> --- a/arch/x86/kernel/ftrace_64.S
> +++ b/arch/x86/kernel/ftrace_64.S
> @@ -23,7 +23,7 @@
>  #endif /* CONFIG_FRAME_POINTER */
> 
>  /* Size of stack used to save mcount regs in save_mcount_regs */
> -#define MCOUNT_REG_SIZE		(SS+8 + MCOUNT_FRAME_SIZE)
> +#define MCOUNT_REG_SIZE		(SIZEOF_PTREGS + MCOUNT_FRAME_SIZE)
> 
>  /*
>   * gcc -pg option adds a call to 'mcount' in most functions.
> @@ -77,7 +77,7 @@
>  	/*
>  	 * We add enough stack to save all regs.
>  	 */
> -	subq $(MCOUNT_REG_SIZE - MCOUNT_FRAME_SIZE), %rsp
> +	subq $(SIZEOF_PTREGS), %rsp
>  	movq %rax, RAX(%rsp)
>  	movq %rcx, RCX(%rsp)
>  	movq %rdx, RDX(%rsp)

I was going to suggest the same thing :-)

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Tue, Mar 31, 2020 at 03:23:15PM -0500, Josh Poimboeuf wrote:
> On Tue, Mar 31, 2020 at 01:16:52PM +0200, Peter Zijlstra wrote:
> > Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
> > 
> > This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that applies
> > to the following instructions:
> > 
> >  - any instruction that terminates a function, like: RETURN and sibling
> >    calls. It allows the stack-frame to be off by @sp_offset, ie. it
> >    allows stuffing the return stack.
> > 
> >  - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
> >    CONTEXT_SWITCH) and here it denotes a @sp_offset sized POP and makes
> >    the instruction continue.
> 
> Looking closer, I see how my UNWIND_HINT_ADJUST idea doesn't work for
> the ftrace_regs_caller() case.  The ORC data is actually correct there.
> So basically we need a way to tell objtool to be quiet.

Right.

> I now understand what you're trying to do with the RET_TAIL thing, and I
> guess it's ok for the ftrace case.  But I'd rather an UNWIND_HINT_IGNORE
> before the tail cail, which would tell objtool to just silence the tail
> call warning.  It's simpler for the user to understand, it's simpler
> logic in objtool, and I think an "ignore warnings for the next insn"
> hint would be more generally applicable anyway.

I like how this is specific on how far the stack can be off, as opposed
so say 'ignore any warning on this instruction'.

Because by saying this RET should be +8, we'll still get a warning when
this is not the case (and in fact I should strengthen the patch to
implement that).

Also, you don't want to suppress any other valid warning at that
instruction.

Furthermore, I really don't think we ought to worry about ease-of-use
here, there's really not that many people writing x86 assembly.

> But also... the RET_OFFSET usage for sync_core() *really* bugs me.

Fair enough.

> I know you said it's like an indirect tail call with a bigger frame, but
> that's kind of stretching it because the function frame is still there.
> 
> And objtool doesn't treat it like a tail call at all.  In fact, it
> handles it *completely* differently from the normal ret-tail-call case.
> Instead of silencing a tail call warning, it adjusts the stack offset
> and continues the code path.
> 
> This basically adds *two* new hint types, while trying to call them the
> same thing.  There's no overlapping functionality between them in
> objtool, other than the use of the same insn->ret_offset variable.  But
> it's two distinct functionalities, depending on the context (return/tail
> vs IRETQ).

I'm not against adding a second/separate hint for this. In fact, I
almost considered teaching objtool how to interpret the whole IRET frame
so that we can do it without hints. It's just that that's too much code
for this one case.

HINT_IRET_SELF ?

Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Tue, Mar 31, 2020 at 10:40:47PM +0200, Peter Zijlstra wrote:
> On Tue, Mar 31, 2020 at 03:23:15PM -0500, Josh Poimboeuf wrote:

> > I now understand what you're trying to do with the RET_TAIL thing, and I
> > guess it's ok for the ftrace case.  But I'd rather an UNWIND_HINT_IGNORE
> > before the tail cail, which would tell objtool to just silence the tail
> > call warning.  It's simpler for the user to understand, it's simpler
> > logic in objtool, and I think an "ignore warnings for the next insn"
> > hint would be more generally applicable anyway.
> 
> I like how this is specific on how far the stack can be off, as opposed
> so say 'ignore any warning on this instruction'.
> 
> Because by saying this RET should be +8, we'll still get a warning when
> this is not the case (and in fact I should strengthen the patch to
> implement that).

Like this; I'm confused on what cfa.offset is vs stack_size though.

But this way we're strict and always warn when the unexpected happens.

--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1423,8 +1423,7 @@ static bool has_modified_stack_frame(str
 	    !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
 		return true;
 
-	if (state->stack_size != initial_func_cfi.cfa.offset &&
-	    !(ret_offset && state->stack_size == initial_func_cfi.cfa.offset + ret_offset))
+	if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
 		return true;
 
 	for (i = 0; i < CFI_NUM_REGS; i++) {

Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Josh Poimboeuf]

On Tue, Mar 31, 2020 at 10:40:47PM +0200, Peter Zijlstra wrote:
> > I now understand what you're trying to do with the RET_TAIL thing, and I
> > guess it's ok for the ftrace case.  But I'd rather an UNWIND_HINT_IGNORE
> > before the tail cail, which would tell objtool to just silence the tail
> > call warning.  It's simpler for the user to understand, it's simpler
> > logic in objtool, and I think an "ignore warnings for the next insn"
> > hint would be more generally applicable anyway.
> 
> I like how this is specific on how far the stack can be off, as opposed
> so say 'ignore any warning on this instruction'.
> 
> Because by saying this RET should be +8, we'll still get a warning when
> this is not the case (and in fact I should strengthen the patch to
> implement that).
> 
> Also, you don't want to suppress any other valid warning at that
> instruction.

Ok, I guess I'm convinced :-)  As we continue to add new warnings to
objtool, it is true that "ignore all warnings at this insn" is probably
too broad.

/me stops writing patch

BTW, if we're in agreement that this hint doesn't belong for
sync_core(), will sp_offset always be +8?  Just wondering if we can
hard-code that assumption.

> > I know you said it's like an indirect tail call with a bigger frame, but
> > that's kind of stretching it because the function frame is still there.
> > 
> > And objtool doesn't treat it like a tail call at all.  In fact, it
> > handles it *completely* differently from the normal ret-tail-call case.
> > Instead of silencing a tail call warning, it adjusts the stack offset
> > and continues the code path.
> > 
> > This basically adds *two* new hint types, while trying to call them the
> > same thing.  There's no overlapping functionality between them in
> > objtool, other than the use of the same insn->ret_offset variable.  But
> > it's two distinct functionalities, depending on the context (return/tail
> > vs IRETQ).
> 
> I'm not against adding a second/separate hint for this. In fact, I
> almost considered teaching objtool how to interpret the whole IRET frame
> so that we can do it without hints. It's just that that's too much code
> for this one case.
> 
> HINT_IRET_SELF ?

Despite my earlier complaint about stack size knowledge, we could just
forget the hint and make "iretq in C code" equivalent to "reduce stack
size by arch_exception_stack_size()" and keep going.  There's
file->c_file which tells you it's a C file.

-- 
Josh

Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Josh Poimboeuf]

On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
> > I'm not against adding a second/separate hint for this. In fact, I
> > almost considered teaching objtool how to interpret the whole IRET frame
> > so that we can do it without hints. It's just that that's too much code
> > for this one case.
> > 
> > HINT_IRET_SELF ?
> 
> Despite my earlier complaint about stack size knowledge, we could just
> forget the hint and make "iretq in C code" equivalent to "reduce stack
> size by arch_exception_stack_size()" and keep going.  There's
> file->c_file which tells you it's a C file.

Or maybe "iretq in an STT_FUNC" is better since this pattern could
presumably happen in a callable asm function.

-- 
Josh

[PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote:
> On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
> > > I'm not against adding a second/separate hint for this. In fact, I
> > > almost considered teaching objtool how to interpret the whole IRET frame
> > > so that we can do it without hints. It's just that that's too much code
> > > for this one case.
> > > 
> > > HINT_IRET_SELF ?
> > 
> > Despite my earlier complaint about stack size knowledge, we could just
> > forget the hint and make "iretq in C code" equivalent to "reduce stack
> > size by arch_exception_stack_size()" and keep going.  There's
> > file->c_file which tells you it's a C file.
> 
> Or maybe "iretq in an STT_FUNC" is better since this pattern could
> presumably happen in a callable asm function.

Like so then?

---
Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 31 Mar 2020 13:16:52 +0200

This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that
applies to any instruction that terminates a function, like: RETURN
and sibling calls. It allows the stack-frame to be off by @sp_offset,
ie. it allows stuffing the return stack.

For ftrace_64.S we split the return path and make sure the
ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
own function.

By splitting the return path every instruction has a unique stack setup
and ORC can generate correct unwinds. Then employ the RET_OFFSET hint to
the tail-call exit that has the direct-call (orig_eax) stuffed on the
return stack.

For sync_core() we teach objtool that an IRET inside an STT_FUNC
simply consumes the exception stack and continues.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/orc_types.h       |    9 ++-
 arch/x86/include/asm/processor.h       |    2
 arch/x86/include/asm/unwind_hints.h    |   12 +---
 arch/x86/kernel/ftrace.c               |   12 ++++
 arch/x86/kernel/ftrace_64.S            |   27 ++++-------
 tools/arch/x86/include/asm/orc_types.h |    9 ++-
 tools/objtool/Makefile                 |    2
 tools/objtool/arch.h                   |    3 +
 tools/objtool/arch/x86/decode.c        |    5 +-
 tools/objtool/check.c                  |   80 ++++++++++-----------------------
 tools/objtool/check.h                  |    4 +
 11 files changed, 74 insertions(+), 91 deletions(-)

--- a/arch/x86/include/asm/orc_types.h
+++ b/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,13 @@
 #define ORC_TYPE_CALL			0
 #define ORC_TYPE_REGS			1
 #define ORC_TYPE_REGS_IRET		2
-#define UNWIND_HINT_TYPE_SAVE		3
-#define UNWIND_HINT_TYPE_RESTORE	4
+
+/*
+ * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
+ * and sibling calls. On these, sp_offset denotes the expected offset from
+ * initial_func_cfi.
+ */
+#define UNWIND_HINT_TYPE_RET_OFFSET	3

 #ifndef __ASSEMBLY__
 /*
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -738,7 +738,6 @@ static inline void sync_core(void)
 	unsigned int tmp;

 	asm volatile (
-		UNWIND_HINT_SAVE
 		"mov %%ss, %0\n\t"
 		"pushq %q0\n\t"
 		"pushq %%rsp\n\t"
@@ -748,7 +747,6 @@ static inline void sync_core(void)
 		"pushq %q0\n\t"
 		"pushq $1f\n\t"
 		"iretq\n\t"
-		UNWIND_HINT_RESTORE
 		"1:"
 		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
 #endif
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -86,12 +86,8 @@
 	UNWIND_HINT sp_offset=\sp_offset
 .endm

-.macro UNWIND_HINT_SAVE
-	UNWIND_HINT type=UNWIND_HINT_TYPE_SAVE
-.endm
-
-.macro UNWIND_HINT_RESTORE
-	UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE
+.macro UNWIND_HINT_RET_OFFSET sp_offset=8
+	UNWIND_HINT type=UNWIND_HINT_TYPE_RET_OFFSET sp_offset=\sp_offset
 .endm

 #else /* !__ASSEMBLY__ */
@@ -108,9 +104,7 @@
 	".balign 4 \n\t"					\
 	".popsection\n\t"

-#define UNWIND_HINT_SAVE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_SAVE, 0)
-
-#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
+#define UNWIND_HINT_RET_OFFSET(offset) UNWIND_HINT(0, (offset), UNWIND_HINT_TYPE_RET_OFFSET, 0)

 #endif /* __ASSEMBLY__ */

--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -282,7 +282,8 @@ static inline void tramp_free(void *tram

 /* Defined as markers to the end of the ftrace default trampolines */
 extern void ftrace_regs_caller_end(void);
-extern void ftrace_epilogue(void);
+extern void ftrace_regs_caller_ret(void);
+extern void ftrace_caller_end(void);
 extern void ftrace_caller_op_ptr(void);
 extern void ftrace_regs_caller_op_ptr(void);

@@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops
 		call_offset = (unsigned long)ftrace_regs_call;
 	} else {
 		start_offset = (unsigned long)ftrace_caller;
-		end_offset = (unsigned long)ftrace_epilogue;
+		end_offset = (unsigned long)ftrace_caller_end;
 		op_offset = (unsigned long)ftrace_caller_op_ptr;
 		call_offset = (unsigned long)ftrace_call;
 	}
@@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
 	if (WARN_ON(ret < 0))
 		goto fail;

+	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
+		ip = ftrace_regs_caller_ret;
+		ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
+		if (WARN_ON(ret < 0))
+			goto fail;
+	}
+
 	/*
 	 * The address of the ftrace_ops that is used for this trampoline
 	 * is stored at the end of the trampoline. This will be used to
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -157,8 +157,12 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBA
 	 * think twice before adding any new code or changing the
 	 * layout here.
 	 */
-SYM_INNER_LABEL(ftrace_epilogue, SYM_L_GLOBAL)
+SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)

+	jmp ftrace_epilogue
+SYM_FUNC_END(ftrace_caller);
+
+SYM_FUNC_START(ftrace_epilogue)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
 	jmp ftrace_stub
@@ -170,14 +174,12 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L
  */
 SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK)
 	retq
-SYM_FUNC_END(ftrace_caller)
+SYM_FUNC_END(ftrace_epilogue)

 SYM_FUNC_START(ftrace_regs_caller)
 	/* Save the current flags before any operations that can change them */
 	pushfq

-	UNWIND_HINT_SAVE
-
 	/* added 8 bytes to save flags */
 	save_mcount_regs 8
 	/* save_mcount_regs fills in first two parameters */
@@ -244,20 +246,14 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_
 	movq %rax, MCOUNT_REG_SIZE(%rsp)

 	restore_mcount_regs 8
+	/* Restore flags */
+	popfq

-	jmp	2f
+SYM_INNER_LABEL(ftrace_regs_caller_ret, SYM_L_GLOBAL);
+	UNWIND_HINT_RET_OFFSET
+	jmp	ftrace_epilogue

 1:	restore_mcount_regs
-
-
-2:
-	/*
-	 * The stack layout is nondetermistic here, depending on which path was
-	 * taken.  This confuses objtool and ORC, rightfully so.  For now,
-	 * pretend the stack always looks like the non-direct case.
-	 */
-	UNWIND_HINT_RESTORE
-
 	/* Restore flags */
 	popfq

@@ -268,7 +264,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_
 	 * to the return.
 	 */
 SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL)
-
 	jmp ftrace_epilogue

 SYM_FUNC_END(ftrace_regs_caller)
--- a/tools/arch/x86/include/asm/orc_types.h
+++ b/tools/arch/x86/include/asm/orc_types.h
@@ -58,8 +58,13 @@
 #define ORC_TYPE_CALL			0
 #define ORC_TYPE_REGS			1
 #define ORC_TYPE_REGS_IRET		2
-#define UNWIND_HINT_TYPE_SAVE		3
-#define UNWIND_HINT_TYPE_RESTORE	4
+
+/*
+ * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
+ * and sibling calls. On these, sp_offset denotes the expected offset from
+ * initial_func_cfi.
+ */
+#define UNWIND_HINT_TYPE_RET_OFFSET	3

 #ifndef __ASSEMBLY__
 /*
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
 	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
 	    -I$(srctree)/tools/arch/$(SRCARCH)/include
 WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
-CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
+CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
 LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)

 # Allow old libelf to be used:
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -19,6 +19,7 @@ enum insn_type {
 	INSN_CALL,
 	INSN_CALL_DYNAMIC,
 	INSN_RETURN,
+	INSN_EXCEPTION_RETURN,
 	INSN_CONTEXT_SWITCH,
 	INSN_STACK,
 	INSN_BUG,
@@ -75,4 +76,6 @@ int arch_decode_instruction(struct elf *

 bool arch_callee_saved_reg(unsigned char reg);

+static const int arch_exception_frame_size = 5*8;
+
 #endif /* _ARCH_H */
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -431,10 +431,13 @@ int arch_decode_instruction(struct elf *

 	case 0xca: /* retf */
 	case 0xcb: /* retf */
-	case 0xcf: /* iret */
 		*type = INSN_CONTEXT_SWITCH;
 		break;

+	case 0xcf: /* iret */
+		*type = INSN_EXCEPTION_RETURN;
+		break;
+
 	case 0xe8:
 		*type = INSN_CALL;
 		break;
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1246,13 +1246,8 @@ static int read_unwind_hints(struct objt

 		cfa = &insn->state.cfa;

-		if (hint->type == UNWIND_HINT_TYPE_SAVE) {
-			insn->save = true;
-			continue;
-
-		} else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
-			insn->restore = true;
-			insn->hint = true;
+		if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
+			insn->ret_offset = hint->sp_offset;
 			continue;
 		}

@@ -1416,20 +1411,26 @@ static bool is_fentry_call(struct instru
 	return false;
 }

-static bool has_modified_stack_frame(struct insn_state *state)
+static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
 {
+	u8 ret_offset = insn->ret_offset;
 	int i;

-	if (state->cfa.base != initial_func_cfi.cfa.base ||
-	    state->cfa.offset != initial_func_cfi.cfa.offset ||
-	    state->stack_size != initial_func_cfi.cfa.offset ||
-	    state->drap)
+	if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
+		return true;
+
+	if (state->cfa.offset != initial_func_cfi.cfa.offset &&
+	    !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
+		return true;
+
+	if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
 		return true;

-	for (i = 0; i < CFI_NUM_REGS; i++)
+	for (i = 0; i < CFI_NUM_REGS; i++) {
 		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
 		    state->regs[i].offset != initial_func_cfi.regs[i].offset)
 			return true;
+	}

 	return false;
 }
@@ -1971,7 +1972,7 @@ static int validate_call(struct instruct

 static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
 {
-	if (has_modified_stack_frame(state)) {
+	if (has_modified_stack_frame(insn, state)) {
 		WARN_FUNC("sibling call from callable instruction with modified stack frame",
 				insn->sec, insn->offset);
 		return 1;
@@ -2000,7 +2001,7 @@ static int validate_return(struct symbol
 		return 1;
 	}

-	if (func && has_modified_stack_frame(state)) {
+	if (func && has_modified_stack_frame(insn, state)) {
 		WARN_FUNC("return with modified stack frame",
 			  insn->sec, insn->offset);
 		return 1;
@@ -2063,47 +2064,9 @@ static int validate_branch(struct objtoo
 				return 0;
 		}

-		if (insn->hint) {
-			if (insn->restore) {
-				struct instruction *save_insn, *i;
-
-				i = insn;
-				save_insn = NULL;
-				sym_for_each_insn_continue_reverse(file, func, i) {
-					if (i->save) {
-						save_insn = i;
-						break;
-					}
-				}
-
-				if (!save_insn) {
-					WARN_FUNC("no corresponding CFI save for CFI restore",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				if (!save_insn->visited) {
-					/*
-					 * Oops, no state to copy yet.
-					 * Hopefully we can reach this
-					 * instruction from another branch
-					 * after the save insn has been
-					 * visited.
-					 */
-					if (insn == first)
-						return 0;
-
-					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
-						  sec, insn->offset);
-					return 1;
-				}
-
-				insn->state = save_insn->state;
-			}
-
+		if (insn->hint)
 			state = insn->state;
-
-		} else
+		else
 			insn->state = state;

 		insn->visited |= visited;
@@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo

 			break;

+		case INSN_EXCEPTION_RETURN:
+			if (func) {
+				state.stack_size -= arch_exception_frame_size;
+				break;
+			}
+
+			/* fallthrough */
 		case INSN_CONTEXT_SWITCH:
 			if (func && (!next_insn || !next_insn->hint)) {
 				WARN_FUNC("unsupported instruction in callable function",
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -33,9 +33,11 @@ struct instruction {
 	unsigned int len;
 	enum insn_type type;
 	unsigned long immediate;
-	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
+	bool alt_group, dead_end, ignore, ignore_alts;
+	bool hint;
 	bool retpoline_safe;
 	u8 visited;
+	u8 ret_offset;
 	struct symbol *call_dest;
 	struct instruction *jump_dest;
 	struct instruction *first_jump_src;

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Josh Poimboeuf]

On Wed, Apr 01, 2020 at 12:27:03AM +0200, Peter Zijlstra wrote:
> On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote:
> > On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
> > > > I'm not against adding a second/separate hint for this. In fact, I
> > > > almost considered teaching objtool how to interpret the whole IRET frame
> > > > so that we can do it without hints. It's just that that's too much code
> > > > for this one case.
> > > > 
> > > > HINT_IRET_SELF ?
> > > 
> > > Despite my earlier complaint about stack size knowledge, we could just
> > > forget the hint and make "iretq in C code" equivalent to "reduce stack
> > > size by arch_exception_stack_size()" and keep going.  There's
> > > file->c_file which tells you it's a C file.
> > 
> > Or maybe "iretq in an STT_FUNC" is better since this pattern could
> > presumably happen in a callable asm function.
> 
> Like so then?

I'd suggest a patch split like:

1) objtool: automagic IRET-in-func
2) objtool: add RET_OFFSET
3) ftrace:  re-organize asm (and use RET_OFFSET hint)
4) objtool: remove now-unused SAVE/RESTORE

> --- a/arch/x86/include/asm/orc_types.h
> +++ b/arch/x86/include/asm/orc_types.h
> @@ -58,8 +58,13 @@
>  #define ORC_TYPE_CALL			0
>  #define ORC_TYPE_REGS			1
>  #define ORC_TYPE_REGS_IRET		2
> -#define UNWIND_HINT_TYPE_SAVE		3
> -#define UNWIND_HINT_TYPE_RESTORE	4
> +
> +/*
> + * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
> + * and sibling calls. On these, sp_offset denotes the expected offset from
> + * initial_func_cfi.
> + */
> +#define UNWIND_HINT_TYPE_RET_OFFSET	3

I think this comment belongs at the UNWIND_HINT_RET_OFFSET macro
definition.

> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -282,7 +282,8 @@ static inline void tramp_free(void *tram
> 
>  /* Defined as markers to the end of the ftrace default trampolines */
>  extern void ftrace_regs_caller_end(void);
> -extern void ftrace_epilogue(void);
> +extern void ftrace_regs_caller_ret(void);
> +extern void ftrace_caller_end(void);
>  extern void ftrace_caller_op_ptr(void);
>  extern void ftrace_regs_caller_op_ptr(void);
> 
> @@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops
>  		call_offset = (unsigned long)ftrace_regs_call;
>  	} else {
>  		start_offset = (unsigned long)ftrace_caller;
> -		end_offset = (unsigned long)ftrace_epilogue;
> +		end_offset = (unsigned long)ftrace_caller_end;
>  		op_offset = (unsigned long)ftrace_caller_op_ptr;
>  		call_offset = (unsigned long)ftrace_call;
>  	}
> @@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
>  	if (WARN_ON(ret < 0))
>  		goto fail;
> 
> +	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> +		ip = ftrace_regs_caller_ret;
> +		ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> +		if (WARN_ON(ret < 0))
> +			goto fail;
> +	}
> +

Hm?  This function creates a trampoline but it looks like this change is
overwriting the original ftrace_64 code itself?

> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
>  	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
>  	    -I$(srctree)/tools/arch/$(SRCARCH)/include
>  WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
> -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> +CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
>  LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)

Why?  Smells like a separate patch at least.

-- 
Josh

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Wed, Apr 01, 2020 at 09:14:02AM -0500, Josh Poimboeuf wrote:
> On Wed, Apr 01, 2020 at 12:27:03AM +0200, Peter Zijlstra wrote:
> > On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote:
> > > On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
> > > > > I'm not against adding a second/separate hint for this. In fact, I
> > > > > almost considered teaching objtool how to interpret the whole IRET frame
> > > > > so that we can do it without hints. It's just that that's too much code
> > > > > for this one case.
> > > > > 
> > > > > HINT_IRET_SELF ?
> > > > 
> > > > Despite my earlier complaint about stack size knowledge, we could just
> > > > forget the hint and make "iretq in C code" equivalent to "reduce stack
> > > > size by arch_exception_stack_size()" and keep going.  There's
> > > > file->c_file which tells you it's a C file.
> > > 
> > > Or maybe "iretq in an STT_FUNC" is better since this pattern could
> > > presumably happen in a callable asm function.
> > 
> > Like so then?
> 
> I'd suggest a patch split like:
> 
> 1) objtool: automagic IRET-in-func
> 2) objtool: add RET_OFFSET
> 3) ftrace:  re-organize asm (and use RET_OFFSET hint)
> 4) objtool: remove now-unused SAVE/RESTORE

Sure.

> > --- a/arch/x86/include/asm/orc_types.h
> > +++ b/arch/x86/include/asm/orc_types.h
> > @@ -58,8 +58,13 @@
> >  #define ORC_TYPE_CALL			0
> >  #define ORC_TYPE_REGS			1
> >  #define ORC_TYPE_REGS_IRET		2
> > -#define UNWIND_HINT_TYPE_SAVE		3
> > -#define UNWIND_HINT_TYPE_RESTORE	4
> > +
> > +/*
> > + * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
> > + * and sibling calls. On these, sp_offset denotes the expected offset from
> > + * initial_func_cfi.
> > + */
> > +#define UNWIND_HINT_TYPE_RET_OFFSET	3
> 
> I think this comment belongs at the UNWIND_HINT_RET_OFFSET macro
> definition.

Humph, ok, but there's two of those :/

> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > @@ -282,7 +282,8 @@ static inline void tramp_free(void *tram
> > 
> >  /* Defined as markers to the end of the ftrace default trampolines */
> >  extern void ftrace_regs_caller_end(void);
> > -extern void ftrace_epilogue(void);
> > +extern void ftrace_regs_caller_ret(void);
> > +extern void ftrace_caller_end(void);
> >  extern void ftrace_caller_op_ptr(void);
> >  extern void ftrace_regs_caller_op_ptr(void);
> > 
> > @@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops
> >  		call_offset = (unsigned long)ftrace_regs_call;
> >  	} else {
> >  		start_offset = (unsigned long)ftrace_caller;
> > -		end_offset = (unsigned long)ftrace_epilogue;
> > +		end_offset = (unsigned long)ftrace_caller_end;
> >  		op_offset = (unsigned long)ftrace_caller_op_ptr;
> >  		call_offset = (unsigned long)ftrace_call;
> >  	}
> > @@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
> >  	if (WARN_ON(ret < 0))
> >  		goto fail;
> > 
> > +	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > +		ip = ftrace_regs_caller_ret;
> > +		ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> > +		if (WARN_ON(ret < 0))
> > +			goto fail;
> > +	}
> > +
> 
> Hm?  This function creates a trampoline but it looks like this change is
> overwriting the original ftrace_64 code itself?

Ahh. So if you look at what the trampoline copies, you'll note we'll
copy until -- but *NOT* including -- the jmp ftrace_epilogue. Instead
we'll write a RET at the end.

However, due to splitting the return path, such that each instruction
has a unique stack offset, we now have a second jmp ftrace_epilogue in
the middle of the function. That too needs to be overwritten by a RET.

> > --- a/tools/objtool/Makefile
> > +++ b/tools/objtool/Makefile
> > @@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
> >  	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
> >  	    -I$(srctree)/tools/arch/$(SRCARCH)/include
> >  WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
> > -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> > +CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
> >  LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> 
> Why?  Smells like a separate patch at least.

Oh, whoops :-) I keep doing this every time I need to run gdb on it.
I'll make it go away.

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Josh Poimboeuf]

On Wed, Apr 01, 2020 at 04:22:26PM +0200, Peter Zijlstra wrote:
> > > --- a/arch/x86/include/asm/orc_types.h
> > > +++ b/arch/x86/include/asm/orc_types.h
> > > @@ -58,8 +58,13 @@
> > >  #define ORC_TYPE_CALL			0
> > >  #define ORC_TYPE_REGS			1
> > >  #define ORC_TYPE_REGS_IRET		2
> > > -#define UNWIND_HINT_TYPE_SAVE		3
> > > -#define UNWIND_HINT_TYPE_RESTORE	4
> > > +
> > > +/*
> > > + * RET_OFFSET: Used on instructions that terminate a function; mostly RETURN
> > > + * and sibling calls. On these, sp_offset denotes the expected offset from
> > > + * initial_func_cfi.
> > > + */
> > > +#define UNWIND_HINT_TYPE_RET_OFFSET	3
> > 
> > I think this comment belongs at the UNWIND_HINT_RET_OFFSET macro
> > definition.
> 
> Humph, ok, but there's two of those :/

Just commenting the asm one should be fine I think.

That way a reader of code using the macro is more likely to understand
its purpose.

> > > --- a/arch/x86/kernel/ftrace.c
> > > +++ b/arch/x86/kernel/ftrace.c
> > > @@ -282,7 +282,8 @@ static inline void tramp_free(void *tram
> > > 
> > >  /* Defined as markers to the end of the ftrace default trampolines */
> > >  extern void ftrace_regs_caller_end(void);
> > > -extern void ftrace_epilogue(void);
> > > +extern void ftrace_regs_caller_ret(void);
> > > +extern void ftrace_caller_end(void);
> > >  extern void ftrace_caller_op_ptr(void);
> > >  extern void ftrace_regs_caller_op_ptr(void);
> > > 
> > > @@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops
> > >  		call_offset = (unsigned long)ftrace_regs_call;
> > >  	} else {
> > >  		start_offset = (unsigned long)ftrace_caller;
> > > -		end_offset = (unsigned long)ftrace_epilogue;
> > > +		end_offset = (unsigned long)ftrace_caller_end;
> > >  		op_offset = (unsigned long)ftrace_caller_op_ptr;
> > >  		call_offset = (unsigned long)ftrace_call;
> > >  	}
> > > @@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
> > >  	if (WARN_ON(ret < 0))
> > >  		goto fail;
> > > 
> > > +	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > > +		ip = ftrace_regs_caller_ret;
> > > +		ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> > > +		if (WARN_ON(ret < 0))
> > > +			goto fail;
> > > +	}
> > > +
> > 
> > Hm?  This function creates a trampoline but it looks like this change is
> > overwriting the original ftrace_64 code itself?
> 
> Ahh. So if you look at what the trampoline copies, you'll note we'll
> copy until -- but *NOT* including -- the jmp ftrace_epilogue. Instead
> we'll write a RET at the end.
> 
> However, due to splitting the return path, such that each instruction
> has a unique stack offset, we now have a second jmp ftrace_epilogue in
> the middle of the function. That too needs to be overwritten by a RET.

Right, but 'ip' needs to point to the trampoline's version of
'ftrace_regs_caller_ret', not the original ftrace_64 version.

> > > --- a/tools/objtool/Makefile
> > > +++ b/tools/objtool/Makefile
> > > @@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
> > >  	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
> > >  	    -I$(srctree)/tools/arch/$(SRCARCH)/include
> > >  WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
> > > -CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> > > +CFLAGS   := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS)
> > >  LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
> > 
> > Why?  Smells like a separate patch at least.
> 
> Oh, whoops :-) I keep doing this every time I need to run gdb on it.
> I'll make it go away.

Ha, I do something similar, I just add -O0 to CFLAGS.

-- 
Josh

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Wed, Apr 01, 2020 at 09:39:16AM -0500, Josh Poimboeuf wrote:
> On Wed, Apr 01, 2020 at 04:22:26PM +0200, Peter Zijlstra wrote:

> > > > @@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops
> > > >  	if (WARN_ON(ret < 0))
> > > >  		goto fail;
> > > > 
> > > > +	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > > > +		ip = ftrace_regs_caller_ret;
> > > > +		ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> > > > +		if (WARN_ON(ret < 0))
> > > > +			goto fail;
> > > > +	}
> > > > +

> Right, but 'ip' needs to point to the trampoline's version of
> 'ftrace_regs_caller_ret', not the original ftrace_64 version.

Duh,

		ip = trampline + (ftrace_regs_caller_ret - ftrace_regs_caller);

it is.

Luckily this would've triggerd a splat the first time I'd have actually
booted this, because the regular text is RO, while the trampoline is
still RW here. Still, good spotting.

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Steven Rostedt]

On Wed, 1 Apr 2020 09:39:16 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> > > > +	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > > > +		ip = ftrace_regs_caller_ret;
> > > > +		ret = probe_kernel_read(ip, (void *)retq, RET_SIZE);
> > > > +		if (WARN_ON(ret < 0))
> > > > +			goto fail;
> > > > +	}
> > > > +  
> > > 
> > > Hm?  This function creates a trampoline but it looks like this change is
> > > overwriting the original ftrace_64 code itself?  
> > 
> > Ahh. So if you look at what the trampoline copies, you'll note we'll
> > copy until -- but *NOT* including -- the jmp ftrace_epilogue. Instead
> > we'll write a RET at the end.
> > 
> > However, due to splitting the return path, such that each instruction
> > has a unique stack offset, we now have a second jmp ftrace_epilogue in
> > the middle of the function. That too needs to be overwritten by a RET.  
> 
> Right, but 'ip' needs to point to the trampoline's version of
> 'ftrace_regs_caller_ret', not the original ftrace_64 version.

I noticed the same thing. And after applying just this patch (with tweaking
the obtool mark ups to make it compile) it crashed.

-- Steve

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Julien Thierry]

Hi Peter,

On 3/31/20 11:27 PM, Peter Zijlstra wrote:
> On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote:
>> On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
>>>> I'm not against adding a second/separate hint for this. In fact, I
>>>> almost considered teaching objtool how to interpret the whole IRET frame
>>>> so that we can do it without hints. It's just that that's too much code
>>>> for this one case.
>>>>
>>>> HINT_IRET_SELF ?
>>>
>>> Despite my earlier complaint about stack size knowledge, we could just
>>> forget the hint and make "iretq in C code" equivalent to "reduce stack
>>> size by arch_exception_stack_size()" and keep going.  There's
>>> file->c_file which tells you it's a C file.
>>
>> Or maybe "iretq in an STT_FUNC" is better since this pattern could
>> presumably happen in a callable asm function.
> 
> Like so then?
> 
> ---
> Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue, 31 Mar 2020 13:16:52 +0200
> 
> This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that
> applies to any instruction that terminates a function, like: RETURN
> and sibling calls. It allows the stack-frame to be off by @sp_offset,
> ie. it allows stuffing the return stack.
> 
> For ftrace_64.S we split the return path and make sure the
> ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
> own function.
> 
> By splitting the return path every instruction has a unique stack setup
> and ORC can generate correct unwinds. Then employ the RET_OFFSET hint to
> the tail-call exit that has the direct-call (orig_eax) stuffed on the
> return stack.
> 
> For sync_core() we teach objtool that an IRET inside an STT_FUNC
> simply consumes the exception stack and continues.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>   arch/x86/include/asm/orc_types.h       |    9 ++-
>   arch/x86/include/asm/processor.h       |    2
>   arch/x86/include/asm/unwind_hints.h    |   12 +---
>   arch/x86/kernel/ftrace.c               |   12 ++++
>   arch/x86/kernel/ftrace_64.S            |   27 ++++-------
>   tools/arch/x86/include/asm/orc_types.h |    9 ++-
>   tools/objtool/Makefile                 |    2
>   tools/objtool/arch.h                   |    3 +
>   tools/objtool/arch/x86/decode.c        |    5 +-
>   tools/objtool/check.c                  |   80 ++++++++++-----------------------
>   tools/objtool/check.h                  |    4 +
>   11 files changed, 74 insertions(+), 91 deletions(-)
> 

[snip]

> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1246,13 +1246,8 @@ static int read_unwind_hints(struct objt
> 
>   		cfa = &insn->state.cfa;
> 
> -		if (hint->type == UNWIND_HINT_TYPE_SAVE) {
> -			insn->save = true;
> -			continue;
> -
> -		} else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
> -			insn->restore = true;
> -			insn->hint = true;
> +		if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
> +			insn->ret_offset = hint->sp_offset;
>   			continue;
>   		}
> 
> @@ -1416,20 +1411,26 @@ static bool is_fentry_call(struct instru
>   	return false;
>   }
> 
> -static bool has_modified_stack_frame(struct insn_state *state)
> +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
>   {
> +	u8 ret_offset = insn->ret_offset;
>   	int i;
> 
> -	if (state->cfa.base != initial_func_cfi.cfa.base ||
> -	    state->cfa.offset != initial_func_cfi.cfa.offset ||
> -	    state->stack_size != initial_func_cfi.cfa.offset ||
> -	    state->drap)
> +	if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
> +		return true;
> +
> +	if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> +	    !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))

Isn't that the same thing as "state->cfa.offset != 
initial_func_cfi.cfa.offset + ret_offset" ?

> +		return true;
> +
> +	if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
>   		return true;
> 
> -	for (i = 0; i < CFI_NUM_REGS; i++)
> +	for (i = 0; i < CFI_NUM_REGS; i++) {
>   		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>   		    state->regs[i].offset != initial_func_cfi.regs[i].offset)
>   			return true;
> +	}
> 
>   	return false;
>   }
> @@ -1971,7 +1972,7 @@ static int validate_call(struct instruct
> 
>   static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
>   {
> -	if (has_modified_stack_frame(state)) {
> +	if (has_modified_stack_frame(insn, state)) {
>   		WARN_FUNC("sibling call from callable instruction with modified stack frame",
>   				insn->sec, insn->offset);
>   		return 1;
> @@ -2000,7 +2001,7 @@ static int validate_return(struct symbol
>   		return 1;
>   	}
> 
> -	if (func && has_modified_stack_frame(state)) {
> +	if (func && has_modified_stack_frame(insn, state)) {
>   		WARN_FUNC("return with modified stack frame",
>   			  insn->sec, insn->offset);
>   		return 1;
> @@ -2063,47 +2064,9 @@ static int validate_branch(struct objtoo
>   				return 0;
>   		}
> 
> -		if (insn->hint) {
> -			if (insn->restore) {
> -				struct instruction *save_insn, *i;
> -
> -				i = insn;
> -				save_insn = NULL;
> -				sym_for_each_insn_continue_reverse(file, func, i) {
> -					if (i->save) {
> -						save_insn = i;
> -						break;
> -					}
> -				}
> -
> -				if (!save_insn) {
> -					WARN_FUNC("no corresponding CFI save for CFI restore",
> -						  sec, insn->offset);
> -					return 1;
> -				}
> -
> -				if (!save_insn->visited) {
> -					/*
> -					 * Oops, no state to copy yet.
> -					 * Hopefully we can reach this
> -					 * instruction from another branch
> -					 * after the save insn has been
> -					 * visited.
> -					 */
> -					if (insn == first)
> -						return 0;
> -
> -					WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> -						  sec, insn->offset);
> -					return 1;
> -				}
> -
> -				insn->state = save_insn->state;
> -			}
> -
> +		if (insn->hint)
>   			state = insn->state;
> -
> -		} else
> +		else
>   			insn->state = state;
> 
>   		insn->visited |= visited;
> @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
> 
>   			break;
> 
> +		case INSN_EXCEPTION_RETURN:
> +			if (func) {
> +				state.stack_size -= arch_exception_frame_size;
> +				break;

Why break instead of returning? Shouldn't an exception return mark the 
end of a branch (whether inside or outside a function) ?

Here it seems it will continue to the next instruction which might have 
been unreachable.

> +			}
> +
> +			/* fallthrough */

What is the purpose of the fallthrough here? If the exception return was 
in a function, it carried on to the next instruction, so it won't use 
the WARN_FUNC(). So, if I'm looking at the right version of the code 
only the "return 0;" will be used. And, unless my previous comment is 
wrong, I'd argue that we should return both for func and !func.

>   		case INSN_CONTEXT_SWITCH:
>   			if (func && (!next_insn || !next_insn->hint)) {
>   				WARN_FUNC("unsupported instruction in callable function",
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -33,9 +33,11 @@ struct instruction {
>   	unsigned int len;
>   	enum insn_type type;
>   	unsigned long immediate;
> -	bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> +	bool alt_group, dead_end, ignore, ignore_alts;
> +	bool hint;
>   	bool retpoline_safe;
>   	u8 visited;
> +	u8 ret_offset;
>   	struct symbol *call_dest;
>   	struct instruction *jump_dest;
>   	struct instruction *first_jump_src;
> 
> 

Cheers,

-- 
Julien Thierry

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Wed, Apr 01, 2020 at 04:43:35PM +0100, Julien Thierry wrote:

> > +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
> >   {
> > +	u8 ret_offset = insn->ret_offset;
> >   	int i;
> > 
> > -	if (state->cfa.base != initial_func_cfi.cfa.base ||
> > -	    state->cfa.offset != initial_func_cfi.cfa.offset ||
> > -	    state->stack_size != initial_func_cfi.cfa.offset ||
> > -	    state->drap)
> > +	if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
> > +		return true;
> > +
> > +	if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> > +	    !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
> 
> Isn't that the same thing as "state->cfa.offset !=
> initial_func_cfi.cfa.offset + ret_offset" ?

I'm confused on what cfa.offset is, sometimes it increase with
stack_size, sometimes it doesn't.

ISTR that for the ftrace case it was indeed cfa.offset + 8, but for the
IRET case below (where it is now not used anymore) it was cfa.offset
(not cfa.offset + 40, which I was expecting).

> > +		return true;
> > +
> > +	if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
> >   		return true;
> > 
> > -	for (i = 0; i < CFI_NUM_REGS; i++)
> > +	for (i = 0; i < CFI_NUM_REGS; i++) {
> >   		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
> >   		    state->regs[i].offset != initial_func_cfi.regs[i].offset)
> >   			return true;
> > +	}
> > 
> >   	return false;
> >   }

> > @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
> > 
> >   			break;
> > 
> > +		case INSN_EXCEPTION_RETURN:
> > +			if (func) {
> > +				state.stack_size -= arch_exception_frame_size;
> > +				break;
> 
> Why break instead of returning? Shouldn't an exception return mark the end
> of a branch (whether inside or outside a function) ?
> 
> Here it seems it will continue to the next instruction which might have been
> unreachable.

The code in question (x86's sync_core()), is an exception return to
self. It pushes an exception frame that points to right after the
exception return instruction.

This is the only usage of IRET in STT_FUNC symbols.

So rather than teaching objtool how to interpret the whole
push;push;push;push;push;iret sequence, teach it how big the frame is
(arch_exception_frame_size) and let it continue.

All the other (real) IRETs are in STT_NOTYPE in the entry assembly.

> > +			}
> > +
> > +			/* fallthrough */
> 
> What is the purpose of the fallthrough here? If the exception return was in
> a function, it carried on to the next instruction, so it won't use the
> WARN_FUNC(). So, if I'm looking at the right version of the code only the
> "return 0;" will be used. And, unless my previous comment is wrong, I'd
> argue that we should return both for func and !func.

That came from the fact that we split it out of INSN_CONTEXT_SWITCH.
You're right that it has now reduced to just return 0.

> >   		case INSN_CONTEXT_SWITCH:
> >   			if (func && (!next_insn || !next_insn->hint)) {
> >   				WARN_FUNC("unsupported instruction in callable function",

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Steven Rostedt]

On Wed, 1 Apr 2020 19:09:10 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> > > +	if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> > > +	    !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))  
> > 
> > Isn't that the same thing as "state->cfa.offset !=
> > initial_func_cfi.cfa.offset + ret_offset" ?  
> 
> I'm confused on what cfa.offset is, sometimes it increase with
> stack_size, sometimes it doesn't.

I believe what Julien is saying is the above logic is equivalent:

	if (x != y &&
	    !(z && x == y + z))

is the same as:

	if (x != y + z)

-- Steve

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Josh Poimboeuf]

On Wed, Apr 01, 2020 at 07:09:10PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 01, 2020 at 04:43:35PM +0100, Julien Thierry wrote:
> 
> > > +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
> > >   {
> > > +	u8 ret_offset = insn->ret_offset;
> > >   	int i;
> > > 
> > > -	if (state->cfa.base != initial_func_cfi.cfa.base ||
> > > -	    state->cfa.offset != initial_func_cfi.cfa.offset ||
> > > -	    state->stack_size != initial_func_cfi.cfa.offset ||
> > > -	    state->drap)
> > > +	if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
> > > +		return true;
> > > +
> > > +	if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> > > +	    !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
> > 
> > Isn't that the same thing as "state->cfa.offset !=
> > initial_func_cfi.cfa.offset + ret_offset" ?
> 
> I'm confused on what cfa.offset is, sometimes it increase with
> stack_size, sometimes it doesn't.
>
> ISTR that for the ftrace case it was indeed cfa.offset + 8, but for the
> IRET case below (where it is now not used anymore) it was cfa.offset
> (not cfa.offset + 40, which I was expecting).

It depends on the value of cfa.base.  If cfa.base is CFI_SP, then
cfa.offset changes with stack_size.  If cfa.base is CFI_BP (i.e. if the
function uses a frame pointer), then cfa.offset is constant (the
distance between RBP on the stack and the previous frame).

-- 
Josh

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Wed, Apr 01, 2020 at 01:33:03PM -0400, Steven Rostedt wrote:
> On Wed, 1 Apr 2020 19:09:10 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > > +	if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> > > > +	    !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))  
> > > 
> > > Isn't that the same thing as "state->cfa.offset !=
> > > initial_func_cfi.cfa.offset + ret_offset" ?  
> > 
> > I'm confused on what cfa.offset is, sometimes it increase with
> > stack_size, sometimes it doesn't.
> 
> I believe what Julien is saying is the above logic is equivalent:
> 
> 	if (x != y &&
> 	    !(z && x == y + z))
> 
> is the same as:
> 
> 	if (x != y + z)

It is not, the former will accept either x==y || x==y+z, while the
latter will only accept x==y+z.

For stack_size, I'm confident in just x==y+z, for offset I saw
conflicting things.

Since the annotation is now used in only a single place, maybe x==y+z
will just work, I'll go try once I've managed to unconfuse myself on the
whole fp-unwind situation.

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Steven Rostedt]

On Wed, 1 Apr 2020 19:45:44 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> > I believe what Julien is saying is the above logic is equivalent:
> > 
> > 	if (x != y &&
> > 	    !(z && x == y + z))
> > 
> > is the same as:
> > 
> > 	if (x != y + z)  
> 
> It is not, the former will accept either x==y || x==y+z, while the
> latter will only accept x==y+z.

No, the former accepts:

	x==y || (z && x == y + z)

Which is the same as: x == y + z

As the second condition is only tested if z != 0, and x ==  y is the same
as x == y + 0

-- Steve

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Wed, Apr 01, 2020 at 02:20:15PM -0400, Steven Rostedt wrote:
> On Wed, 1 Apr 2020 19:45:44 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > I believe what Julien is saying is the above logic is equivalent:
> > > 
> > > 	if (x != y &&
> > > 	    !(z && x == y + z))
> > > 
> > > is the same as:
> > > 
> > > 	if (x != y + z)  
> > 
> > It is not, the former will accept either x==y || x==y+z, while the
> > latter will only accept x==y+z.
> 
> No, the former accepts:
> 
> 	x==y || (z && x == y + z)
> 
> Which is the same as: x == y + z
> 
> As the second condition is only tested if z != 0, and x ==  y is the same
> as x == y + 0

Right, so it accepts both +0 and +z, while the latter will only accept
+z.

( in the iret case I had offset at +0 and stack_size at +40, while with
  the ftrace case I had both at +8; which is why I wrote the form that
  accepts +0 and +z )

Anyway, I tested it, and for the ftrace case (the only current user of
the hint) +z is correct for both offset and stack_size. I build both FP
and ORC variants.

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Julien Thierry]

On 4/1/20 6:09 PM, Peter Zijlstra wrote:
> On Wed, Apr 01, 2020 at 04:43:35PM +0100, Julien Thierry wrote:
> 
>>> +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
>>>    {
>>> +	u8 ret_offset = insn->ret_offset;
>>>    	int i;
>>>
>>> -	if (state->cfa.base != initial_func_cfi.cfa.base ||
>>> -	    state->cfa.offset != initial_func_cfi.cfa.offset ||
>>> -	    state->stack_size != initial_func_cfi.cfa.offset ||
>>> -	    state->drap)
>>> +	if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
>>> +		return true;
>>> +
>>> +	if (state->cfa.offset != initial_func_cfi.cfa.offset &&
>>> +	    !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
>>
>> Isn't that the same thing as "state->cfa.offset !=
>> initial_func_cfi.cfa.offset + ret_offset" ?
> 
> I'm confused on what cfa.offset is, sometimes it increase with
> stack_size, sometimes it doesn't.
> 

Steven already replied for me about that :) .

> ISTR that for the ftrace case it was indeed cfa.offset + 8, but for the
> IRET case below (where it is now not used anymore) it was cfa.offset
> (not cfa.offset + 40, which I was expecting).
> 
>>> +		return true;
>>> +
>>> +	if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
>>>    		return true;
>>>
>>> -	for (i = 0; i < CFI_NUM_REGS; i++)
>>> +	for (i = 0; i < CFI_NUM_REGS; i++) {
>>>    		if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>>>    		    state->regs[i].offset != initial_func_cfi.regs[i].offset)
>>>    			return true;
>>> +	}
>>>
>>>    	return false;
>>>    }
> 
>>> @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
>>>
>>>    			break;
>>>
>>> +		case INSN_EXCEPTION_RETURN:
>>> +			if (func) {
>>> +				state.stack_size -= arch_exception_frame_size;
>>> +				break;
>>
>> Why break instead of returning? Shouldn't an exception return mark the end
>> of a branch (whether inside or outside a function) ?
>>
>> Here it seems it will continue to the next instruction which might have been
>> unreachable.
> 
> The code in question (x86's sync_core()), is an exception return to
> self. It pushes an exception frame that points to right after the
> exception return instruction.
> 
> This is the only usage of IRET in STT_FUNC symbols.
> 
> So rather than teaching objtool how to interpret the whole
> push;push;push;push;push;iret sequence, teach it how big the frame is
> (arch_exception_frame_size) and let it continue.
> 
> All the other (real) IRETs are in STT_NOTYPE in the entry assembly.
> 

Right, I see.. However I'm not completely convinced by this. I must 
admit I haven't followed the whole conversation, but what was the issue 
with the HINT_IRET_SELF? It seemed more elegant, but I might be missing 
some context.

Otherwise, it might be worth having a comment in the code to point that 
this only handles the sync_core() case.


Also, instead of adding a special "arch_exception_frame_size", I could 
suggest:
- Picking this patch [1] from a completely arbitrary source
- Getting rid of INSN_STACK type, any instruction could then include 
stack ops on top of their existing semantics, they can just have an 
empty list if they don't touch SP/BP
- x86 decoder adds a stack_op to the iret to modify the stack pointer by 
the right amount

[1] https://www.spinics.net/lists/kernel/msg3453725.html

Thanks,

-- 
Julien Thierry

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Julien Thierry]

On 4/2/20 7:41 AM, Julien Thierry wrote:
> 
> 
> On 4/1/20 6:09 PM, Peter Zijlstra wrote:
>> On Wed, Apr 01, 2020 at 04:43:35PM +0100, Julien Thierry wrote:
>>>> +        return true;
>>>> +
>>>> +    if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
>>>>            return true;
>>>>
>>>> -    for (i = 0; i < CFI_NUM_REGS; i++)
>>>> +    for (i = 0; i < CFI_NUM_REGS; i++) {
>>>>            if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>>>>                state->regs[i].offset != 
>>>> initial_func_cfi.regs[i].offset)
>>>>                return true;
>>>> +    }
>>>>
>>>>        return false;
>>>>    }
>>
>>>> @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
>>>>
>>>>                break;
>>>>
>>>> +        case INSN_EXCEPTION_RETURN:
>>>> +            if (func) {
>>>> +                state.stack_size -= arch_exception_frame_size;
>>>> +                break;
>>>
>>> Why break instead of returning? Shouldn't an exception return mark 
>>> the end
>>> of a branch (whether inside or outside a function) ?
>>>
>>> Here it seems it will continue to the next instruction which might 
>>> have been
>>> unreachable.
>>
>> The code in question (x86's sync_core()), is an exception return to
>> self. It pushes an exception frame that points to right after the
>> exception return instruction.
>>
>> This is the only usage of IRET in STT_FUNC symbols.
>>
>> So rather than teaching objtool how to interpret the whole
>> push;push;push;push;push;iret sequence, teach it how big the frame is
>> (arch_exception_frame_size) and let it continue.
>>
>> All the other (real) IRETs are in STT_NOTYPE in the entry assembly.
>>
> 
> Right, I see.. However I'm not completely convinced by this. I must 
> admit I haven't followed the whole conversation, but what was the issue 
> with the HINT_IRET_SELF? It seemed more elegant, but I might be missing 
> some context.
> 
> Otherwise, it might be worth having a comment in the code to point that 
> this only handles the sync_core() case.
> 
> 
> Also, instead of adding a special "arch_exception_frame_size", I could 
> suggest:
> - Picking this patch [1] from a completely arbitrary source
> - Getting rid of INSN_STACK type, any instruction could then include 
> stack ops on top of their existing semantics, they can just have an 
> empty list if they don't touch SP/BP
> - x86 decoder adds a stack_op to the iret to modify the stack pointer by 
> the right amount
> 

And the x86 decode could also lookup the symbol containing an IRET and 
chose whether its type should be INSN_CONTEXT_SWITCH or INSN_OTHER 
depending on whether the symbol is a function or not.

This would avoid having the arch specific pattern detected the generic 
stack validation part of objtool.

-- 
Julien Thierry

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
> On 4/1/20 6:09 PM, Peter Zijlstra wrote:

> > The code in question (x86's sync_core()), is an exception return to
> > self. It pushes an exception frame that points to right after the
> > exception return instruction.
> > 
> > This is the only usage of IRET in STT_FUNC symbols.
> > 
> > So rather than teaching objtool how to interpret the whole
> > push;push;push;push;push;iret sequence, teach it how big the frame is
> > (arch_exception_frame_size) and let it continue.
> > 
> > All the other (real) IRETs are in STT_NOTYPE in the entry assembly.
> > 
> 
> Right, I see.. However I'm not completely convinced by this. I must admit I
> haven't followed the whole conversation, but what was the issue with the
> HINT_IRET_SELF? It seemed more elegant, but I might be missing some context.

https://lkml.kernel.org/r/20200331211755.pb7f3wa6oxzjnswc@treble

Josh didn't think it was worth it, I think.

> Otherwise, it might be worth having a comment in the code to point that this
> only handles the sync_core() case.

I can certainly do that. Does ARM have any ERETs sprinkled around in
places it should not have? That is, is this going to be a problem for
you?

> Also, instead of adding a special "arch_exception_frame_size", I could
> suggest:
> - Picking this patch [1] from a completely arbitrary source
> - Getting rid of INSN_STACK type, any instruction could then include stack
> ops on top of their existing semantics, they can just have an empty list if
> they don't touch SP/BP
> - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
> right amount

That's not the worst idea, lemme try that.

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Julien Thierry]

On 4/2/20 8:50 AM, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
>> On 4/1/20 6:09 PM, Peter Zijlstra wrote:
> 
>>> The code in question (x86's sync_core()), is an exception return to
>>> self. It pushes an exception frame that points to right after the
>>> exception return instruction.
>>>
>>> This is the only usage of IRET in STT_FUNC symbols.
>>>
>>> So rather than teaching objtool how to interpret the whole
>>> push;push;push;push;push;iret sequence, teach it how big the frame is
>>> (arch_exception_frame_size) and let it continue.
>>>
>>> All the other (real) IRETs are in STT_NOTYPE in the entry assembly.
>>>
>>
>> Right, I see.. However I'm not completely convinced by this. I must admit I
>> haven't followed the whole conversation, but what was the issue with the
>> HINT_IRET_SELF? It seemed more elegant, but I might be missing some context.
> 
> https://lkml.kernel.org/r/20200331211755.pb7f3wa6oxzjnswc@treble
> 
> Josh didn't think it was worth it, I think.
> 
>> Otherwise, it might be worth having a comment in the code to point that this
>> only handles the sync_core() case.
> 
> I can certainly do that. Does ARM have any ERETs sprinkled around in
> places it should not have? That is, is this going to be a problem for
> you?
> 

I had a quick look and I don't think there are ERETS in function 
symbols. And, worst case scenario, I could also just keep the arm64 
decoder making ERETS as INSN_CONTEXT_SWITCH as I didn't need more 
semantics so far (and arm64 ERET don't affect the stack anyway).

After you pointed out this only affect this very specific pattern, I 
admit that my concerns are more about "not having weird stuff in the 
generic part".
If it's too much of a hassle I can understand if you prefer to just put 
a comment. But if most of this can be kept to the arch specific decoder 
I think it'd be nicer :) .

>> Also, instead of adding a special "arch_exception_frame_size", I could
>> suggest:
>> - Picking this patch [1] from a completely arbitrary source
>> - Getting rid of INSN_STACK type, any instruction could then include stack
>> ops on top of their existing semantics, they can just have an empty list if
>> they don't touch SP/BP
>> - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
>> right amount
> 
> That's not the worst idea, lemme try that.
> 

Thanks, keep me in Cc if you post a new version using that!

Cheers,

-- 
Julien Thierry

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Peter Zijlstra]

On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:

> > Also, instead of adding a special "arch_exception_frame_size", I could
> > suggest:
> > - Picking this patch [1] from a completely arbitrary source
> > - Getting rid of INSN_STACK type, any instruction could then include stack
> > ops on top of their existing semantics, they can just have an empty list if
> > they don't touch SP/BP
> > - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
> > right amount
> 
> That's not the worst idea, lemme try that.

Something like so then?

--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -738,7 +738,6 @@ static inline void sync_core(void)
 	unsigned int tmp;
 
 	asm volatile (
-		UNWIND_HINT_SAVE
 		"mov %%ss, %0\n\t"
 		"pushq %q0\n\t"
 		"pushq %%rsp\n\t"
@@ -748,7 +747,6 @@ static inline void sync_core(void)
 		"pushq %q0\n\t"
 		"pushq $1f\n\t"
 		"iretq\n\t"
-		UNWIND_HINT_RESTORE
 		"1:"
 		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
 #endif
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -19,6 +19,7 @@ enum insn_type {
 	INSN_CALL,
 	INSN_CALL_DYNAMIC,
 	INSN_RETURN,
+	INSN_EXCEPTION_RETURN,
 	INSN_CONTEXT_SWITCH,
 	INSN_STACK,
 	INSN_BUG,
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf *
 		*type = INSN_RETURN;
 		break;
 
+	case 0xcf: /* iret */
+		*type = INSN_EXCEPTION_RETURN;
+
+		/* add $40, %rsp */
+		op->src.type = OP_SRC_ADD;
+		op->src.reg = CFI_SP;
+		op->src.offset = 5*8;
+		op->dest.type = OP_DEST_REG;
+		op->dest.reg = CFI_SP;
+		break;
+
 	case 0xca: /* retf */
 	case 0xcb: /* retf */
-	case 0xcf: /* iret */
 		*type = INSN_CONTEXT_SWITCH;
 		break;
 
@@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf *
 
 	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
 
-	if (*type == INSN_STACK)
+	if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
 		list_add_tail(&op->list, ops_list);
 	else
 		free(op);
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2224,6 +2224,20 @@ static int validate_branch(struct objtoo
 
 			break;
 
+		case INSN_EXCEPTION_RETURN:
+			if (handle_insn_ops(insn, &state))
+				return 1;
+
+			/*
+			 * This handles x86's sync_core() case, where we use an
+			 * IRET to self. All 'normal' IRET instructions are in
+			 * STT_NOTYPE entry symbols.
+			 */
+			if (func)
+				break;
+
+			return 0;
+
 		case INSN_CONTEXT_SWITCH:
 			if (func && (!next_insn || !next_insn->hint)) {
 				WARN_FUNC("unsupported instruction in callable function",

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Julien Thierry]

On 4/2/20 9:17 AM, Peter Zijlstra wrote:
> On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
> 
>>> Also, instead of adding a special "arch_exception_frame_size", I could
>>> suggest:
>>> - Picking this patch [1] from a completely arbitrary source
>>> - Getting rid of INSN_STACK type, any instruction could then include stack
>>> ops on top of their existing semantics, they can just have an empty list if
>>> they don't touch SP/BP
>>> - x86 decoder adds a stack_op to the iret to modify the stack pointer by the
>>> right amount
>>
>> That's not the worst idea, lemme try that.
> 
> Something like so then?
> 

Yes, you could even remove INSN_STACK from insn_type and just always 
call handle_insn_ops() before the switch statement on insn->type. If the 
list is empty it does nothing.
This way you wouldn't need to call it for the INSN_EXCEPTION_RETURN 
case, and any type of instructions could use stack_ops.


And the other suggestion is my other email was that you don't even need 
to add INSN_EXCEPTION_RETURN. You can keep IRET as INSN_CONTEXT_SWITCH 
by default and x86 decoder lookups the symbol conaining an iret. If it's 
a function symbol, it can just set the type to INSN_OTHER so that it 
caries on to the next instruction after having handled the stack_op.

And everything fits under tools/objtool/arch/x86 :) .

Or is it too far-fetch'd?


> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -738,7 +738,6 @@ static inline void sync_core(void)
>   	unsigned int tmp;
>   
>   	asm volatile (
> -		UNWIND_HINT_SAVE
>   		"mov %%ss, %0\n\t"
>   		"pushq %q0\n\t"
>   		"pushq %%rsp\n\t"
> @@ -748,7 +747,6 @@ static inline void sync_core(void)
>   		"pushq %q0\n\t"
>   		"pushq $1f\n\t"
>   		"iretq\n\t"
> -		UNWIND_HINT_RESTORE
>   		"1:"
>   		: "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory");
>   #endif
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -19,6 +19,7 @@ enum insn_type {
>   	INSN_CALL,
>   	INSN_CALL_DYNAMIC,
>   	INSN_RETURN,
> +	INSN_EXCEPTION_RETURN,
>   	INSN_CONTEXT_SWITCH,
>   	INSN_STACK,
>   	INSN_BUG,
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf *
>   		*type = INSN_RETURN;
>   		break;
>   
> +	case 0xcf: /* iret */
> +		*type = INSN_EXCEPTION_RETURN;
> +
> +		/* add $40, %rsp */
> +		op->src.type = OP_SRC_ADD;
> +		op->src.reg = CFI_SP;
> +		op->src.offset = 5*8;
> +		op->dest.type = OP_DEST_REG;
> +		op->dest.reg = CFI_SP;
> +		break;
> +
>   	case 0xca: /* retf */
>   	case 0xcb: /* retf */
> -	case 0xcf: /* iret */
>   		*type = INSN_CONTEXT_SWITCH;
>   		break;
>   
> @@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf *
>   
>   	*immediate = insn.immediate.nbytes ? insn.immediate.value : 0;
>   
> -	if (*type == INSN_STACK)
> +	if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN)
>   		list_add_tail(&op->list, ops_list);
>   	else
>   		free(op);
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2224,6 +2224,20 @@ static int validate_branch(struct objtoo
>   
>   			break;
>   
> +		case INSN_EXCEPTION_RETURN:
> +			if (handle_insn_ops(insn, &state))
> +				return 1;
> +
> +			/*
> +			 * This handles x86's sync_core() case, where we use an
> +			 * IRET to self. All 'normal' IRET instructions are in
> +			 * STT_NOTYPE entry symbols.
> +			 */
> +			if (func)
> +				break;
> +
> +			return 0;
> +
>   		case INSN_CONTEXT_SWITCH:
>   			if (func && (!next_insn || !next_insn->hint)) {
>   				WARN_FUNC("unsupported instruction in callable function",
> 

Cheers,

-- 
Julien Thierry

Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

[Miroslav Benes]

On Thu, 2 Apr 2020, Julien Thierry wrote:

> 
> 
> On 4/2/20 9:17 AM, Peter Zijlstra wrote:
> > On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote:
> >> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote:
> > 
> >>> Also, instead of adding a special "arch_exception_frame_size", I could
> >>> suggest:
> >>> - Picking this patch [1] from a completely arbitrary source
> >>> - Getting rid of INSN_STACK type, any instruction could then include stack
> >>> ops on top of their existing semantics, they can just have an empty list
> >>> if
> >>> they don't touch SP/BP
> >>> - x86 decoder adds a stack_op to the iret to modify the stack pointer by
> >>> the
> >>> right amount
> >>
> >> That's not the worst idea, lemme try that.
> > 
> > Something like so then?
> > 
> 
> Yes, you could even remove INSN_STACK from insn_type and just always call
> handle_insn_ops() before the switch statement on insn->type. If the list is
> empty it does nothing.
> This way you wouldn't need to call it for the INSN_EXCEPTION_RETURN case, and
> any type of instructions could use stack_ops.
> 
> 
> And the other suggestion is my other email was that you don't even need to add
> INSN_EXCEPTION_RETURN. You can keep IRET as INSN_CONTEXT_SWITCH by default and
> x86 decoder lookups the symbol conaining an iret. If it's a function symbol,
> it can just set the type to INSN_OTHER so that it caries on to the next
> instruction after having handled the stack_op.
> 
> And everything fits under tools/objtool/arch/x86 :) .
> 
> Or is it too far-fetch'd?

Imho no. Well, it depends. I can see benefits of both approach. PeterZ's 
patch is quite minimal and it demonstrates itself as a one-off hack quite 
well. On the other hand, it is in a generic code, which is not nice 
especially when other archs do not have such thing. So your proposal would 
indeed make sense to hide it in arch-specific code. Especially for the 
future. And INSN_STACK is not used much in the code, so it can be removed 
easily as you proposed.

And going more in direction of supporting more archs in the future, I'd 
say it would make sense to allow more generic things such as "forget about 
INSN_STACK. If an instruction has non-empty stack_ops list, just process 
it". It is definitely more flexible.

So yes, I think it make sense unless I am missing something.

Miroslav