Re: [PATCH v4 4/9] fprobe: rethook: Use ftrace_regs in fprobe exit handler and rethook

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Tue, 5 Sep 2023 12:30:58 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 5 Sep 2023 22:36:33 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > Yes, arch_rethook_trampoline() is good. It needs to save all registers.
> > 
> > In this series, I'm trying to change the pt_regs with ftrace_regs which will
> > reduce trampoline overhead if DYNAMIC_FTRACE_WITH_ARGS=y.
> > 
> > kprobe -> (pt_regs) -> rethook_try_hook()
> > fprobe -> (ftrace_regs) -> rethook_try_hook_ftrace() # new function
> > 
> > Thus, we need to ensure that the ftrace_regs which is saved in the ftrace
> > *without* FTRACE_WITH_REGS flags, can be used for hooking the function
> > return. I saw;
> > 
> > void arch_rethook_prepare(struct rethook_node *rh, struct pt_regs *regs, bool mcount)
> > {
> >         rh->ret_addr = regs->gprs[14];
> >         rh->frame = regs->gprs[15];
> > 
> >         /* Replace the return addr with trampoline addr */
> >         regs->gprs[14] = (unsigned long)&arch_rethook_trampoline;
> > }
> > 
> > gprs[15] is a stack pointer, so it is saved in ftrace_regs too, but what about
> > gprs[14]? (I guess it is a link register)
> > We need to read the gprs[14] and ensure that is restored to gpr14 when the
> > ftrace is exit even without FTRACE_WITH_REGS flag.
> > 
> > IOW, it is ftrace save regs/restore regs code issue. I need to check how the
> > function_graph implements it.
> 
> I would argue that the link register should also be saved in ftrace_regs.
> 
> The thing that ftrace_regs is not suppose to save is the general purpose
> registers.

Let me confirm that if ftrace_regs user changes a member of the ftrace_regs,
is that restored to the actual register when exits the ftrace too?

On x86, we just tweak the stack on memory, so I'm sure that that change is
effective, but not sure on other arch. Ah, but function_graph may also need it.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>