Re: ARM Ftrace Function Graph Fails With UNWINDER_FRAME_POINTER

From: Justin Chen <justin.chen@broadcom.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Ard Biesheuvel <ardb@kernel.org>,
	mhiramat@kernel.org, mark.rutland@arm.com, linux@armlinux.org.uk,
	linux-trace-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Florian Fainelli <florian.fainelli@broadcom.com>,
	Doug Berger <doug.berger@broadcom.com>
Subject: Re: ARM Ftrace Function Graph Fails With UNWINDER_FRAME_POINTER
Date: Fri, 1 Dec 2023 14:59:45 -0800	[thread overview]
Message-ID: <4597db06-dabc-4bb2-9f24-bd30e07ff86d@broadcom.com> (raw)
In-Reply-To: <20231201130702.2824115c@gandalf.local.home>

[-- Attachment #1: Type: text/plain, Size: 1780 bytes --]

On 12/1/23 10:07 AM, Steven Rostedt wrote:
> On Fri, 1 Dec 2023 09:25:59 -0800
> Justin Chen <justin.chen@broadcom.com> wrote:
> 
>>> It appears the sub instruction at 0x6dd0 correctly accounts for the
>>> extra 8 bytes, so the frame pointer is valid. So it is our assumption
>>> that there are no gaps between the stack frames is invalid.
>>
>> Thanks for the assistance. The gap between the stack frame depends on
>> the function. Most do not have a gap. Some have 8 (as shown above), some
>> have 12. A single assumption here is not going to work. I'm having a
>> hard time finding out the reasoning for this gap. I tried disabling a
>> bunch of gcc flags as well as -O2 and the gap still exists.
> 
> That code was originally added because of some strange things that gcc did
> with mcount (for example, it made a copy of the stack frame that it passed
> to mcount, where the function graph tracer replaced the copy of the return
> stack making the shadow stack go out of sync and crash). This was very hard
> to debug and I added this code to detect it if it happened again.
> 
> Well it's been over a decade since that happened (2009).
> 
>    71e308a239c09 ("function-graph: add stack frame test")
> 
> I'm happy assuming that the compiler folks are aware of our tricks with
> hijacking return calls and I don't expect it to happen again. We can just
> rip out those checks. That is, if it's only causing false positives, I
> don't think it's worth keeping around.
> 
> Has it detected any real issues on the Arm platforms?
> 
> -- Steve

I am not familiar enough to make a call. But from my limited testing 
with ARM, I didn't see any issues. If you would like me to, I can submit 
a patch to remove the check entirely. Or maybe only disable it for ARM?

Thanks,
Justin

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]