Re: [PATCH v3] ftrace: Get the true parent ip for function tracer

[Steven Rostedt]

On Sat, 5 Oct 2024 10:13:20 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:

> The crash happened here:

This has been bothering me all weekend so I dug more into it.

The reason it was bothering me is because we use current later on, and it
has no issue. But then I noticed the real bug!

I was confused because the crashed instruction pointer was in the
get_current() code. But that's not where the crash actually happened.

> 
> static __always_inline unsigned long
> function_get_true_parent_ip(unsigned long parent_ip, struct ftrace_regs *fregs)
> {
>         unsigned long true_parent_ip;
>         int idx = 0;
> 
>         true_parent_ip = parent_ip;
>         if (unlikely(parent_ip == (unsigned long)&return_to_handler))
>                 true_parent_ip = ftrace_graph_ret_addr(current, &idx, parent_ip,  <<<----- CRASH

That's not the crash

>                                 (unsigned long *)fregs->regs.sp);

The above is!

fregs should *NEVER* be used directly. OK, I need to make:

struct ftrace_regs {
	void *nothing_here[];
};

Now, to stop bugs like this.

fregs is unique by architecture, and may not even be defined! That is, it
can pass NULL for fregs. And only x86 has fregs->regs available. Other
archs do not.

You must use the fregs helper functions, thus the above can be:


static __always_inline unsigned long
function_get_true_parent_ip(unsigned long parent_ip, struct ftrace_regs *fregs)
{
	unsigned long true_parent_ip;
	int idx = 0;

	true_parent_ip = parent_ip;
	if (unlikely(parent_ip == (unsigned long)&return_to_handler) && fregs)
		true_parent_ip = ftrace_graph_ret_addr(current, &idx, parent_ip,
				(unsigned long *)ftrace_regs_get_stack_pointer(fregs));

	return true_parent_ip;
}

So you can make a v5 with this update. And I'll go and make the empty
ftrace_regs structure.

Thanks!

-- Steve


>         return true_parent_ip;
> }
> 
> It appears that on some archs (x86 32 bit) the function tracer can be
> called when "current" is not set up yet, and can crash when accessing it.
> 
> So perhaps we need to add:
> 
> #ifdef CONFIG_ARCH_WANTS_NO_INSTR
> static __always_inline unsigned long
> function_get_true_parent_ip(unsigned long parent_ip, struct ftrace_regs *fregs)
> {
>         unsigned long true_parent_ip;
>         int idx = 0;
> 
>         true_parent_ip = parent_ip;
>         if (unlikely(parent_ip == (unsigned long)&return_to_handler))
>                 true_parent_ip = ftrace_graph_ret_addr(current, &idx, parent_ip,  <<<----- CRASH
>                                 (unsigned long *)fregs->regs.sp);
>         return true_parent_ip;
> }
> #else
> # define function_get_true_parent_ip(parent_ip, fregs)  parent_ip
> #endif
> 
> That is, if the arch has noinstr implemented, it should always be safe
> to access current, but if not, then there's no guarantee.
> 
> ?