Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in
 alternative
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: <20200414103618.12657-1-alexandre.chartre@oracle.com>
 <20200414103618.12657-7-alexandre.chartre@oracle.com>
 <20200416141843.GP20730@hirez.programming.kicks-ass.net>
From:   Alexandre Chartre <alexandre.chartre@oracle.com>
Message-ID: <806684aa-dcba-644e-540a-c88055b0d936@oracle.com>
Date:   Thu, 16 Apr 2020 16:43:18 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <20200416141843.GP20730@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 4/16/20 4:18 PM, Peter Zijlstra wrote:
> On Tue, Apr 14, 2020 at 12:36:15PM +0200, Alexandre Chartre wrote:
>> To allow a valid stack unwinding, an alternative should have code
>> where the same stack changes happens at the same places as in the
>> original code. Add a check in objtool to validate that stack changes
>> in alternative are effectively consitent with the original code.
> 
> This thing is completely buggered, it warns all over the place, even for
> obviously correct alternatives like:
> 
> 0000000000000310 <return_to_handler>:
>   310:   48 83 ec 18             sub    $0x18,%rsp
>   314:   48 89 04 24             mov    %rax,(%rsp)
>   318:   48 89 54 24 08          mov    %rdx,0x8(%rsp)
>   31d:   48 89 ef                mov    %rbp,%rdi
>   320:   e8 00 00 00 00          callq  325 <return_to_handler+0x15>
>                          321: R_X86_64_PLT32     ftrace_return_to_handler-0x4
>   325:   48 89 c7                mov    %rax,%rdi
>   328:   48 8b 54 24 08          mov    0x8(%rsp),%rdx
>   32d:   48 8b 04 24             mov    (%rsp),%rax
>   331:   48 83 c4 18             add    $0x18,%rsp
>   335:   ff e7                   jmpq   *%rdi
>   337:   90                      nop
>   338:   90                      nop
>   339:   90                      nop
> 
> 
> Where 335 has two alternatives:
> 
>     0:   e9 00 00 00 00          jmpq   5 <.altinstr_replacement+0x5>
>                          1: R_X86_64_PLT32       __x86_retpoline_rdi-0x4
> 
> and
> 
>     5:   0f ae e8                lfence
>     8:   ff e7                   jmpq   *%rdi
> 
> 
> And it then comes back with:
> 
>    defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .entry.text+0x335: error in alternative
>    defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x5: in alternative 2
>    defconfig-build/arch/x86/kernel/ftrace_64.o: warning: objtool: .altinstr_replacement+0x8: misaligned alternative state change
> 
> which is just utter crap, JMP has no (CFI) state change.

I think that's because the original nop instructions are not reached, so
they probably have an undefined stack state. Hence there's a different
stack state between 335 (jmpq *%rdi) and 337 (nop).

With the alternative, we have:

335:         lfence
338:         jmpq *%rdi

So now instruction 338 is reached with the stack state we had at 335
which is different from the original stack state at 338 (undefined)
with the unreached instruction.

Don't we have a state change recorded at 337 because the instruction
is seen as not reached?


alex.