diff mbox series

[v2,2/2] kernel/trace: Remove function callback casts

Message ID 20200719155033.24201-3-oscar.carter@gmx.com (mailing list archive)
State New, archived
Headers show
Series kernel/trace: Remove function callback casts | expand

Commit Message

Oscar Carter July 19, 2020, 3:50 p.m. UTC
In an effort to enable -Wcast-function-type in the top-level Makefile to
support Control Flow Integrity builds, there are the need to remove all
the function callback casts.

ftrace_ops_list_func() can no longer be defined as ftrace_ops_no_ops().
The reason for ftrace_ops_no_ops() is to use that when an architecture
calls ftrace_ops_list_func() with only two parameters (called from
assembly). And to make sure there's no C side-effects, those archs call
ftrace_ops_no_ops() which only has two parameters, as the function
ftrace_ops_list_func() has four parameters.

This patch removes the no longer needed function ftrace_ops_no_ops() and
all the function callback casts using the previous defined ftrace_func
union and the two function helpers called ftrace_set_ufunc() and
ftrace_same_address_ufunc().

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
 kernel/trace/ftrace.c | 48 ++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 19 deletions(-)

--
2.20.1

Comments

Steven Rostedt July 21, 2020, 6:05 p.m. UTC | #1
On Sun, 19 Jul 2020 17:50:33 +0200
Oscar Carter <oscar.carter@gmx.com> wrote:

> In an effort to enable -Wcast-function-type in the top-level Makefile to
> support Control Flow Integrity builds, there are the need to remove all
> the function callback casts.
> 
> ftrace_ops_list_func() can no longer be defined as ftrace_ops_no_ops().
> The reason for ftrace_ops_no_ops() is to use that when an architecture
> calls ftrace_ops_list_func() with only two parameters (called from
> assembly). And to make sure there's no C side-effects, those archs call
> ftrace_ops_no_ops() which only has two parameters, as the function
> ftrace_ops_list_func() has four parameters.
> 
> This patch removes the no longer needed function ftrace_ops_no_ops() and
> all the function callback casts using the previous defined ftrace_func
> union and the two function helpers called ftrace_set_ufunc() and
> ftrace_same_address_ufunc().

Ug, I think I prefer the linker trick better.

> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
>  kernel/trace/ftrace.c | 48 ++++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index fd8fbb422860..124ccf914657 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -143,9 +143,7 @@ static inline bool ftrace_same_address_ufunc(union ftrace_func *ufunc,
>  	return (ufunc->ops == func);
>  }
>  #else
> -/* See comment below, where ftrace_ops_list_func is defined */
> -static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip);
> -#define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
> +static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip);
> 
>  static inline void ftrace_set_ufunc(union ftrace_func *ufunc,
>  				    ftrace_func_no_ops_t func)
> @@ -198,22 +196,29 @@ static void ftrace_sync_ipi(void *data)
>  	smp_rmb();
>  }
> 
> -static ftrace_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
> +static union ftrace_func ftrace_ops_get_list_func(struct ftrace_ops *ops)
>  {
> +	union ftrace_func list_func;
> +
>  	/*
>  	 * If this is a dynamic, RCU, or per CPU ops, or we force list func,
>  	 * then it needs to call the list anyway.
>  	 */
>  	if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_RCU) ||
>  	    FTRACE_FORCE_LIST_FUNC)
> -		return ftrace_ops_list_func;
> +		ftrace_set_ufunc(&list_func, ftrace_ops_list_func);
> +	else
> +		list_func.ops = ftrace_ops_get_func(ops);
> 
> -	return ftrace_ops_get_func(ops);
> +	return list_func;

Is this the same as returning a pointer? It makes me very nervous about
returning a union. Can a compiler return something allocated on the stack?

Also, don't use "ufunc" as that makes me think its a user space variable.



>  }
> 
>  static void update_ftrace_function(void)
>  {
> -	ftrace_func_t func;
> +	union ftrace_func func;
> +#ifndef CONFIG_DYNAMIC_FTRACE
> +	union ftrace_func tmp;
> +#endif
> 
>  	/*
>  	 * Prepare the ftrace_ops that the arch callback will use.
> @@ -225,7 +230,7 @@ static void update_ftrace_function(void)
> 
>  	/* If there's no ftrace_ops registered, just call the stub function */
>  	if (set_function_trace_op == &ftrace_list_end) {
> -		func = ftrace_stub;
> +		func.ops = ftrace_stub;
> 
>  	/*
>  	 * If we are at the end of the list and this ops is
> @@ -239,21 +244,21 @@ static void update_ftrace_function(void)
>  	} else {
>  		/* Just use the default ftrace_ops */
>  		set_function_trace_op = &ftrace_list_end;
> -		func = ftrace_ops_list_func;
> +		ftrace_set_ufunc(&func, ftrace_ops_list_func);
>  	}
> 
>  	update_function_graph_func();
> 
>  	/* If there's no change, then do nothing more here */
> -	if (ftrace_trace_function == func)
> +	if (ftrace_trace_function == func.ops)
>  		return;
> 
>  	/*
>  	 * If we are using the list function, it doesn't care
>  	 * about the function_trace_ops.
>  	 */
> -	if (func == ftrace_ops_list_func) {
> -		ftrace_trace_function = func;
> +	if (ftrace_same_address_ufunc(&func, ftrace_ops_list_func)) {
> +		ftrace_trace_function = func.ops;
>  		/*
>  		 * Don't even bother setting function_trace_ops,
>  		 * it would be racy to do so anyway.
> @@ -272,7 +277,9 @@ static void update_ftrace_function(void)
>  	 * function we want, albeit indirectly, but it handles the
>  	 * ftrace_ops and doesn't depend on function_trace_op.
>  	 */
> -	ftrace_trace_function = ftrace_ops_list_func;
> +	ftrace_set_ufunc(&tmp, ftrace_ops_list_func);
> +	ftrace_trace_function = tmp.ops;
> +
>  	/*
>  	 * Make sure all CPUs see this. Yes this is slow, but static
>  	 * tracing is slow and nasty to have enabled.
> @@ -287,7 +294,7 @@ static void update_ftrace_function(void)
>  	/* OK, we are all set to update the ftrace_trace_function now! */
>  #endif /* !CONFIG_DYNAMIC_FTRACE */
> 
> -	ftrace_trace_function = func;
> +	ftrace_trace_function = func.ops;
>  }

This looks really intrusive for what it's trying to accomplish.

The linker trick is far less intrusive, and I believe less error prone.

-- Steve


> 
>  static void add_ftrace_ops(struct ftrace_ops __rcu **list,
> @@ -2680,6 +2687,7 @@ void ftrace_modify_all_code(int command)
>  	int update = command & FTRACE_UPDATE_TRACE_FUNC;
>  	int mod_flags = 0;
>  	int err = 0;
> +	union ftrace_func func;
> 
>  	if (command & FTRACE_MAY_SLEEP)
>  		mod_flags = FTRACE_MODIFY_MAY_SLEEP_FL;
> @@ -2695,7 +2703,8 @@ void ftrace_modify_all_code(int command)
>  	 * traced.
>  	 */
>  	if (update) {
> -		err = ftrace_update_ftrace_func(ftrace_ops_list_func);
> +		ftrace_set_ufunc(&func, ftrace_ops_list_func);
> +		err = ftrace_update_ftrace_func(func.ops);
>  		if (FTRACE_WARN_ON(err))
>  			return;
>  	}
> @@ -2705,7 +2714,9 @@ void ftrace_modify_all_code(int command)
>  	else if (command & FTRACE_DISABLE_CALLS)
>  		ftrace_replace_code(mod_flags);
> 
> -	if (update && ftrace_trace_function != ftrace_ops_list_func) {
> +	ftrace_set_ufunc(&func, ftrace_ops_list_func);
> +
> +	if (update && ftrace_trace_function != func.ops) {
>  		function_trace_op = set_function_trace_op;
>  		smp_wmb();
>  		/* If irqs are disabled, we are in stop machine */
> @@ -6890,14 +6901,13 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
>  {
>  	__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
>  }
> -NOKPROBE_SYMBOL(ftrace_ops_list_func);
>  #else
> -static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
> +static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
>  {
>  	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
>  }
> -NOKPROBE_SYMBOL(ftrace_ops_no_ops);
>  #endif
> +NOKPROBE_SYMBOL(ftrace_ops_list_func);
> 
>  /*
>   * If there's only one function registered but it does not support
> --
> 2.20.1
Oscar Carter July 24, 2020, 4:19 p.m. UTC | #2
Hi Steven,

On Tue, Jul 21, 2020 at 02:05:45PM -0400, Steven Rostedt wrote:
> On Sun, 19 Jul 2020 17:50:33 +0200
> Oscar Carter <oscar.carter@gmx.com> wrote:
>
> > In an effort to enable -Wcast-function-type in the top-level Makefile to
> > support Control Flow Integrity builds, there are the need to remove all
> > the function callback casts.
> >
> > ftrace_ops_list_func() can no longer be defined as ftrace_ops_no_ops().
> > The reason for ftrace_ops_no_ops() is to use that when an architecture
> > calls ftrace_ops_list_func() with only two parameters (called from
> > assembly). And to make sure there's no C side-effects, those archs call
> > ftrace_ops_no_ops() which only has two parameters, as the function
> > ftrace_ops_list_func() has four parameters.
> >
> > This patch removes the no longer needed function ftrace_ops_no_ops() and
> > all the function callback casts using the previous defined ftrace_func
> > union and the two function helpers called ftrace_set_ufunc() and
> > ftrace_same_address_ufunc().
>
> Ug, I think I prefer the linker trick better.
>
> >
> > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > ---
> >  kernel/trace/ftrace.c | 48 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 29 insertions(+), 19 deletions(-)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index fd8fbb422860..124ccf914657 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -143,9 +143,7 @@ static inline bool ftrace_same_address_ufunc(union ftrace_func *ufunc,
> >  	return (ufunc->ops == func);
> >  }
> >  #else
> > -/* See comment below, where ftrace_ops_list_func is defined */
> > -static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip);
> > -#define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
> > +static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip);
> >
> >  static inline void ftrace_set_ufunc(union ftrace_func *ufunc,
> >  				    ftrace_func_no_ops_t func)
> > @@ -198,22 +196,29 @@ static void ftrace_sync_ipi(void *data)
> >  	smp_rmb();
> >  }
> >
> > -static ftrace_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
> > +static union ftrace_func ftrace_ops_get_list_func(struct ftrace_ops *ops)
> >  {
> > +	union ftrace_func list_func;
> > +
> >  	/*
> >  	 * If this is a dynamic, RCU, or per CPU ops, or we force list func,
> >  	 * then it needs to call the list anyway.
> >  	 */
> >  	if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_RCU) ||
> >  	    FTRACE_FORCE_LIST_FUNC)
> > -		return ftrace_ops_list_func;
> > +		ftrace_set_ufunc(&list_func, ftrace_ops_list_func);
> > +	else
> > +		list_func.ops = ftrace_ops_get_func(ops);
> >
> > -	return ftrace_ops_get_func(ops);
> > +	return list_func;
>
> Is this the same as returning a pointer? It makes me very nervous about
> returning a union. Can a compiler return something allocated on the stack?

Yes, it can. The union is returned by value (copied). So, there is no
problem.

If you don't like to return a union by value, I can modify the prototype
of the ftrace_ops_get_list_func() to the following one:

static void ftrace_ops_get_list_func(struct ftrace_ops *ops,
				     union ftrace_func *list_func)

This way the old local variable "list_func" is removed and we need to pass
the address of a union to this function. Then the function modify the union.

If you prefer this option I have no problem with the change.

> Also, don't use "ufunc" as that makes me think its a user space variable.

Ok, would "bfunc" or "jfunc" be better?
bfunc => blend - function
jfunc => join - function

> >  }
> >
> >  static void update_ftrace_function(void)
> >  {
> > -	ftrace_func_t func;
> > +	union ftrace_func func;
> > +#ifndef CONFIG_DYNAMIC_FTRACE
> > +	union ftrace_func tmp;
> > +#endif
> >
> >  	/*
> >  	 * Prepare the ftrace_ops that the arch callback will use.
> > @@ -225,7 +230,7 @@ static void update_ftrace_function(void)
> >
> >  	/* If there's no ftrace_ops registered, just call the stub function */
> >  	if (set_function_trace_op == &ftrace_list_end) {
> > -		func = ftrace_stub;
> > +		func.ops = ftrace_stub;
> >
> >  	/*
> >  	 * If we are at the end of the list and this ops is
> > @@ -239,21 +244,21 @@ static void update_ftrace_function(void)
> >  	} else {
> >  		/* Just use the default ftrace_ops */
> >  		set_function_trace_op = &ftrace_list_end;
> > -		func = ftrace_ops_list_func;
> > +		ftrace_set_ufunc(&func, ftrace_ops_list_func);
> >  	}
> >
> >  	update_function_graph_func();
> >
> >  	/* If there's no change, then do nothing more here */
> > -	if (ftrace_trace_function == func)
> > +	if (ftrace_trace_function == func.ops)
> >  		return;
> >
> >  	/*
> >  	 * If we are using the list function, it doesn't care
> >  	 * about the function_trace_ops.
> >  	 */
> > -	if (func == ftrace_ops_list_func) {
> > -		ftrace_trace_function = func;
> > +	if (ftrace_same_address_ufunc(&func, ftrace_ops_list_func)) {
> > +		ftrace_trace_function = func.ops;
> >  		/*
> >  		 * Don't even bother setting function_trace_ops,
> >  		 * it would be racy to do so anyway.
> > @@ -272,7 +277,9 @@ static void update_ftrace_function(void)
> >  	 * function we want, albeit indirectly, but it handles the
> >  	 * ftrace_ops and doesn't depend on function_trace_op.
> >  	 */
> > -	ftrace_trace_function = ftrace_ops_list_func;
> > +	ftrace_set_ufunc(&tmp, ftrace_ops_list_func);
> > +	ftrace_trace_function = tmp.ops;
> > +
> >  	/*
> >  	 * Make sure all CPUs see this. Yes this is slow, but static
> >  	 * tracing is slow and nasty to have enabled.
> > @@ -287,7 +294,7 @@ static void update_ftrace_function(void)
> >  	/* OK, we are all set to update the ftrace_trace_function now! */
> >  #endif /* !CONFIG_DYNAMIC_FTRACE */
> >
> > -	ftrace_trace_function = func;
> > +	ftrace_trace_function = func.ops;
> >  }
>
> This looks really intrusive for what it's trying to accomplish.

What this patch is trying to accomplish is to remove the warning
-Wcast-function-type in a way that allows the CFI build to get the
necessary info about the prototypes (4 parameters or 2 parameters) of
the functions used for archs that support ftrace ops and for the archs
that don't support ftrace ops. This info is necessary to allow the
compiler to insert a check to guarantee that an indirect call can only
jump to functions that match the prototype. This way, the attack surface
is reduced if an attacker can get some control over the system.

> The linker trick is far less intrusive, and I believe less error prone.

If we use the linker trick, the warning -Wcast-function-type dissapears,
but in a way that makes impossible to the compiler to get the necessary
info about function prototypes to insert the commented check. As far I
know, this linker trick (redirection of a function) is hidden for the
CFI build.

So, in my opinion, the linker trick is not suitable if we want to protect
the function pointers of the ftrace subsystem against an attack that
modifiy the normal flow of the kernel.

> -- Steve

Thanks,
Oscar Carter

> >
> >  static void add_ftrace_ops(struct ftrace_ops __rcu **list,
> > @@ -2680,6 +2687,7 @@ void ftrace_modify_all_code(int command)
> >  	int update = command & FTRACE_UPDATE_TRACE_FUNC;
> >  	int mod_flags = 0;
> >  	int err = 0;
> > +	union ftrace_func func;
> >
> >  	if (command & FTRACE_MAY_SLEEP)
> >  		mod_flags = FTRACE_MODIFY_MAY_SLEEP_FL;
> > @@ -2695,7 +2703,8 @@ void ftrace_modify_all_code(int command)
> >  	 * traced.
> >  	 */
> >  	if (update) {
> > -		err = ftrace_update_ftrace_func(ftrace_ops_list_func);
> > +		ftrace_set_ufunc(&func, ftrace_ops_list_func);
> > +		err = ftrace_update_ftrace_func(func.ops);
> >  		if (FTRACE_WARN_ON(err))
> >  			return;
> >  	}
> > @@ -2705,7 +2714,9 @@ void ftrace_modify_all_code(int command)
> >  	else if (command & FTRACE_DISABLE_CALLS)
> >  		ftrace_replace_code(mod_flags);
> >
> > -	if (update && ftrace_trace_function != ftrace_ops_list_func) {
> > +	ftrace_set_ufunc(&func, ftrace_ops_list_func);
> > +
> > +	if (update && ftrace_trace_function != func.ops) {
> >  		function_trace_op = set_function_trace_op;
> >  		smp_wmb();
> >  		/* If irqs are disabled, we are in stop machine */
> > @@ -6890,14 +6901,13 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> >  {
> >  	__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
> >  }
> > -NOKPROBE_SYMBOL(ftrace_ops_list_func);
> >  #else
> > -static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
> > +static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
> >  {
> >  	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
> >  }
> > -NOKPROBE_SYMBOL(ftrace_ops_no_ops);
> >  #endif
> > +NOKPROBE_SYMBOL(ftrace_ops_list_func);
> >
> >  /*
> >   * If there's only one function registered but it does not support
> > --
> > 2.20.1
>
Steven Rostedt July 24, 2020, 4:35 p.m. UTC | #3
On Fri, 24 Jul 2020 18:19:21 +0200
Oscar Carter <oscar.carter@gmx.com> wrote:

> > The linker trick is far less intrusive, and I believe less error prone.  
> 
> If we use the linker trick, the warning -Wcast-function-type dissapears,
> but in a way that makes impossible to the compiler to get the necessary
> info about function prototypes to insert the commented check. As far I
> know, this linker trick (redirection of a function) is hidden for the
> CFI build.
> 
> So, in my opinion, the linker trick is not suitable if we want to protect
> the function pointers of the ftrace subsystem against an attack that
> modifiy the normal flow of the kernel.

The linker trick should only affect architectures that don't implement
the needed features. I can make it so the linker trick is only applied
to those archs, and other archs that want more protection only need to
add these features to their architectures.

It's much less intrusive than this patch.

-- Steve
Oscar Carter July 24, 2020, 5:14 p.m. UTC | #4
On Fri, Jul 24, 2020 at 12:35:28PM -0400, Steven Rostedt wrote:
> On Fri, 24 Jul 2020 18:19:21 +0200
> Oscar Carter <oscar.carter@gmx.com> wrote:
>
> > > The linker trick is far less intrusive, and I believe less error prone.
> >
> > If we use the linker trick, the warning -Wcast-function-type dissapears,
> > but in a way that makes impossible to the compiler to get the necessary
> > info about function prototypes to insert the commented check. As far I
> > know, this linker trick (redirection of a function) is hidden for the
> > CFI build.
> >
> > So, in my opinion, the linker trick is not suitable if we want to protect
> > the function pointers of the ftrace subsystem against an attack that
> > modifiy the normal flow of the kernel.
>
> The linker trick should only affect architectures that don't implement
> the needed features. I can make it so the linker trick is only applied
> to those archs, and other archs that want more protection only need to
> add these features to their architectures.
>
> It's much less intrusive than this patch.

Sorry, but I don't understand your proposal. What features an arch need to
add if want the CFI protection?

>
> -- Steve

Thanks,
Oscar Carter
Oscar Carter July 24, 2020, 5:24 p.m. UTC | #5
Hi Steven,

On Fri, Jul 24, 2020 at 07:14:18PM +0200, Oscar Carter wrote:
> On Fri, Jul 24, 2020 at 12:35:28PM -0400, Steven Rostedt wrote:
> > On Fri, 24 Jul 2020 18:19:21 +0200
> > Oscar Carter <oscar.carter@gmx.com> wrote:
> >
> > > > The linker trick is far less intrusive, and I believe less error prone.
> > >
> > > If we use the linker trick, the warning -Wcast-function-type dissapears,
> > > but in a way that makes impossible to the compiler to get the necessary
> > > info about function prototypes to insert the commented check. As far I
> > > know, this linker trick (redirection of a function) is hidden for the
> > > CFI build.
> > >
> > > So, in my opinion, the linker trick is not suitable if we want to protect
> > > the function pointers of the ftrace subsystem against an attack that
> > > modifiy the normal flow of the kernel.
> >
> > The linker trick should only affect architectures that don't implement
> > the needed features. I can make it so the linker trick is only applied
> > to those archs, and other archs that want more protection only need to
> > add these features to their architectures.
> >
> > It's much less intrusive than this patch.
>
> Sorry, but I don't understand your proposal. What features an arch need to
> add if want the CFI protection?

Typo correction.

Sorry, but I don't understand your proposal. What features does an arch need
to add if want the CFI protection?

>
> >
> > -- Steve
>
Thanks,
Oscar Carter
Steven Rostedt July 24, 2020, 5:36 p.m. UTC | #6
On Fri, 24 Jul 2020 19:14:18 +0200
Oscar Carter <oscar.carter@gmx.com> wrote:

> > The linker trick should only affect architectures that don't implement
> > the needed features. I can make it so the linker trick is only applied
> > to those archs, and other archs that want more protection only need to
> > add these features to their architectures.
> 
> > It's much less intrusive than this patch.  
> 
> Sorry, but I don't understand your proposal. What features an arch need to
> add if want the CFI protection?

The better question is, what features should an arch add to not need
the linker trick ;-)

That is, they need to change it so that they add the two parameters
that is expected by the ftrace core code. Once they do that, then they
don't need to use the linker trick, and no function typecast is needed.

In other-words, if they support the ftrace_ops and regs passing, they
can define ARCH_SUPPORTS_FTRACE_OPS. Note, they don't even really need
to support the regs, (can just send NULL), if they don't have
HAVE_DYNAMIC_FTRACE_WITH_REGS.

Which BTW, is supported by the following architectures:

  arm
  arm64
  csky
  parisc
  powerpc
  riscv
  s390
  x86

All of the above architectures should not even be hitting the code that
does the function cast. What architecture are you doing all this for?

-- Steve
Steven Rostedt July 24, 2020, 5:40 p.m. UTC | #7
On Fri, 24 Jul 2020 13:36:56 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Which BTW, is supported by the following architectures:
> 
>   arm
>   arm64
>   csky
>   parisc
>   powerpc
>   riscv
>   s390
>   x86

And here's a list of architectures that have function tracing but need
to be updated:

  ia64
  microblaze
  mips
  nds32
  sh
  sparc
  xtensa

> 
> All of the above architectures should not even be hitting the code
> that does the function cast. What architecture are you doing all this
> for?

Which one of the above is this patch set for?

-- Steve
Steven Rostedt July 24, 2020, 5:48 p.m. UTC | #8
On Fri, 24 Jul 2020 13:40:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 24 Jul 2020 13:36:56 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > Which BTW, is supported by the following architectures:


> >   x86  

Ah, you can lose support on x86 if you don't enable DYNAMIC_FTRACE,
which is stupid to do. I only enabled disabling of DYNAMIC_FTRACE on
x86 to test it, as not all architectures have it, and I currently only
test on x86.

Without DYNAMIC_FTRACE enabled, you *always* call into the ftrace
infrastructure for *every* function. This adds something like 15 to 20%
overhead to the kernel. Did I say it was stupid to do so?

If you are going through all this work because some randconfig causes
this warning because it enables CONFIG_FUNCTION_TRACER but without
DYNAMIC_FTRACE enabled, then I strongly suggest you start spending your
time elsewhere, because it will be a big NAK on my part to add all this
intrusive code for a config used only for debugging the non
DYNAMIC_FTRACE case.

-- Steve
Oscar Carter July 24, 2020, 5:55 p.m. UTC | #9
On Fri, Jul 24, 2020 at 01:40:20PM -0400, Steven Rostedt wrote:
> On Fri, 24 Jul 2020 13:36:56 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > Which BTW, is supported by the following architectures:
> >
> >   arm
> >   arm64
> >   csky
> >   parisc
> >   powerpc
> >   riscv
> >   s390
> >   x86
>
> And here's a list of architectures that have function tracing but need
> to be updated:
>
>   ia64
>   microblaze
>   mips
>   nds32
>   sh
>   sparc
>   xtensa
>
> >
> > All of the above architectures should not even be hitting the code
> > that does the function cast. What architecture are you doing all this
> > for?
>
> Which one of the above is this patch set for?

This patch is the result of a warning obtained with the following:

make allmodconfig ARCH=powerpc
make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -j4

And with the -Wcast-function-type enabled in the top level makefile.

>
> -- Steve

Thanks,
Oscar Carter
Steven Rostedt July 24, 2020, 6:34 p.m. UTC | #10
On Fri, 24 Jul 2020 19:55:00 +0200
Oscar Carter <oscar.carter@gmx.com> wrote:

> > Which one of the above is this patch set for?  
> 
> This patch is the result of a warning obtained with the following:
> 
> make allmodconfig ARCH=powerpc
> make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -j4
> 
> And with the -Wcast-function-type enabled in the top level makefile.

Looking into powerpc I found this:

arch/powerpc/include/asm/ftrace.h:

  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
  #define ARCH_SUPPORTS_FTRACE_OPS 1
  #endif


arch/powerpc/Kconfig:

	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL
[..]

  config MPROFILE_KERNEL
	depends on PPC64 && CPU_LITTLE_ENDIAN && FUNCTION_TRACER
	def_bool $(success,$(srctree)/arch/powerpc/tools/gcc-check-mprofile-kernel.sh $(CC) -I$(srctree)/include -D__KERNEL__)

So, it looks like you need to be 64bit PowerPC, Little Endian, and gcc
needs to support -mprofile.

Otherwise, it falls back to the old way that does the type casting.

If you are really concerned about this, I would recommend adding
support to the architecture you care about, and then this will no
longer be an issue.

The funny part is, you can still add support for ftrace_ops, without
adding support for DYNAMIC_FTRACE_WITH_REGS, if you only care about not
having to do that typecast.

My NAK still stands. I wont let an intrusive patch be added to the
ftrace core code to deal with an unsupported feature in an architecture.

I would be will to add that linker trick to remove the warning. Or we
just use that warning as incentive to get architecture developers to
implement this feature ;-)

-- Steve
Oscar Carter July 25, 2020, 3:09 p.m. UTC | #11
Hi Steven,

On Fri, Jul 24, 2020 at 02:34:57PM -0400, Steven Rostedt wrote:
> On Fri, 24 Jul 2020 19:55:00 +0200
> Oscar Carter <oscar.carter@gmx.com> wrote:
>
> > > Which one of the above is this patch set for?
> >
> > This patch is the result of a warning obtained with the following:
> >
> > make allmodconfig ARCH=powerpc
> > make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -j4
> >
> > And with the -Wcast-function-type enabled in the top level makefile.
>
> Looking into powerpc I found this:
>
> arch/powerpc/include/asm/ftrace.h:
>
>   #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>   #define ARCH_SUPPORTS_FTRACE_OPS 1
>   #endif
>
>
> arch/powerpc/Kconfig:
>
> 	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL
> [..]
>
>   config MPROFILE_KERNEL
> 	depends on PPC64 && CPU_LITTLE_ENDIAN && FUNCTION_TRACER
> 	def_bool $(success,$(srctree)/arch/powerpc/tools/gcc-check-mprofile-kernel.sh $(CC) -I$(srctree)/include -D__KERNEL__)
>
> So, it looks like you need to be 64bit PowerPC, Little Endian, and gcc
> needs to support -mprofile.
>
> Otherwise, it falls back to the old way that does the type casting.
>
> If you are really concerned about this, I would recommend adding
> support to the architecture you care about, and then this will no
> longer be an issue.
>
> The funny part is, you can still add support for ftrace_ops, without
> adding support for DYNAMIC_FTRACE_WITH_REGS, if you only care about not
> having to do that typecast.

I agree with you. I will try to add the support for ftrace_ops without
adding support for DYNAMIC_FTRACE_WITH_REGS to the powerpc architecture.

It's a great solution to allow a clean CFI build and so, protect this
arch (powerpc) against attacks that try to modify the normal control
flow.

I take a look at the kernel documentation about port ftrace to other
architectures [1] but it is out of date.

If I try to do this I will need some help. Some info that point me to the
right direction would be greatly appreciated. Some advice about what
functions I will need to implement would be really helpfull. Or point me
to the right piece of code that I can pick as base point.

> My NAK still stands. I wont let an intrusive patch be added to the
> ftrace core code to deal with an unsupported feature in an architecture.

Don't worry. I agree with you and thanks for finding a better way to
accomplish the initial purpose.

> I would be will to add that linker trick to remove the warning. Or we
> just use that warning as incentive to get architecture developers to
> implement this feature ;-)

In my opinion it would be better to leave the code as it an make the warning
visible until this feature has been added.

> -- Steve

[1] https://www.kernel.org/doc/html/latest/trace/ftrace-design.html

Thanks again,
Oscar Carter
Oscar Carter July 25, 2020, 3:19 p.m. UTC | #12
Sorry, typo correction for the last paragraph:

On Sat, Jul 25, 2020 at 05:09:14PM +0200, Oscar Carter wrote:
> > I would be will to add that linker trick to remove the warning. Or we
> > just use that warning as incentive to get architecture developers to
> > implement this feature ;-)
>
> In my opinion it would be better to leave the code as it an make the warning
> visible until this feature has been added.

In my opinion it would be better to leave the code as is and make the warning
visible until this feature has been added.

Thanks again,
Oscar Carter
Oscar Carter July 26, 2020, 3:52 p.m. UTC | #13
Hi Steven,

On Sat, Jul 25, 2020 at 05:09:14PM +0200, Oscar Carter wrote:
> Hi Steven,
>
> On Fri, Jul 24, 2020 at 02:34:57PM -0400, Steven Rostedt wrote:
> > On Fri, 24 Jul 2020 19:55:00 +0200
> > Oscar Carter <oscar.carter@gmx.com> wrote:
> >
> > > > Which one of the above is this patch set for?
> > >
> > > This patch is the result of a warning obtained with the following:
> > >
> > > make allmodconfig ARCH=powerpc
> > > make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -j4
> > >
> > > And with the -Wcast-function-type enabled in the top level makefile.
> >
> > Looking into powerpc I found this:
> >
> > arch/powerpc/include/asm/ftrace.h:
> >
> >   #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> >   #define ARCH_SUPPORTS_FTRACE_OPS 1
> >   #endif
> >
> >
> > arch/powerpc/Kconfig:
> >
> > 	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL
> > [..]
> >
> >   config MPROFILE_KERNEL
> > 	depends on PPC64 && CPU_LITTLE_ENDIAN && FUNCTION_TRACER
> > 	def_bool $(success,$(srctree)/arch/powerpc/tools/gcc-check-mprofile-kernel.sh $(CC) -I$(srctree)/include -D__KERNEL__)
> >
> > So, it looks like you need to be 64bit PowerPC, Little Endian, and gcc
> > needs to support -mprofile.
> >
> > Otherwise, it falls back to the old way that does the type casting.
> >
> > If you are really concerned about this, I would recommend adding
> > support to the architecture you care about, and then this will no
> > longer be an issue.
> >
> > The funny part is, you can still add support for ftrace_ops, without
> > adding support for DYNAMIC_FTRACE_WITH_REGS, if you only care about not
> > having to do that typecast.
>
> I agree with you. I will try to add the support for ftrace_ops without
> adding support for DYNAMIC_FTRACE_WITH_REGS to the powerpc architecture.
>
> It's a great solution to allow a clean CFI build and so, protect this
> arch (powerpc) against attacks that try to modify the normal control
> flow.
>
> I take a look at the kernel documentation about port ftrace to other
> architectures [1] but it is out of date.
>
> If I try to do this I will need some help. Some info that point me to the
> right direction would be greatly appreciated. Some advice about what
> functions I will need to implement would be really helpfull. Or point me
> to the right piece of code that I can pick as base point.

I've been searching and reading the code as much as possible. I've found
two patches that I think can be useful to guide me. One [1] adds support
for ftrace_ops to the riscv architecture. The other one [2] adds support
for ftrace_ops to the parisc architecture.

[1] commit 71e736a7d655 ("riscv/ftrace: Add ARCH_SUPPORTS_FTRACE_OPS support")
[2] commit d562aca37a54 ("parisc/ftrace: Add ARCH_SUPPORTS_FTRACE_OPS support")

Due to powerpc arch calls the needed functions from assembly, I based my
idea on the patch for the RISCV arch.

Can something like the following work?

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index bc76970b6ee5..1c51ff5afae1 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -61,9 +61,8 @@ struct dyn_arch_ftrace {
 };
 #endif /* __ASSEMBLY__ */

-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 #define ARCH_SUPPORTS_FTRACE_OPS 1
-#endif
+
 #endif /* CONFIG_FUNCTION_TRACER */

 #ifndef __ASSEMBLY__
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index e023ae59c429..e69a4e945986 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -29,6 +29,10 @@ _GLOBAL(ftrace_caller)
        MCOUNT_SAVE_FRAME
        /* r3 ends up with link register */
        subi    r3, r3, MCOUNT_INSN_SIZE
+
+       /* Set ftrace_ops (r5) to the global variable function_trace_op */
+       /* Set pt_regs (r6) to NULL */
+
 .globl ftrace_call
 ftrace_call:
        bl      ftrace_stub
diff --git a/arch/powerpc/kernel/trace/ftrace_64_pg.S b/arch/powerpc/kernel/trace/ftrace_64_pg.S
index 6708e24db0ab..a741448b1df9 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_pg.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_pg.S
@@ -22,6 +22,10 @@ _GLOBAL_TOC(ftrace_caller)
        std     r3, 128(r1)
        ld      r4, 16(r11)
        subi    r3, r3, MCOUNT_INSN_SIZE
+
+       /* Set ftrace_ops (r5) to the global variable function_trace_op */
+       /* Set pt_regs (r6) to NULL */
+
 .globl ftrace_call
 ftrace_call:
        bl      ftrace_stub

To add support for ftrace_ops to the powerpc architecture is only necessary
to fill the r5 and r6 registers before the call to ftrace_stub in all the
cases. The register r5 is a pointer to ftrace_ops struct and the register
r6 is a pointer to pt_regs struct. These registers are the third and fourth
parameters of a function with the following prototype. The first and second
ones are yet set.

void func(unsigned long ip, unsigned long parent_ip,
	  struct ftrace_ops *ops, struct pt_regs *regs);

Am I in the right direction? or am I totally wrong?

Thanks for your time and patience.
Oscar Carter.

> > My NAK still stands. I wont let an intrusive patch be added to the
> > ftrace core code to deal with an unsupported feature in an architecture.
>
> Don't worry. I agree with you and thanks for finding a better way to
> accomplish the initial purpose.
>
> > I would be will to add that linker trick to remove the warning. Or we
> > just use that warning as incentive to get architecture developers to
> > implement this feature ;-)
>
> In my opinion it would be better to leave the code as it an make the warning
> visible until this feature has been added.
>
> > -- Steve
>
> [1] https://www.kernel.org/doc/html/latest/trace/ftrace-design.html
>
> Thanks again,
> Oscar Carter
Steven Rostedt July 27, 2020, 1:53 p.m. UTC | #14
On Sun, 26 Jul 2020 17:52:42 +0200
Oscar Carter <oscar.carter@gmx.com> wrote:

> > If I try to do this I will need some help. Some info that point me to the
> > right direction would be greatly appreciated. Some advice about what
> > functions I will need to implement would be really helpfull. Or point me
> > to the right piece of code that I can pick as base point.  
> 
> I've been searching and reading the code as much as possible. I've found
> two patches that I think can be useful to guide me. One [1] adds support
> for ftrace_ops to the riscv architecture. The other one [2] adds support
> for ftrace_ops to the parisc architecture.
> 
> [1] commit 71e736a7d655 ("riscv/ftrace: Add ARCH_SUPPORTS_FTRACE_OPS support")
> [2] commit d562aca37a54 ("parisc/ftrace: Add ARCH_SUPPORTS_FTRACE_OPS support")
> 
> Due to powerpc arch calls the needed functions from assembly, I based my
> idea on the patch for the RISCV arch.
> 
> Can something like the following work?
> 
> diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> index bc76970b6ee5..1c51ff5afae1 100644
> --- a/arch/powerpc/include/asm/ftrace.h
> +++ b/arch/powerpc/include/asm/ftrace.h
> @@ -61,9 +61,8 @@ struct dyn_arch_ftrace {
>  };
>  #endif /* __ASSEMBLY__ */
> 
> -#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>  #define ARCH_SUPPORTS_FTRACE_OPS 1
> -#endif
> +
>  #endif /* CONFIG_FUNCTION_TRACER */
> 
>  #ifndef __ASSEMBLY__
> diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
> index e023ae59c429..e69a4e945986 100644
> --- a/arch/powerpc/kernel/trace/ftrace_32.S
> +++ b/arch/powerpc/kernel/trace/ftrace_32.S
> @@ -29,6 +29,10 @@ _GLOBAL(ftrace_caller)
>         MCOUNT_SAVE_FRAME
>         /* r3 ends up with link register */
>         subi    r3, r3, MCOUNT_INSN_SIZE
> +
> +       /* Set ftrace_ops (r5) to the global variable function_trace_op */
> +       /* Set pt_regs (r6) to NULL */
> +
>  .globl ftrace_call
>  ftrace_call:
>         bl      ftrace_stub
> diff --git a/arch/powerpc/kernel/trace/ftrace_64_pg.S b/arch/powerpc/kernel/trace/ftrace_64_pg.S
> index 6708e24db0ab..a741448b1df9 100644
> --- a/arch/powerpc/kernel/trace/ftrace_64_pg.S
> +++ b/arch/powerpc/kernel/trace/ftrace_64_pg.S
> @@ -22,6 +22,10 @@ _GLOBAL_TOC(ftrace_caller)
>         std     r3, 128(r1)
>         ld      r4, 16(r11)
>         subi    r3, r3, MCOUNT_INSN_SIZE
> +
> +       /* Set ftrace_ops (r5) to the global variable function_trace_op */
> +       /* Set pt_regs (r6) to NULL */

I'm guessing you are going to do the above here. If so, this looks correct.

> +
>  .globl ftrace_call
>  ftrace_call:
>         bl      ftrace_stub
> 
> To add support for ftrace_ops to the powerpc architecture is only necessary
> to fill the r5 and r6 registers before the call to ftrace_stub in all the
> cases. The register r5 is a pointer to ftrace_ops struct and the register
> r6 is a pointer to pt_regs struct. These registers are the third and fourth
> parameters of a function with the following prototype. The first and second
> ones are yet set.

I guess you mean that the first and second ones are already set. But,
yeah, you are on the correct path here!

> 
> void func(unsigned long ip, unsigned long parent_ip,
> 	  struct ftrace_ops *ops, struct pt_regs *regs);
> 
> Am I in the right direction? or am I totally wrong?

No, you don't look wrong. But the true test is to try it out :-)

Don't forget to update ftrace_32.S as well.


> 
> Thanks for your time and patience.

My pleasure. Thanks for doing this. The more people that understand all
this, the better!

-- Steve
Oscar Carter July 31, 2020, 2:41 p.m. UTC | #15
Hi Steven,

On Mon, Jul 27, 2020 at 09:53:06AM -0400, Steven Rostedt wrote:
> On Sun, 26 Jul 2020 17:52:42 +0200
> Oscar Carter <oscar.carter@gmx.com> wrote:
>
> > > If I try to do this I will need some help. Some info that point me to the
> > > right direction would be greatly appreciated. Some advice about what
> > > functions I will need to implement would be really helpfull. Or point me
> > > to the right piece of code that I can pick as base point.
> >
> > I've been searching and reading the code as much as possible. I've found
> > two patches that I think can be useful to guide me. One [1] adds support
> > for ftrace_ops to the riscv architecture. The other one [2] adds support
> > for ftrace_ops to the parisc architecture.
> >
> > [1] commit 71e736a7d655 ("riscv/ftrace: Add ARCH_SUPPORTS_FTRACE_OPS support")
> > [2] commit d562aca37a54 ("parisc/ftrace: Add ARCH_SUPPORTS_FTRACE_OPS support")
> >
> > Due to powerpc arch calls the needed functions from assembly, I based my
> > idea on the patch for the RISCV arch.
> >
> > Can something like the following work?
> >
> > diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
> > index bc76970b6ee5..1c51ff5afae1 100644
> > --- a/arch/powerpc/include/asm/ftrace.h
> > +++ b/arch/powerpc/include/asm/ftrace.h
> > @@ -61,9 +61,8 @@ struct dyn_arch_ftrace {
> >  };
> >  #endif /* __ASSEMBLY__ */
> >
> > -#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> >  #define ARCH_SUPPORTS_FTRACE_OPS 1
> > -#endif
> > +
> >  #endif /* CONFIG_FUNCTION_TRACER */
> >
> >  #ifndef __ASSEMBLY__
> > diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
> > index e023ae59c429..e69a4e945986 100644
> > --- a/arch/powerpc/kernel/trace/ftrace_32.S
> > +++ b/arch/powerpc/kernel/trace/ftrace_32.S
> > @@ -29,6 +29,10 @@ _GLOBAL(ftrace_caller)
> >         MCOUNT_SAVE_FRAME
> >         /* r3 ends up with link register */
> >         subi    r3, r3, MCOUNT_INSN_SIZE
> > +
> > +       /* Set ftrace_ops (r5) to the global variable function_trace_op */
> > +       /* Set pt_regs (r6) to NULL */
> > +
> >  .globl ftrace_call
> >  ftrace_call:
> >         bl      ftrace_stub
> > diff --git a/arch/powerpc/kernel/trace/ftrace_64_pg.S b/arch/powerpc/kernel/trace/ftrace_64_pg.S
> > index 6708e24db0ab..a741448b1df9 100644
> > --- a/arch/powerpc/kernel/trace/ftrace_64_pg.S
> > +++ b/arch/powerpc/kernel/trace/ftrace_64_pg.S
> > @@ -22,6 +22,10 @@ _GLOBAL_TOC(ftrace_caller)
> >         std     r3, 128(r1)
> >         ld      r4, 16(r11)
> >         subi    r3, r3, MCOUNT_INSN_SIZE
> > +
> > +       /* Set ftrace_ops (r5) to the global variable function_trace_op */
> > +       /* Set pt_regs (r6) to NULL */
>
> I'm guessing you are going to do the above here. If so, this looks correct.
>
> > +
> >  .globl ftrace_call
> >  ftrace_call:
> >         bl      ftrace_stub
> >
> > To add support for ftrace_ops to the powerpc architecture is only necessary
> > to fill the r5 and r6 registers before the call to ftrace_stub in all the
> > cases. The register r5 is a pointer to ftrace_ops struct and the register
> > r6 is a pointer to pt_regs struct. These registers are the third and fourth
> > parameters of a function with the following prototype. The first and second
> > ones are yet set.
>
> I guess you mean that the first and second ones are already set. But,
> yeah, you are on the correct path here!
>
> >
> > void func(unsigned long ip, unsigned long parent_ip,
> > 	  struct ftrace_ops *ops, struct pt_regs *regs);
> >
> > Am I in the right direction? or am I totally wrong?
>
> No, you don't look wrong. But the true test is to try it out :-)

Of course. I will do the commented work, I will test it and then I will send
a new patch.

> Don't forget to update ftrace_32.S as well.
>
>
> >
> > Thanks for your time and patience.
>
> My pleasure. Thanks for doing this. The more people that understand all
> this, the better!

Thanks for your guidance and advices.

> -- Steve

Regards,
Oscar Carter
diff mbox series

Patch

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index fd8fbb422860..124ccf914657 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -143,9 +143,7 @@  static inline bool ftrace_same_address_ufunc(union ftrace_func *ufunc,
 	return (ufunc->ops == func);
 }
 #else
-/* See comment below, where ftrace_ops_list_func is defined */
-static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip);
-#define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
+static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip);

 static inline void ftrace_set_ufunc(union ftrace_func *ufunc,
 				    ftrace_func_no_ops_t func)
@@ -198,22 +196,29 @@  static void ftrace_sync_ipi(void *data)
 	smp_rmb();
 }

-static ftrace_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
+static union ftrace_func ftrace_ops_get_list_func(struct ftrace_ops *ops)
 {
+	union ftrace_func list_func;
+
 	/*
 	 * If this is a dynamic, RCU, or per CPU ops, or we force list func,
 	 * then it needs to call the list anyway.
 	 */
 	if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_RCU) ||
 	    FTRACE_FORCE_LIST_FUNC)
-		return ftrace_ops_list_func;
+		ftrace_set_ufunc(&list_func, ftrace_ops_list_func);
+	else
+		list_func.ops = ftrace_ops_get_func(ops);

-	return ftrace_ops_get_func(ops);
+	return list_func;
 }

 static void update_ftrace_function(void)
 {
-	ftrace_func_t func;
+	union ftrace_func func;
+#ifndef CONFIG_DYNAMIC_FTRACE
+	union ftrace_func tmp;
+#endif

 	/*
 	 * Prepare the ftrace_ops that the arch callback will use.
@@ -225,7 +230,7 @@  static void update_ftrace_function(void)

 	/* If there's no ftrace_ops registered, just call the stub function */
 	if (set_function_trace_op == &ftrace_list_end) {
-		func = ftrace_stub;
+		func.ops = ftrace_stub;

 	/*
 	 * If we are at the end of the list and this ops is
@@ -239,21 +244,21 @@  static void update_ftrace_function(void)
 	} else {
 		/* Just use the default ftrace_ops */
 		set_function_trace_op = &ftrace_list_end;
-		func = ftrace_ops_list_func;
+		ftrace_set_ufunc(&func, ftrace_ops_list_func);
 	}

 	update_function_graph_func();

 	/* If there's no change, then do nothing more here */
-	if (ftrace_trace_function == func)
+	if (ftrace_trace_function == func.ops)
 		return;

 	/*
 	 * If we are using the list function, it doesn't care
 	 * about the function_trace_ops.
 	 */
-	if (func == ftrace_ops_list_func) {
-		ftrace_trace_function = func;
+	if (ftrace_same_address_ufunc(&func, ftrace_ops_list_func)) {
+		ftrace_trace_function = func.ops;
 		/*
 		 * Don't even bother setting function_trace_ops,
 		 * it would be racy to do so anyway.
@@ -272,7 +277,9 @@  static void update_ftrace_function(void)
 	 * function we want, albeit indirectly, but it handles the
 	 * ftrace_ops and doesn't depend on function_trace_op.
 	 */
-	ftrace_trace_function = ftrace_ops_list_func;
+	ftrace_set_ufunc(&tmp, ftrace_ops_list_func);
+	ftrace_trace_function = tmp.ops;
+
 	/*
 	 * Make sure all CPUs see this. Yes this is slow, but static
 	 * tracing is slow and nasty to have enabled.
@@ -287,7 +294,7 @@  static void update_ftrace_function(void)
 	/* OK, we are all set to update the ftrace_trace_function now! */
 #endif /* !CONFIG_DYNAMIC_FTRACE */

-	ftrace_trace_function = func;
+	ftrace_trace_function = func.ops;
 }

 static void add_ftrace_ops(struct ftrace_ops __rcu **list,
@@ -2680,6 +2687,7 @@  void ftrace_modify_all_code(int command)
 	int update = command & FTRACE_UPDATE_TRACE_FUNC;
 	int mod_flags = 0;
 	int err = 0;
+	union ftrace_func func;

 	if (command & FTRACE_MAY_SLEEP)
 		mod_flags = FTRACE_MODIFY_MAY_SLEEP_FL;
@@ -2695,7 +2703,8 @@  void ftrace_modify_all_code(int command)
 	 * traced.
 	 */
 	if (update) {
-		err = ftrace_update_ftrace_func(ftrace_ops_list_func);
+		ftrace_set_ufunc(&func, ftrace_ops_list_func);
+		err = ftrace_update_ftrace_func(func.ops);
 		if (FTRACE_WARN_ON(err))
 			return;
 	}
@@ -2705,7 +2714,9 @@  void ftrace_modify_all_code(int command)
 	else if (command & FTRACE_DISABLE_CALLS)
 		ftrace_replace_code(mod_flags);

-	if (update && ftrace_trace_function != ftrace_ops_list_func) {
+	ftrace_set_ufunc(&func, ftrace_ops_list_func);
+
+	if (update && ftrace_trace_function != func.ops) {
 		function_trace_op = set_function_trace_op;
 		smp_wmb();
 		/* If irqs are disabled, we are in stop machine */
@@ -6890,14 +6901,13 @@  static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
 {
 	__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
 }
-NOKPROBE_SYMBOL(ftrace_ops_list_func);
 #else
-static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
+static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
 {
 	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
 }
-NOKPROBE_SYMBOL(ftrace_ops_no_ops);
 #endif
+NOKPROBE_SYMBOL(ftrace_ops_list_func);

 /*
  * If there's only one function registered but it does not support