Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 4/7] objtool: Add support for return trampoline call
From:   Alexandre Chartre <alexandre.chartre@oracle.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org, jpoimboe@redhat.com,
        jthierry@redhat.com, tglx@linutronix.de
References: <20200402082220.808-1-alexandre.chartre@oracle.com>
 <20200402082220.808-5-alexandre.chartre@oracle.com>
 <20200402152717.GE20730@hirez.programming.kicks-ass.net>
 <94272807-23c0-3eae-8312-9488607186ca@oracle.com>
Message-ID: <8a33ffae-1926-9c0c-35b2-8276c49cfe5a@oracle.com>
Date:   Mon, 6 Apr 2020 16:55:52 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.0
MIME-Version: 1.0
In-Reply-To: <94272807-23c0-3eae-8312-9488607186ca@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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.