* [PATCH] kernel/trace: Remove function callback casts @ 2020-06-14 7:01 Oscar Carter 2020-06-15 20:17 ` Steven Rostedt 0 siblings, 1 reply; 5+ messages in thread From: Oscar Carter @ 2020-06-14 7:01 UTC (permalink / raw) To: Kees Cook, Steven Rostedt, Ingo Molnar Cc: kernel-hardening, linux-kernel, Oscar Carter In an effort to enable -Wcast-function-type in the top-level Makefile to support Control Flow Integrity builds, remove all the function callback casts. To do this, use the ftrace_ops_list_func function as a wrapper when the arch not supports ftrace ops instead of the use of a function cast. Signed-off-by: Oscar Carter <oscar.carter@gmx.com> --- kernel/trace/ftrace.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index c163c3531faf..ed1efc0e3a25 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -119,13 +119,12 @@ 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 + +#if !ARCH_SUPPORTS_FTRACE_OPS /* 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 static inline void ftrace_ops_init(struct ftrace_ops *ops) @@ -6860,6 +6859,12 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, } NOKPROBE_SYMBOL(ftrace_ops_list_func); #else +static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, + struct ftrace_ops *op, struct pt_regs *regs) +{ + ftrace_ops_no_ops(ip, parent_ip); +} + static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip) { __ftrace_ops_list_func(ip, parent_ip, NULL, NULL); -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] kernel/trace: Remove function callback casts 2020-06-14 7:01 [PATCH] kernel/trace: Remove function callback casts Oscar Carter @ 2020-06-15 20:17 ` Steven Rostedt 2020-06-15 20:22 ` Steven Rostedt 0 siblings, 1 reply; 5+ messages in thread From: Steven Rostedt @ 2020-06-15 20:17 UTC (permalink / raw) To: Oscar Carter; +Cc: Kees Cook, Ingo Molnar, kernel-hardening, linux-kernel On Sun, 14 Jun 2020 09:01:54 +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, remove all the function callback > casts. > > To do this, use the ftrace_ops_list_func function as a wrapper when the > arch not supports ftrace ops instead of the use of a function cast. > We need more tricker than this. > Signed-off-by: Oscar Carter <oscar.carter@gmx.com> > --- > kernel/trace/ftrace.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index c163c3531faf..ed1efc0e3a25 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -119,13 +119,12 @@ 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 > + > +#if !ARCH_SUPPORTS_FTRACE_OPS > /* 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) The reason for the typecast is because this gets called from asm with only two parameters. > #endif > > static inline void ftrace_ops_init(struct ftrace_ops *ops) > @@ -6860,6 +6859,12 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, > } > NOKPROBE_SYMBOL(ftrace_ops_list_func); > #else > +static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, > + struct ftrace_ops *op, struct pt_regs *regs) > +{ > + ftrace_ops_no_ops(ip, parent_ip); > +} > + > static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip) > { > __ftrace_ops_list_func(ip, parent_ip, NULL, NULL); > -- > 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kernel/trace: Remove function callback casts 2020-06-15 20:17 ` Steven Rostedt @ 2020-06-15 20:22 ` Steven Rostedt 2020-06-17 20:28 ` Steven Rostedt 0 siblings, 1 reply; 5+ messages in thread From: Steven Rostedt @ 2020-06-15 20:22 UTC (permalink / raw) To: Oscar Carter; +Cc: Kees Cook, Ingo Molnar, kernel-hardening, linux-kernel On Mon, 15 Jun 2020 16:17:38 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Sun, 14 Jun 2020 09:01:54 +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, remove all the function callback > > casts. > > > > To do this, use the ftrace_ops_list_func function as a wrapper when the > > arch not supports ftrace ops instead of the use of a function cast. > > > > We need more tricker than this. > > > Signed-off-by: Oscar Carter <oscar.carter@gmx.com> > > --- > > kernel/trace/ftrace.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index c163c3531faf..ed1efc0e3a25 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -119,13 +119,12 @@ 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 > > + > > +#if !ARCH_SUPPORTS_FTRACE_OPS > > /* 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) > > The reason for the typecast is because this gets called from asm with only two parameters. [ Some how hit a short cut key that sent this before I was finished! :-p ] As I was saying. This typecast is being paranoid, as archs will call the ftrace_ops_list_func directly, and only pass in two parameters. Now one way around this is to instead of having the typecast, I could use linker magic to create another function that I can define without the typecast to get the same effect. Similar to what I did in commit: 46f9469247c6f ("ftrace: Rename ftrace_graph_stub to ftrace_stub_graph") -- Steve > > > #endif > > > > static inline void ftrace_ops_init(struct ftrace_ops *ops) > > @@ -6860,6 +6859,12 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, > > } > > NOKPROBE_SYMBOL(ftrace_ops_list_func); > > #else > > +static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip, > > + struct ftrace_ops *op, struct pt_regs *regs) > > +{ > > + ftrace_ops_no_ops(ip, parent_ip); > > +} > > + > > static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip) > > { > > __ftrace_ops_list_func(ip, parent_ip, NULL, NULL); > > -- > > 2.20.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] kernel/trace: Remove function callback casts 2020-06-15 20:22 ` Steven Rostedt @ 2020-06-17 20:28 ` Steven Rostedt 2020-06-20 14:23 ` Oscar Carter 0 siblings, 1 reply; 5+ messages in thread From: Steven Rostedt @ 2020-06-17 20:28 UTC (permalink / raw) To: Oscar Carter; +Cc: Kees Cook, Ingo Molnar, kernel-hardening, linux-kernel On Mon, 15 Jun 2020 16:22:45 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > As I was saying. This typecast is being paranoid, as archs will call > the ftrace_ops_list_func directly, and only pass in two parameters. > > Now one way around this is to instead of having the typecast, I could > use linker magic to create another function that I can define without > the typecast to get the same effect. Similar to what I did in commit: > > 46f9469247c6f ("ftrace: Rename ftrace_graph_stub to ftrace_stub_graph") Would something like this work for you? -- Steve 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] kernel/trace: Remove function callback casts 2020-06-17 20:28 ` Steven Rostedt @ 2020-06-20 14:23 ` Oscar Carter 0 siblings, 0 replies; 5+ messages in thread From: Oscar Carter @ 2020-06-20 14:23 UTC (permalink / raw) To: Steven Rostedt Cc: Oscar Carter, Kees Cook, Ingo Molnar, kernel-hardening, linux-kernel On Wed, Jun 17, 2020 at 04:28:00PM -0400, Steven Rostedt wrote: > On Mon, 15 Jun 2020 16:22:45 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > As I was saying. This typecast is being paranoid, as archs will call > > the ftrace_ops_list_func directly, and only pass in two parameters. > > > > Now one way around this is to instead of having the typecast, I could > > use linker magic to create another function that I can define without > > the typecast to get the same effect. Similar to what I did in commit: > > > > 46f9469247c6f ("ftrace: Rename ftrace_graph_stub to ftrace_stub_graph") > > Would something like this work for you? > > -- Steve > > 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 Thanks for your work and the proper patch sent [1]. I will follow the thread to see if I can help. [1] https://lore.kernel.org/lkml/20200617165616.52241bde@oasis.local.home/ Regards, Oscar Carter ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-06-20 14:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-14 7:01 [PATCH] kernel/trace: Remove function callback casts Oscar Carter 2020-06-15 20:17 ` Steven Rostedt 2020-06-15 20:22 ` Steven Rostedt 2020-06-17 20:28 ` Steven Rostedt 2020-06-20 14:23 ` Oscar Carter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).