Messages in this thread Patch in this message | | | Date | Thu, 26 Mar 2020 12:30:49 +0100 | From | Peter Zijlstra <> | Subject | Re: [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;
| |