Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 26 Mar 2020 12:30:49 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     tglx@linutronix.de, jpoimboe@redhat.com
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, mhiramat@kernel.org,
        mbenes@suse.cz
Subject: Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case
Message-ID: <20200326113049.GD20696@hirez.programming.kicks-ass.net>
References: <20200325174525.772641599@infradead.org>
 <20200325174605.369570202@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200325174605.369570202@infradead.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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;