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

[Peter Zijlstra <peterz@infradead.org>]

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;