FUNCTION_GRAPH_TRACER + UNWINDER_FRAME_POINTER broken on arm

FUNCTION_GRAPH_TRACER + UNWINDER_FRAME_POINTER broken on arm

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 911 bytes --]

Hello,

With FUNCTION_GRAPH_TRACER=y and CONFIG_UNWINDER_FRAME_POINTER=y I
experience the following issue:

	# echo function_graph > /sys/kernel/tracing/current_tracer
	[  303.812295] Kernel panic - not syncing: �e
	[  303.816738] ---[ end Kernel panic - not syncing: �e ]---

This reproduces reliably on v6.9 (on an i.MX25 based machine).

Bisection points on v5.18-rc1~150^2~7^2~6 ("ARM: ftrace: enable
HAVE_FUNCTION_GRAPH_FP_TEST"). 

#regzbot introduced: 953f534a7ed6b725d4f101d2949393acc9262880

I didn't try to understand the issue yet, but just disabled
UNWINDER_FRAME_POINTER for now, which makes the problem disappear.
Disabling CONFIG_DYNAMIC_FTRACE is another option to work around the
problem.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: FUNCTION_GRAPH_TRACER + UNWINDER_FRAME_POINTER broken on arm

[Linux regression tracking (Thorsten Leemhuis)]

On 25.05.24 09:59, Uwe Kleine-König wrote:
> 
> With FUNCTION_GRAPH_TRACER=y and CONFIG_UNWINDER_FRAME_POINTER=y I
> experience the following issue:
> 
> 	# echo function_graph > /sys/kernel/tracing/current_tracer
> 	[  303.812295] Kernel panic - not syncing: �e
> 	[  303.816738] ---[ end Kernel panic - not syncing: �e ]---
> 
> This reproduces reliably on v6.9 (on an i.MX25 based machine).
> 
> Bisection points on v5.18-rc1~150^2~7^2~6 ("ARM: ftrace: enable
> HAVE_FUNCTION_GRAPH_FP_TEST"). 
> 
> #regzbot introduced: 953f534a7ed6b725d4f101d2949393acc9262880

Thx for involving regzbot.

> I didn't try to understand the issue yet, but just disabled
> UNWINDER_FRAME_POINTER for now, which makes the problem disappear.
> Disabling CONFIG_DYNAMIC_FTRACE is another option to work around the
> problem.

TWIMC: I had a quick off-list chat with Uwe and pointed him to an
earlier thread about problems caused by that commit that might or might
contain an solution for the problem:
https://lore.kernel.org/all/d870c149-4363-43de-b0ea-7125dec5608e@broadcom.com/

He plans to look into this and report back.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: FUNCTION_GRAPH_TRACER + UNWINDER_FRAME_POINTER broken on arm

[Thorsten Leemhuis]

On 27.05.24 10:39, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 25.05.24 09:59, Uwe Kleine-König wrote:
>
> https://lore.kernel.org/all/d870c149-4363-43de-b0ea-7125dec5608e@broadcom.com/
>
> He plans to look into this and report back.

Ohh, I see, another Thorsten just reported back in that thread after
talking with Uwe (sorry, saw that right after sending the previous msg):
https://lore.kernel.org/all/ZlQ8oDh%2FKgkm1+WE@shell.armlinux.org.uk/

Ciao, Thorsten

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: FUNCTION_GRAPH_TRACER + UNWINDER_FRAME_POINTER broken on arm

[Arnd Bergmann]

On Sat, May 25, 2024, at 09:59, Uwe Kleine-König wrote:
>
> With FUNCTION_GRAPH_TRACER=y and CONFIG_UNWINDER_FRAME_POINTER=y I
> experience the following issue:
>
> 	# echo function_graph > /sys/kernel/tracing/current_tracer
> 	[  303.812295] Kernel panic - not syncing: �e
> 	[  303.816738] ---[ end Kernel panic - not syncing: �e ]---
>
> This reproduces reliably on v6.9 (on an i.MX25 based machine).
>
> Bisection points on v5.18-rc1~150^2~7^2~6 ("ARM: ftrace: enable

I took a look at what the remaining uses of the frame pointer
unwinder are, or if we could just use the Arm unwinder
everywhere. For OABI kernels, the frame pointer unwinder is
still required of course, but the Arm unwinder is already the
default for all CONFIG_AEABI kernels and required for
all v6/v7/v7m ones. Until two years ago with commit
41918ec82eb6 ("ARM: ftrace: enable the graph tracer with
the EABI unwinder"), the frame pointer unwinder was also
required for CONFIG_FUNCTION_GRAPH_TRACER, and that is
likely why you had that configuration, but we no longer
depend on it for anything else.

However, I came across this bit from arch/arm/include/asm/ftrace.h
that seems to be the only downside of CONFIG_ARM_UNWIND:

#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
/*
 * return_address uses walk_stackframe to do it's work.  If both
 * CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses unwind
 * information.  For this to work in the function tracer many functions would
 * have to be marked with __notrace.  So for now just depend on
 * !CONFIG_ARM_UNWIND.
 */
void *return_address(unsigned int);
#else
static inline void *return_address(unsigned int level)
{
       return NULL;
}
#endif
#define ftrace_return_address(n) return_address(n)

You added this in 4bf1fa5a34aa ("[ARM] 5613/1: implement
CALLER_ADDRESSx") 15 years ago, and it appears nobody ever
got around to finish the job for return_address() if I read
this correctly. Is this something we should finally add,
and then make the frame pointer unwinder specific to OABI
kernels?

     Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: FUNCTION_GRAPH_TRACER + UNWINDER_FRAME_POINTER broken on arm

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2753 bytes --]

Hello Arnd,

On Mon, May 27, 2024 at 11:18:32AM +0200, Arnd Bergmann wrote:
> On Sat, May 25, 2024, at 09:59, Uwe Kleine-König wrote:
> >
> > With FUNCTION_GRAPH_TRACER=y and CONFIG_UNWINDER_FRAME_POINTER=y I
> > experience the following issue:
> >
> > 	# echo function_graph > /sys/kernel/tracing/current_tracer
> > 	[  303.812295] Kernel panic - not syncing: �e
> > 	[  303.816738] ---[ end Kernel panic - not syncing: �e ]---
> >
> > This reproduces reliably on v6.9 (on an i.MX25 based machine).
> >
> > Bisection points on v5.18-rc1~150^2~7^2~6 ("ARM: ftrace: enable
> 
> I took a look at what the remaining uses of the frame pointer
> unwinder are, or if we could just use the Arm unwinder
> everywhere. For OABI kernels, the frame pointer unwinder is
> still required of course, but the Arm unwinder is already the
> default for all CONFIG_AEABI kernels and required for
> all v6/v7/v7m ones. Until two years ago with commit
> 41918ec82eb6 ("ARM: ftrace: enable the graph tracer with
> the EABI unwinder"), the frame pointer unwinder was also
> required for CONFIG_FUNCTION_GRAPH_TRACER, and that is
> likely why you had that configuration, but we no longer
> depend on it for anything else.
> 
> However, I came across this bit from arch/arm/include/asm/ftrace.h
> that seems to be the only downside of CONFIG_ARM_UNWIND:
> 
> #if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
> /*
>  * return_address uses walk_stackframe to do it's work.  If both
>  * CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses unwind
>  * information.  For this to work in the function tracer many functions would
>  * have to be marked with __notrace.  So for now just depend on
>  * !CONFIG_ARM_UNWIND.
>  */
> void *return_address(unsigned int);
> #else
> static inline void *return_address(unsigned int level)
> {
>        return NULL;
> }
> #endif
> #define ftrace_return_address(n) return_address(n)
> 
> You added this in 4bf1fa5a34aa ("[ARM] 5613/1: implement
> CALLER_ADDRESSx") 15 years ago, and it appears nobody ever
> got around to finish the job for return_address() if I read
> this correctly. Is this something we should finally add,
> and then make the frame pointer unwinder specific to OABI
> kernels?

Oh, now you mention it I remember, but only on a high level and not the
deep details. I guess nobody came around finishing it because having the
dummy implementation for return_address doesn't hurt?

Completing it would mean adding the numerous __notrace annotions??

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: FUNCTION_GRAPH_TRACER + UNWINDER_FRAME_POINTER broken on arm

[Arnd Bergmann]

On Mon, May 27, 2024, at 14:25, Uwe Kleine-König wrote:
> On Mon, May 27, 2024 at 11:18:32AM +0200, Arnd Bergmann wrote:
>> On Sat, May 25, 2024, at 09:59, Uwe Kleine-König wrote:
>> 
>> You added this in 4bf1fa5a34aa ("[ARM] 5613/1: implement
>> CALLER_ADDRESSx") 15 years ago, and it appears nobody ever
>> got around to finish the job for return_address() if I read
>> this correctly. Is this something we should finally add,
>> and then make the frame pointer unwinder specific to OABI
>> kernels?
>
> Oh, now you mention it I remember, but only on a high level and not the
> deep details. I guess nobody came around finishing it because having the
> dummy implementation for return_address doesn't hurt?
>
> Completing it would mean adding the numerous __notrace annotions??

I only know what I could see in the changelog, so that
is my best guess as well. It does look like there are
only a couple of CALLER_ADDR1 and CALLER_ADDR2 sites
in the tree, and CALLER_ADDR0 should already work
as intended even without an unwinder.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: FUNCTION_GRAPH_TRACER + UNWINDER_FRAME_POINTER broken on arm

[Steven Rostedt]

On Mon, 27 May 2024 14:25:52 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> Completing it would mean adding the numerous __notrace annotions??

as long as it only affects arm.

Note, the history of HAVE_FUNCTION_GRAPH_FP_TEST is from gcc making a
copy of the stack frame, and when the function graph tracer went to
modify the return address, it only modified the copy (not the actual
return address) causing the shadow stack to become out of sync with the
real addresses and returning to the wrong address. This caused random
crashes. I added the FP_TEST to catch if this ever happens again.

See 71e308a239c09 ("function-graph: add stack frame test")

-- Steve

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel