[3/4] ftrace: abstract DYNAMIC_FTRACE_WITH_ARGS accesses

  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.

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(-)
  

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.

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
>
  

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)?

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.

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?

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>
  

On Tue, Oct 25, 2022 at 12:30 PM 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)?

Currently, we have:
config FPROBE
    depends on DYNAMIC_FTRACE_WITH_REGS

Because fprobe registers its ftrace ops with FTRACE_OPS_FL_SAVE_REGS
and calls ftrace_get_regs() to give pt_regs to registered fprobe
callbacks.

We'll need to refactor fprobe a bit to support ||
DYNAMIC_FTRACE_WITH_ARGS and therefore work on arm64.

> 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.

Indeed, if one were to call the regs_query_register_offset() on a
pt_regs that comes out of a ftrace trampoline with FTRACE_WITH_REGS,
for example in a fprobe callback, one could get offsets to
uninitialized memory already (for the registers we can't get outside
of an exception handler on arm64 for example iiuc)

And it'd get way worse with FTRACE_WITH_ARGS if we implement
ftrace_regs_query_register_offset() by calling
regs_query_register_offset() for ftrace_regs that contain sparse
pt_regs.

> 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.

I think it would make sense for a ftrace_regs_query_register_offset()
to only return offsets to the registers that are actually saved by the
arch in a ftrace_regs (whether that's WITH_ARGS or WITH_REGS).

But that also means that if we introduce "fprobe_events" in the
tracing sysfs interface, we can't have it support a %REG syntax
compatible with the one in "kprobe_events" anyway.

Masami, how about having "fprobe_events" only support $argN, $retval
etc but no %REG, from the beginning ? Then it would be clear to users
that fprobe can not guarantee registers and we'd never have to fake
registers when we don't have them. Users would have to make a decision
between using fprobe which is fast but only has arguments and return
value and kprobe which is slow but has all registers.

I realize this has consequences for the kretprobe and rethook
unification plan but if we have fprobe_events support %REG at the
beginning, we'd have to break it at some point down the line anyway
right ?

> ... does that make sense to you?
>
> Thanks,
> Mark.
  

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>
  

On Tue, 25 Oct 2022 15:20:57 +0200
Florent Revest <revest@chromium.org> wrote:

> On Tue, Oct 25, 2022 at 12:30 PM 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, if the register is NOT saved, just return -ENOENT.

> 
> Currently, we have:
> config FPROBE
>     depends on DYNAMIC_FTRACE_WITH_REGS
> 
> Because fprobe registers its ftrace ops with FTRACE_OPS_FL_SAVE_REGS
> and calls ftrace_get_regs() to give pt_regs to registered fprobe
> callbacks.
> 
> We'll need to refactor fprobe a bit to support ||
> DYNAMIC_FTRACE_WITH_ARGS and therefore work on arm64.

Yeah, that is what I will do.

> > 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.
> 
> Indeed, if one were to call the regs_query_register_offset() on a
> pt_regs that comes out of a ftrace trampoline with FTRACE_WITH_REGS,
> for example in a fprobe callback, one could get offsets to
> uninitialized memory already (for the registers we can't get outside
> of an exception handler on arm64 for example iiuc)
> 
> And it'd get way worse with FTRACE_WITH_ARGS if we implement
> ftrace_regs_query_register_offset() by calling
> regs_query_register_offset() for ftrace_regs that contain sparse
> pt_regs.

Ah, I got what you meant. If you consider that case, it can return
-ENOTSUPPORT for FTRACE_WITH_ARGS case at this moment. I'll fill it
afterwards.

> 
> > 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.
> 
> I think it would make sense for a ftrace_regs_query_register_offset()
> to only return offsets to the registers that are actually saved by the
> arch in a ftrace_regs (whether that's WITH_ARGS or WITH_REGS).
> 
> But that also means that if we introduce "fprobe_events" in the
> tracing sysfs interface, we can't have it support a %REG syntax
> compatible with the one in "kprobe_events" anyway.
> 
> Masami, how about having "fprobe_events" only support $argN, $retval
> etc but no %REG, from the beginning ? Then it would be clear to users
> that fprobe can not guarantee registers and we'd never have to fake
> registers when we don't have them. Users would have to make a decision
> between using fprobe which is fast but only has arguments and return
> value and kprobe which is slow but has all registers.

Hmm, for the first implementation, it is OK. But later I need to
implement the register access, because, debuginfo maps it differently.
I would like to arrow perf-probe to set it up eventually.

> I realize this has consequences for the kretprobe and rethook
> unification plan but if we have fprobe_events support %REG at the
> beginning, we'd have to break it at some point down the line anyway
> right ?

No, because %REG support doesn't mean we guarantee to access
all registers. We can just use %REG as alias of $argN :)

Thank you,

> 
> > ... does that make sense to you?
> >
> > Thanks,
> > Mark.
  

On Wed, Oct 26, 2022 at 12:17:54AM +0900, Masami Hiramatsu wrote:
> 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.

Sure, that's what I intend to implement for arm64. I'll use -EINVAL to match
the existing regs_query_register_offset() logic.

> Others will just return regs_query_register_offset(&fregs->regs, name). That
> is enough at this moment. Later we can improve it.

Sorry, what I was trying to ask was whether fprobe currently always set
FTRACE_OPS_FL_SAVE_REGS (which AFAICT it does); so I now agree that's
sufficient -- sorry for the noise!

Thanks,
Mark.