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:3.148 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site