Re: [PATCH 4/7] objtool: Add support for return trampoline call

[Alexandre Chartre]

On 4/6/20 4:34 PM, Alexandre Chartre wrote:
> On 4/2/20 5:27 PM, Peter Zijlstra wrote:
> > On Thu, Apr 02, 2020 at 10:22:17AM +0200, Alexandre Chartre wrote:
> > > With retpoline, the return instruction is used to branch to an address
> > > stored on the stack. So, unlike a regular return instruction, when a
> > > retpoline return instruction is reached the stack has been modified
> > > compared to what we have when the function was entered.
> > >
> > > Provide the mechanism to explicitly call-out such return instruction
> > > so that objtool can correctly handle them.
> >
> > https://lkml.kernel.org/r/20200331222703.GH2452@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> >
> > And also, the split out version:
> >
> > ÂÂ https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=core/objtool&id=ec9d9549901dfd2ff411676dfc624e50219e4d5a
>
> HINT_RET_OFFSET works fine when an immediate value is pushed on the
> stack. However if the value is pushed from a callee-saved register
> (%rbp, %rbx, %r12-%r15) then we still have a "return with modified
> stack frame" warning. That's because objtool checks callee-saved
> registers pushed/popped on the stack, and we have retpoline functions
> built for each register (see arch/x86/lib/retpoline.S)
>
> So that's why I also added a bool to has_modified_stack_frame() to
> no check registers:
>
> @@ -1432,7 +1478,8 @@ static bool is_fentry_call(struct instruction *insn)
>  ÂÂÂÂ return false;
>  Â}
>
> -static bool has_modified_stack_frame(struct insn_state *state)
> +static bool has_modified_stack_frame(struct insn_state *state,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ bool check_registers)
>  Â{
>  ÂÂÂÂ int i;
>
> @@ -1442,6 +1489,9 @@ static bool has_modified_stack_frame(struct insn_state *state)
>  ÂÂÂÂÂÂÂÂ state->drap)
>  ÂÂÂÂÂÂÂÂ return true;
>
> +ÂÂÂ if (!check_registers)
> +ÂÂÂÂÂÂÂ return false;
> +
>  ÂÂÂÂ for (i = 0; i < CFI_NUM_REGS; i++)
>  ÂÂÂÂÂÂÂÂ if (state->regs[i].base != initial_func_cfi.regs[i].base ||
>  ÂÂÂÂÂÂÂÂÂÂÂÂ state->regs[i].offset != initial_func_cfi.regs[i].offset)

Here is a simple change on top of the UNWIND_HINT_RET_OFFSET patch to prevent
this problem:

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index dbb2b2187037..97db8f49e06f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1390,6 +1390,14 @@ static bool has_modified_stack_frame(struct instruction *insn,
        if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
                return true;
 
+       /*
+        * If there is a ret offset hint then don't check registers
+        * because a callee-saved register might have been pushed on
+        * the stack.
+        */
+       if (ret_offset)
+               return false;
+
        for (i = 0; i < CFI_NUM_REGS; i++) {
                if (state->regs[i].base != initial_func_cfi.regs[i].base ||
                    state->regs[i].offset != initial_func_cfi.regs[i].offset)


alex.