diff mbox series

tracing: Use linker magic instead of recasting ftrace_ops_list_func()

Message ID 20200617165616.52241bde@oasis.local.home (mailing list archive)
State New, archived
Headers show
Series tracing: Use linker magic instead of recasting ftrace_ops_list_func() | expand

Commit Message

Steven Rostedt June 17, 2020, 8:56 p.m. UTC 
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

In an effort to enable -Wcast-function-type in the top-level Makefile to
support Control Flow Integrity builds, all function casts need to be
removed.

This means that 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
ftrace_ops_list_func() has four parameters.

Instead of a typecast, use vmlinux.lds.h to define ftrace_ops_list_func() to
arch_ftrace_ops_list_func() that will define the proper set of parameters.

Link: https://lore.kernel.org/r/20200614070154.6039-1-oscar.carter@gmx.com

Requested-by: Oscar Carter <oscar.carter@gmx.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/asm-generic/vmlinux.lds.h |  7 ++++++-
 kernel/trace/ftrace.c             | 23 ++++++++++-------------
 2 files changed, 16 insertions(+), 14 deletions(-)

Comments

Jann Horn June 17, 2020, 9:30 p.m. UTC | #1 
On Wed, Jun 17, 2020 at 10:56 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> In an effort to enable -Wcast-function-type in the top-level Makefile to
> support Control Flow Integrity builds, all function casts need to be
> removed.
>
> This means that 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
> ftrace_ops_list_func() has four parameters.
>
> Instead of a typecast, use vmlinux.lds.h to define ftrace_ops_list_func() to
> arch_ftrace_ops_list_func() that will define the proper set of parameters.
>
> Link: https://lore.kernel.org/r/20200614070154.6039-1-oscar.carter@gmx.com
[...]
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
[...]
> +                       ftrace_ops_list_func = arch_ftrace_ops_list_func;
>  #else
>  # ifdef CONFIG_FUNCTION_TRACER
>  #  define MCOUNT_REC() ftrace_stub_graph = ftrace_stub;
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
[...]
> +/* Defined by vmlinux.lds.h see the commment above arch_ftrace_ops_list_func for details */
> +void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> +                         struct ftrace_ops *op, struct pt_regs *regs);
[...]
> +void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
>  {

Well, it's not like the function cast itself is the part that's
problematic for CFI; the problematic part is when you actually make a
C function call (in particular an indirect one) where the destination
is compiled with a prototype that is different from the prototype used
at the call site. Doing this linker hackery isn't really any better
than shutting up the compiler warning by piling on enough casts or
whatever. (There should be some combination of casts that'll shut up
this warning, right?)

IIUC the real issue here is that ftrace_func_t is defined as a fixed
type, but actually has different types depending on the architecture?
If so, it might be cleaner to define ftrace_func_t differently
depending on architecture, or something like that?

And if that's not feasible, I think it would be better to at least
replace this linker trickery with straightforward
shut-up-the-compiler-casts - it'd be much easier to understand what's
actually going on that way.
Steven Rostedt June 17, 2020, 10:36 p.m. UTC | #2 
On Wed, 17 Jun 2020 23:30:07 +0200
Jann Horn <jannh@google.com> wrote:
> [...]
> > +/* Defined by vmlinux.lds.h see the commment above arch_ftrace_ops_list_func for details */
> > +void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> > +                         struct ftrace_ops *op, struct pt_regs *regs);  
> [...]
> > +void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
> >  {  
> 
> Well, it's not like the function cast itself is the part that's
> problematic for CFI; the problematic part is when you actually make a
> C function call (in particular an indirect one) where the destination
> is compiled with a prototype that is different from the prototype used
> at the call site. Doing this linker hackery isn't really any better
> than shutting up the compiler warning by piling on enough casts or
> whatever. (There should be some combination of casts that'll shut up
> this warning, right?)

It's not called by C, it's called by assembly.

> 
> IIUC the real issue here is that ftrace_func_t is defined as a fixed
> type, but actually has different types depending on the architecture?
> If so, it might be cleaner to define ftrace_func_t differently
> depending on architecture, or something like that?

There's functions that use this type.

When you register a function to be used by the function tracer (that
will have 4 parameters). If the arch supports it, it will call it
directly from the trampoline in assembly, but if it does not, then the
C code will only let assembly call the two parameter version, that will
call the 4 parameter function (adding NULLs to the extra two arguments). 

> 
> And if that's not feasible, I think it would be better to at least
> replace this linker trickery with straightforward
> shut-up-the-compiler-casts - it'd be much easier to understand what's
> actually going on that way.

OK, what's the way to shut up the compiler for it, and we can have that
instead.

-- Steve
Jann Horn June 17, 2020, 11:12 p.m. UTC | #3 
On Thu, Jun 18, 2020 at 12:36 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 17 Jun 2020 23:30:07 +0200
> Jann Horn <jannh@google.com> wrote:
> > [...]
> > > +/* Defined by vmlinux.lds.h see the commment above arch_ftrace_ops_list_func for details */
> > > +void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> > > +                         struct ftrace_ops *op, struct pt_regs *regs);
> > [...]
> > > +void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
> > >  {
> >
> > Well, it's not like the function cast itself is the part that's
> > problematic for CFI; the problematic part is when you actually make a
> > C function call (in particular an indirect one) where the destination
> > is compiled with a prototype that is different from the prototype used
> > at the call site. Doing this linker hackery isn't really any better
> > than shutting up the compiler warning by piling on enough casts or
> > whatever. (There should be some combination of casts that'll shut up
> > this warning, right?)
>
> It's not called by C, it's called by assembly.

Except on nds32 and parisc, right?

> > IIUC the real issue here is that ftrace_func_t is defined as a fixed
> > type, but actually has different types depending on the architecture?
> > If so, it might be cleaner to define ftrace_func_t differently
> > depending on architecture, or something like that?
>
> There's functions that use this type.
>
> When you register a function to be used by the function tracer (that
> will have 4 parameters). If the arch supports it, it will call it
> directly from the trampoline in assembly, but if it does not, then the
> C code will only let assembly call the two parameter version, that will
> call the 4 parameter function (adding NULLs to the extra two arguments).

So essentially there are two types, right? One type for the
registration API, one type for the assembly API? On some
architectures, the types are the same (and assembly calls directly
into the function); on other architectures, the types are not the
same, and assembly calls the NULL-adding helper (with the
assembly-API-type), and then the helper calls the function with the
registration-API-type?

Would it not be possible to have two function types (#define'd as the
same if ARCH_SUPPORTS_FTRACE_OPS), and then ensure that ftrace_func_t
is only used as ftrace_asm_func_t if ARCH_SUPPORTS_FTRACE_OPS?
Something like this (entirely untested)?

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index e339dac91ee62..b08fa393c1fad 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -89,6 +89,11 @@ struct ftrace_ops;

 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
                              struct ftrace_ops *op, struct pt_regs *regs);
+#if ARCH_SUPPORTS_FTRACE_OPS
+#define ftrace_asm_func_t ftrace_func_t
+#else
+typedef void (*ftrace_asm_func_t)(unsigned long ip, unsigned long parent_ip);
+#endif

 ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);

@@ -254,8 +259,12 @@ extern enum ftrace_tracing_type_t ftrace_tracing_type;
 int register_ftrace_function(struct ftrace_ops *ops);
 int unregister_ftrace_function(struct ftrace_ops *ops);

+#if ARCH_SUPPORTS_FTRACE_OPS
 extern void ftrace_stub(unsigned long a0, unsigned long a1,
                        struct ftrace_ops *op, struct pt_regs *regs);
+#else
+extern void ftrace_stub(unsigned long a0, unsigned long a1);
+#endif

 #else /* !CONFIG_FUNCTION_TRACER */
 /*
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index c163c3531fafc..abd076cdd84d9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -116,7 +116,7 @@ static int ftrace_disabled __read_mostly;
 DEFINE_MUTEX(ftrace_lock);

 struct ftrace_ops __rcu *ftrace_ops_list __read_mostly = &ftrace_list_end;
-ftrace_func_t ftrace_trace_function __read_mostly = ftrace_stub;
+ftrace_asm_func_t ftrace_trace_function __read_mostly = ftrace_stub;
 struct ftrace_ops global_ops;

 #if ARCH_SUPPORTS_FTRACE_OPS
@@ -125,7 +125,7 @@ static void ftrace_ops_list_func(unsigned long ip,
unsigned long parent_ip,
 #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)
+#define ftrace_ops_list_func ftrace_ops_no_ops
 #endif

 static inline void ftrace_ops_init(struct ftrace_ops *ops)
@@ -166,22 +166,25 @@ static void ftrace_sync_ipi(void *data)
        smp_rmb();
 }

-static ftrace_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
+static ftrace_asm_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
 {
+#if FTRACE_FORCE_LIST_FUNC
+       return ftrace_ops_list_func;
+#else
        /*
         * 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)
+       if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_RCU))
                return ftrace_ops_list_func;

        return ftrace_ops_get_func(ops);
+#endif
 }

 static void update_ftrace_function(void)
 {
-       ftrace_func_t func;
+       ftrace_asm_func_t func;

        /*
         * Prepare the ftrace_ops that the arch callback will use.
kernel test robot June 18, 2020, 9:13 a.m. UTC | #4 
Hi Steven,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on tip/perf/core linus/master v5.8-rc1 next-20200618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Steven-Rostedt/tracing-Use-linker-magic-instead-of-recasting-ftrace_ops_list_func/20200618-045733
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a5dc8300df75e8b8384b4c82225f1e4a0b4d9b55
config: parisc-randconfig-r011-20200618 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

kernel/trace/ftrace.c:300:5: warning: no previous prototype for '__register_ftrace_function' [-Wmissing-prototypes]
300 | int __register_ftrace_function(struct ftrace_ops *ops)
|     ^~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c:343:5: warning: no previous prototype for '__unregister_ftrace_function' [-Wmissing-prototypes]
343 | int __unregister_ftrace_function(struct ftrace_ops *ops)
|     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c:582:5: warning: no previous prototype for 'ftrace_profile_pages_init' [-Wmissing-prototypes]
582 | int ftrace_profile_pages_init(struct ftrace_profile_stat *stat)
|     ^~~~~~~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c:3796:15: warning: no previous prototype for 'arch_ftrace_match_adjust' [-Wmissing-prototypes]
3796 | char * __weak arch_ftrace_match_adjust(char *str, const char *search)
|               ^~~~~~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c: In function 'process_mod_list':
kernel/trace/ftrace.c:4107:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
4107 |  int ret;
|      ^~~
kernel/trace/ftrace.c: In function 'ftrace_regex_release':
kernel/trace/ftrace.c:5512:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
5512 |  int ret;
|      ^~~
kernel/trace/ftrace.c: At top level:
>> kernel/trace/ftrace.c:6854:6: warning: no previous prototype for 'arch_ftrace_ops_list_func' [-Wmissing-prototypes]
6854 | void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
|      ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +/arch_ftrace_ops_list_func +6854 kernel/trace/ftrace.c

  6836	
  6837	/*
  6838	 * Some archs only support passing ip and parent_ip. Even though
  6839	 * the list function ignores the op parameter, we do not want any
  6840	 * C side effects, where a function is called without the caller
  6841	 * sending a third parameter.
  6842	 * Archs are to support both the regs and ftrace_ops at the same time.
  6843	 * If they support ftrace_ops, it is assumed they support regs.
  6844	 * If call backs want to use regs, they must either check for regs
  6845	 * being NULL, or CONFIG_DYNAMIC_FTRACE_WITH_REGS.
  6846	 * Note, CONFIG_DYNAMIC_FTRACE_WITH_REGS expects a full regs to be saved.
  6847	 * An architecture can pass partial regs with ftrace_ops and still
  6848	 * set the ARCH_SUPPORTS_FTRACE_OPS.
  6849	 *
  6850	 * In vmlinux.lds.h, ftrace_ops_list_func() is defined to be
  6851	 * arch_ftrace_ops_list_func.
  6852	 */
  6853	#if ARCH_SUPPORTS_FTRACE_OPS
> 6854	void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
  6855				       struct ftrace_ops *op, struct pt_regs *regs)
  6856	{
  6857		__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
  6858	}
  6859	#else
  6860	void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
  6861	{
  6862		__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
  6863	}
  6864	#endif
  6865	NOKPROBE_SYMBOL(arch_ftrace_ops_list_func);
  6866	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot June 18, 2020, 10:06 a.m. UTC | #5 
Hi Steven,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on tip/perf/core linus/master v5.8-rc1 next-20200618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Steven-Rostedt/tracing-Use-linker-magic-instead-of-recasting-ftrace_ops_list_func/20200618-045733
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git a5dc8300df75e8b8384b4c82225f1e4a0b4d9b55
config: x86_64-randconfig-a015-20200618 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 487ca07fcc75d52755c9fe2ee05bcb3b6eeeec44)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

kernel/trace/ftrace.c:300:5: warning: no previous prototype for function '__register_ftrace_function' [-Wmissing-prototypes]
int __register_ftrace_function(struct ftrace_ops *ops)
^
kernel/trace/ftrace.c:300:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __register_ftrace_function(struct ftrace_ops *ops)
^
static
kernel/trace/ftrace.c:343:5: warning: no previous prototype for function '__unregister_ftrace_function' [-Wmissing-prototypes]
int __unregister_ftrace_function(struct ftrace_ops *ops)
^
kernel/trace/ftrace.c:343:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __unregister_ftrace_function(struct ftrace_ops *ops)
^
static
kernel/trace/ftrace.c:582:5: warning: no previous prototype for function 'ftrace_profile_pages_init' [-Wmissing-prototypes]
int ftrace_profile_pages_init(struct ftrace_profile_stat *stat)
^
kernel/trace/ftrace.c:582:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int ftrace_profile_pages_init(struct ftrace_profile_stat *stat)
^
static
kernel/trace/ftrace.c:3796:15: warning: no previous prototype for function 'arch_ftrace_match_adjust' [-Wmissing-prototypes]
char * __weak arch_ftrace_match_adjust(char *str, const char *search)
^
kernel/trace/ftrace.c:3796:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
char * __weak arch_ftrace_match_adjust(char *str, const char *search)
^
static
>> kernel/trace/ftrace.c:6854:6: warning: no previous prototype for function 'arch_ftrace_ops_list_func' [-Wmissing-prototypes]
void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
^
kernel/trace/ftrace.c:6854:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
^
static
5 warnings generated.

vim +/arch_ftrace_ops_list_func +6854 kernel/trace/ftrace.c

  6836	
  6837	/*
  6838	 * Some archs only support passing ip and parent_ip. Even though
  6839	 * the list function ignores the op parameter, we do not want any
  6840	 * C side effects, where a function is called without the caller
  6841	 * sending a third parameter.
  6842	 * Archs are to support both the regs and ftrace_ops at the same time.
  6843	 * If they support ftrace_ops, it is assumed they support regs.
  6844	 * If call backs want to use regs, they must either check for regs
  6845	 * being NULL, or CONFIG_DYNAMIC_FTRACE_WITH_REGS.
  6846	 * Note, CONFIG_DYNAMIC_FTRACE_WITH_REGS expects a full regs to be saved.
  6847	 * An architecture can pass partial regs with ftrace_ops and still
  6848	 * set the ARCH_SUPPORTS_FTRACE_OPS.
  6849	 *
  6850	 * In vmlinux.lds.h, ftrace_ops_list_func() is defined to be
  6851	 * arch_ftrace_ops_list_func.
  6852	 */
  6853	#if ARCH_SUPPORTS_FTRACE_OPS
> 6854	void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
  6855				       struct ftrace_ops *op, struct pt_regs *regs)
  6856	{
  6857		__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
  6858	}
  6859	#else
  6860	void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
  6861	{
  6862		__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
  6863	}
  6864	#endif
  6865	NOKPROBE_SYMBOL(arch_ftrace_ops_list_func);
  6866	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Steven Rostedt June 18, 2020, 4:41 p.m. UTC | #6 
On Thu, 18 Jun 2020 01:12:37 +0200
Jann Horn <jannh@google.com> wrote:

> static ftrace_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
> +static ftrace_asm_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
>  {
> +#if FTRACE_FORCE_LIST_FUNC
> +       return ftrace_ops_list_func;
> +#else
>         /*
>          * 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)
> +       if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_RCU))
>                 return ftrace_ops_list_func;
> 
>         return ftrace_ops_get_func(ops);

But ftrace_ops_get_func() returns ftrace_func_t type, wont this complain?

-- Steve


> +#endif
>  }
Jann Horn June 18, 2020, 5:58 p.m. UTC | #7 
On Thu, Jun 18, 2020 at 6:42 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 18 Jun 2020 01:12:37 +0200
> Jann Horn <jannh@google.com> wrote:
>
> > static ftrace_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
> > +static ftrace_asm_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
> >  {
> > +#if FTRACE_FORCE_LIST_FUNC
> > +       return ftrace_ops_list_func;
> > +#else
> >         /*
> >          * 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)
> > +       if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_RCU))
> >                 return ftrace_ops_list_func;
> >
> >         return ftrace_ops_get_func(ops);
>
> But ftrace_ops_get_func() returns ftrace_func_t type, wont this complain?

No, because we only compile this case under FTRACE_FORCE_LIST_FUNC==0,
which means ARCH_SUPPORTS_FTRACE_OPS, which means the preprocessor
turns all occurrences of ftrace_asm_func_t into ftrace_func_t.

Essentially my idea here is to take the high-level rule "you can only
directly call ftrace_func_t-typed functions from assembly if
ARCH_SUPPORTS_FTRACE_OPS", and encode it in the type system. And then
the compiler won't complain as long as we make sure that we never cast
between the two types under ARCH_SUPPORTS_FTRACE_OPS==0.
diff mbox series

Patch

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index db600ef218d7..120babd9ba44 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -145,13 +145,18 @@ 
  * Need to also make ftrace_stub_graph point to ftrace_stub
  * so that the same stub location may have different protocols
  * and not mess up with C verifiers.
+ *
+ * ftrace_ops_list_func will be defined as arch_ftrace_ops_list_func
+ * as some archs will have a different prototype for that function
+ * but ftrace_ops_list_func() will have a single prototype.
  */
 #define MCOUNT_REC()	. = ALIGN(8);				\
 			__start_mcount_loc = .;			\
 			KEEP(*(__mcount_loc))			\
 			KEEP(*(__patchable_function_entries))	\
 			__stop_mcount_loc = .;			\
-			ftrace_stub_graph = ftrace_stub;
+			ftrace_stub_graph = ftrace_stub;	\
+			ftrace_ops_list_func = arch_ftrace_ops_list_func;
 #else
 # ifdef CONFIG_FUNCTION_TRACER
 #  define MCOUNT_REC()	ftrace_stub_graph = ftrace_stub;
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f060838e9cbb..b775d399026e 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -119,14 +119,9 @@  struct ftrace_ops __rcu *ftrace_ops_list __read_mostly = &ftrace_list_end;
 ftrace_func_t ftrace_trace_function __read_mostly = ftrace_stub;
 struct ftrace_ops global_ops;
 
-#if ARCH_SUPPORTS_FTRACE_OPS
-static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
-				 struct ftrace_ops *op, struct pt_regs *regs);
-#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)
-#endif
+/* Defined by vmlinux.lds.h see the commment above arch_ftrace_ops_list_func for details */
+void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
+			  struct ftrace_ops *op, struct pt_regs *regs);
 
 static inline void ftrace_ops_init(struct ftrace_ops *ops)
 {
@@ -6859,21 +6854,23 @@  __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
  * Note, CONFIG_DYNAMIC_FTRACE_WITH_REGS expects a full regs to be saved.
  * An architecture can pass partial regs with ftrace_ops and still
  * set the ARCH_SUPPORTS_FTRACE_OPS.
+ *
+ * In vmlinux.lds.h, ftrace_ops_list_func() is defined to be
+ * arch_ftrace_ops_list_func.
  */
 #if ARCH_SUPPORTS_FTRACE_OPS
-static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
-				 struct ftrace_ops *op, struct pt_regs *regs)
+void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
+			       struct ftrace_ops *op, struct pt_regs *regs)
 {
 	__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)
+void arch_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(arch_ftrace_ops_list_func);
 
 /*
  * If there's only one function registered but it does not support