lkml.org 
[lkml]   [2020]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v4 01/13] objtool: Remove CFI save/restore special case
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;
\
 
 \ /
  Last update: 2020-03-26 12:31    [W:0.075 / U:19.788 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site