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

[Alexandre Chartre <alexandre.chartre@oracle.com>]

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@worktop.programming.kicks-ass.net
>>
>> 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.