On Sat, 5 Oct 2024 10:13:20 -0400
Steven Rostedt <[email protected]> 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.
>
> ?
