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 v2 1/3] objtool: check: Fully validate the stack frame
To:     Josh Poimboeuf <jpoimboe@redhat.com>
Cc:     linux-kernel@vger.kernel.org, peterz@infradead.org, mbenes@suse.cz,
        raphael.gault@arm.com, benh@kernel.crashing.org
References: <20200928093631.210610-1-jthierry@redhat.com>
 <20200928093631.210610-2-jthierry@redhat.com>
 <20200929191834.7daofidv6b5aef3y@treble>
From:   Julien Thierry <jthierry@redhat.com>
Message-ID: <5540c2a9-db13-e05e-713c-22ec00f21aa6@redhat.com>
Date:   Mon, 12 Oct 2020 11:21:49 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20200929191834.7daofidv6b5aef3y@treble>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 9/29/20 8:18 PM, Josh Poimboeuf wrote:
> On Mon, Sep 28, 2020 at 10:36:29AM +0100, Julien Thierry wrote:
>> +++ b/tools/objtool/arch/x86/include/cfi_regs.h
>> @@ -22,4 +22,7 @@
>>   #define CFI_RA			16
>>   #define CFI_NUM_REGS		17
> 
> A few more naming nitpicks:
> 
>> +#define STACKFRAME_BP_OFFSET	-16
>> +#define STACKFRAME_RA_OFFSET	-8
> 
> "Stack frame" has more than one meaning now, I suppose.  i.e. it could
> also include the callee-saved registers and any other stack space
> allocated by the function.
> 
> Would "call frame" be clearer?
> 
>    CALL_FRAME_BP_OFFSET
>    CALL_FRAME_RA_OFFSET
> 
> ?

I would've thought that the call-frame could include the stackframe + 
other callee saved regs. Whereas stackframe tends to used for the 
caller's frame pointer + return address (i.e. what allows unwinding). 
Unless I'm getting lost with things.

And if call frame is associated with the region starting from the stack 
pointer at the parent call point (since this is what CFA is), then it 
shouldn't be associated with the framepointer + return address structure 
since this could be anywhere on the call frame (not at a fixed offset) 
as long as the new frame pointer points to the structure.

> 
>> +++ b/tools/objtool/cfi.h
>> @@ -35,4 +35,6 @@ struct cfi_state {
>>   	bool end;
>>   };
>>   
>> +#define STACKFRAME_SIZE	16
> 
> CALL_FRAME_SIZE ?
> 
> I'm sort of contradicting my previous comment here, but even though this
> value may be generic, it's also very much intertwined with the
> CALL_FRAME_{BP|RA}_OFFSET values.  So I get the feeling it really
> belongs in the arch-specific cfi_regs.h next to the other defines after
> all.
> 

Agreed.

-- 
Julien Thierry