Re: [PATCH 3/4] ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses

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

On Tue, 25 Oct 2022 11:30:38 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, Oct 25, 2022 at 05:40:01PM +0900, Masami Hiramatsu wrote:
> > Hi Mark,
> > 
> > On Mon, 24 Oct 2022 15:08:45 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > > In subsequent patches we'll arrange for architectures to have an
> > > ftrace_regs which is entirely distinct from pt_regs. In preparation for
> > > this, we need to minimize the use of pt_regs to where strictly necessary
> > > in the core ftrace code.
> > > 
> > > This patch adds new ftrace_regs_{get,set}_*() helpers which can be used
> > > to manipulate ftrace_regs. When CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=y,
> > > these can always be used on any ftrace_regs, and when
> > > CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS=n these can be used when regs are
> > > available. A new ftrace_regs_has_args(fregs) helper is added which code
> > > can use to check when these are usable.
> > 
> > Can you also add the ftrace_regs_query_register_offset() as a wrapper of
> > regs_query_register_offset()? I would like to use it for fprobe_events.
> 
> Sure!
> 
> Just to check, with FTRACE_WITH_REGS, does fprobe always sample the full
> pt_regs, or do callers also need to check ftrace_regs_has_args(fregs)?

No, please return -ENOENT or any error value if the given register
is not saved on arm64. Others will just return
 regs_query_register_offset(&fregs->regs, name). That is enough
at this moment. Later we can improve it.

> I ask because if neither of those are the case, with FTRACE_WITH_REGS,
> ftrace_regs_query_register_offset() would accept names of registers which might
> not have been sampled, and could give offsets to uninitialized memory.

Currently fprobe depends on CONFIG_HAVE_DYNAMIC_FTRACE_WITH_REGS, but
in the future, I will move it on WITH_ARGS.

> Atop that, I'm not exactly sure what to implement for powerpc/s390/x86 here. If
> those might be used without a full pt_regs, I think
> ftrace_regs_query_register_offset() should also take the fregs as a parameter
> and use that to check which registers are available.
> 
> ... does that make sense to you?

Yeah, that is OK. I think only arm64 changes the ftrace_regs not wraps
pt_regs. So there is no problem even if we access the empty register.
Only arm64 implementation is different, so it should have different
implementation.

Thank you,

> 
> Thanks,
> Mark.
> 
> > 
> > Thank you,
> > 
> > > 
> > > Co-developed-by: Florent Revest <revest@chromium.org>
> > > Signed-off-by: Florent Revest <revest@chromium.org>
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > ---
> > >  arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
> > >  arch/s390/include/asm/ftrace.h    | 17 +++++++++++++++++
> > >  arch/x86/include/asm/ftrace.h     | 14 ++++++++++++++
> > >  include/linux/ftrace.h            | 27 +++++++++++++++++++++++++++
> > >  kernel/trace/Kconfig              |  6 +++---
> > >  5 files changed, 78 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> > > index c3eb48f67566..faecb20d78bf 100644
> > > --- a/arch/powerpc/include/asm/ftrace.h
> > > +++ b/arch/powerpc/include/asm/ftrace.h
> > > @@ -44,6 +44,23 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> > >  	regs_set_return_ip(&fregs->regs, ip);
> > >  }
> > >  
> > > +static __always_inline unsigned long
> > > +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> > > +{
> > > +	return instruction_pointer(&fregs->regs)
> > > +}
> > > +
> > > +#define ftrace_regs_get_argument(fregs, n) \
> > > +	regs_get_kernel_argument(&(fregs)->regs, n)
> > > +#define ftrace_regs_get_stack_pointer(fregs) \
> > > +	kernel_stack_pointer(&(fregs)->regs)
> > > +#define ftrace_regs_return_value(fregs) \
> > > +	regs_return_value(&(fregs)->regs)
> > > +#define ftrace_regs_set_return_value(fregs, ret) \
> > > +	regs_set_return_value(&(fregs)->regs, ret)
> > > +#define ftrace_override_function_with_return(fregs) \
> > > +	override_function_with_return(&(fregs)->regs)
> > > +
> > >  struct ftrace_ops;
> > >  
> > >  #define ftrace_graph_func ftrace_graph_func
> > > diff --git a/arch/s390/include/asm/ftrace.h b/arch/s390/include/asm/ftrace.h
> > > index b8957882404f..5fdc806458aa 100644
> > > --- a/arch/s390/include/asm/ftrace.h
> > > +++ b/arch/s390/include/asm/ftrace.h
> > > @@ -54,6 +54,12 @@ static __always_inline struct pt_regs *arch_ftrace_get_regs(struct ftrace_regs *
> > >  	return NULL;
> > >  }
> > >  
> > > +static __always_inline unsigned long
> > > +ftrace_regs_get_instruction_pointer(const struct ftrace_regs *fregs)
> > > +{
> > > +	return fregs->regs.psw.addr;
> > > +}
> > > +
> > >  static __always_inline void
> > >  ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> > >  				    unsigned long ip)
> > > @@ -61,6 +67,17 @@ ftrace_regs_set_instruction_pointer(struct ftrace_regs *fregs,
> > >  	fregs->regs.psw.addr = ip;
> > >  }
> > >  
> > > +#define ftrace_regs_get_argument(fregs, n) \
> > > +	regs_get_kernel_argument(&(fregs)->regs, n)
> > > +#define ftrace_regs_get_stack_pointer(fregs) \
> > > +	kernel_stack_pointer(&(fregs)->regs)
> > > +#define ftrace_regs_return_value(fregs) \
> > > +	regs_return_value(&(fregs)->regs)
> > > +#define ftrace_regs_set_return_value(fregs, ret) \
> > > +	regs_set_return_value(&(fregs)->regs, ret)
> > > +#define ftrace_override_function_with_return(fregs) \
> > > +	override_function_with_return(&(fregs)->regs)
> > > +
> > >  /*
> > >   * When an ftrace registered caller is tracing a function that is
> > >   * also set by a register_ftrace_direct() call, it needs to be
> > > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > > index b73e858bd96f..b3737b42e8a1 100644
> > > --- a/arch/x86/include/asm/ftrace.h
> > > +++ b/arch/x86/include/asm/ftrace.h
> > > @@ -51,6 +51,20 @@ arch_ftrace_get_regs(struct ftrace_regs *fregs)
> > >  #define ftrace_regs_set_instruction_pointer(fregs, _ip)	\
> > >  	do { (fregs)->regs.ip = (_ip); } while (0)
> > >  
> > > +#define ftrace_regs_get_instruction_pointer(fregs) \
> > > +	((fregs)->regs.ip)
> > > +
> > > +#define ftrace_regs_get_argument(fregs, n) \
> > > +	regs_get_kernel_argument(&(fregs)->regs, n)
> > > +#define ftrace_regs_get_stack_pointer(fregs) \
> > > +	kernel_stack_pointer(&(fregs)->regs)
> > > +#define ftrace_regs_return_value(fregs) \
> > > +	regs_return_value(&(fregs)->regs)
> > > +#define ftrace_regs_set_return_value(fregs, ret) \
> > > +	regs_set_return_value(&(fregs)->regs, ret)
> > > +#define ftrace_override_function_with_return(fregs) \
> > > +	override_function_with_return(&(fregs)->regs)
> > > +
> > >  struct ftrace_ops;
> > >  #define ftrace_graph_func ftrace_graph_func
> > >  void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > index e9905f741916..3b13e3c21438 100644
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -125,6 +125,33 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
> > >  	return arch_ftrace_get_regs(fregs);
> > >  }
> > >  
> > > +/*
> > > + * When true, the ftrace_regs_{get,set}_*() functions may be used on fregs.
> > > + * Note: this can be true even when ftrace_get_regs() cannot provide a pt_regs.
> > > + */
> > > +static __always_inline bool ftrace_regs_has_args(struct ftrace_regs *fregs)
> > > +{
> > > +	if (IS_ENABLED(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS))
> > > +		return true;
> > > +
> > > +	return ftrace_get_regs(fregs) != NULL;
> > > +}
> > > +
> > > +#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > > +#define ftrace_regs_get_instruction_pointer(fregs) \
> > > +	instruction_pointer(ftrace_get_regs(fregs))
> > > +#define ftrace_regs_get_argument(fregs, n) \
> > > +	regs_get_kernel_argument(ftrace_get_regs(fregs), n)
> > > +#define ftrace_regs_get_stack_pointer(fregs) \
> > > +	kernel_stack_pointer(ftrace_get_regs(fregs))
> > > +#define ftrace_regs_return_value(fregs) \
> > > +	regs_return_value(ftrace_get_regs(fregs))
> > > +#define ftrace_regs_set_return_value(fregs, ret) \
> > > +	regs_set_return_value(ftrace_get_regs(fregs), ret)
> > > +#define ftrace_override_function_with_return(fregs) \
> > > +	override_function_with_return(ftrace_get_regs(fregs))
> > > +#endif
> > > +
> > >  typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
> > >  			      struct ftrace_ops *op, struct ftrace_regs *fregs);
> > >  
> > > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > > index e9e95c790b8e..2c6611c13f99 100644
> > > --- a/kernel/trace/Kconfig
> > > +++ b/kernel/trace/Kconfig
> > > @@ -46,10 +46,10 @@ config HAVE_DYNAMIC_FTRACE_WITH_ARGS
> > >  	bool
> > >  	help
> > >  	 If this is set, then arguments and stack can be found from
> > > -	 the pt_regs passed into the function callback regs parameter
> > > +	 the ftrace_regs passed into the function callback regs parameter
> > >  	 by default, even without setting the REGS flag in the ftrace_ops.
> > > -	 This allows for use of regs_get_kernel_argument() and
> > > -	 kernel_stack_pointer().
> > > +	 This allows for use of ftrace_regs_get_argument() and
> > > +	 ftrace_regs_get_stack_pointer().
> > >  
> > >  config HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
> > >  	bool
> > > -- 
> > > 2.30.2
> > > 
> > 
> > 
> > -- 
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


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

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