linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
@ 2021-10-05  5:39 Gustavo A. R. Silva
  2021-10-05 15:17 ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2021-10-05  5:39 UTC (permalink / raw)
  To: Steven Rostedt, Ingo Molnar, Daniel Bristot de Oliveira
  Cc: linux-kernel, Gustavo A. R. Silva

In order to make sure new function cast mismatches are not introduced
in the kernel (to avoid tripping CFI checking), the kernel should be
globally built with -Wcast-function-type.

So, fix the following -Wcast-function-type warnings on powerpc64
(ppc64_defconfig):

kernel/trace/ftrace.c: In function 'ftrace_ops_get_list_func':
kernel/trace/ftrace.c:128:31: warning: cast between incompatible function types from 'void (*)(long unsigned int,  long unsigned int)' to 'void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)' [-Wcast-function-type]
  128 | #define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
      |                               ^
kernel/trace/ftrace.c:174:10: note: in expansion of macro 'ftrace_ops_list_func'
  174 |   return ftrace_ops_list_func;
      |          ^~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c: In function 'update_ftrace_function':
kernel/trace/ftrace.c:128:31: warning: cast between incompatible function types from 'void (*)(long unsigned int,  long unsigned int)' to 'void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)' [-Wcast-function-type]
  128 | #define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
      |                               ^
kernel/trace/ftrace.c:207:10: note: in expansion of macro 'ftrace_ops_list_func'
  207 |   func = ftrace_ops_list_func;
      |          ^~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c:128:31: warning: cast between incompatible function types from 'void (*)(long unsigned int,  long unsigned int)' to 'void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)' [-Wcast-function-type]
  128 | #define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
      |                               ^
kernel/trace/ftrace.c:220:14: note: in expansion of macro 'ftrace_ops_list_func'
  220 |  if (func == ftrace_ops_list_func) {
      |              ^~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c: In function 'ftrace_modify_all_code':
kernel/trace/ftrace.c:128:31: warning: cast between incompatible function types from 'void (*)(long unsigned int,  long unsigned int)' to 'void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)' [-Wcast-function-type]
  128 | #define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
      |                               ^
kernel/trace/ftrace.c:2698:35: note: in expansion of macro 'ftrace_ops_list_func'
 2698 |   err = ftrace_update_ftrace_func(ftrace_ops_list_func);
      |                                   ^~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c:128:31: warning: cast between incompatible function types from 'void (*)(long unsigned int,  long unsigned int)' to 'void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)' [-Wcast-function-type]

  128 | #define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
      |                               ^
kernel/trace/ftrace.c:2708:41: note: in expansion of macro 'ftrace_ops_list_func'
 2708 |  if (update && ftrace_trace_function != ftrace_ops_list_func) {

Link: https://github.com/KSPP/linux/issues/20
Link: https://lore.kernel.org/lkml/20210930095300.73be1555@canb.auug.org.au/
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---

Hi Steven,

I need your help here, please. In particular to review the following
pieces of code:

 @@ -142,6 +142,7 @@ static inline void ftrace_ops_init(struct ftrace_ops *ops)
 static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip,
                            struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 +#if ARCH_SUPPORTS_FTRACE_OPS
        struct trace_array *tr = op->private;
        int pid;
 
 @@ -155,6 +156,7 @@ static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip,
        }
 
        op->saved_func(ip, parent_ip, op, fregs);
 +#endif

 @@ -7006,7 +7008,11 @@ __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
                                pr_warn("op=%p %pS\n", op, op);
                                goto out;
                        }
 +#if ARCH_SUPPORTS_FTRACE_OPS
                        op->func(ip, parent_ip, op, fregs);
 +#else
 +                       op->func(ip, parent_ip);
 +#endif
                }
        } while_for_each_ftrace_op(op);
 out:

 @@ -7050,6 +7056,7 @@ NOKPROBE_SYMBOL(ftrace_ops_no_ops);
 static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip,
                                   struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 +#if ARCH_SUPPORTS_FTRACE_OPS
        int bit;
 
        bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_LIST_START, TRACE_LIST_MAX);
 @@ -7063,6 +7070,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip,
 
        preempt_enable_notrace();
        trace_clear_recursion(bit);
 +#endif
 }

Not sure if the above is the right solution when ARCH_SUPPORTS_FTRACE_OPS
is not supported. So, this is my first try to solve this issue.

JFYI: These are the last know -Wcast-function-type warnings. So, after
fixing this we will able to enable -Wcast-function-type, globally.

Thanks!

 include/linux/ftrace.h            |  7 +++++++
 kernel/trace/fgraph.c             |  2 +-
 kernel/trace/ftrace.c             | 34 +++++++++++++++++++------------
 kernel/trace/trace_event_perf.c   |  2 +-
 kernel/trace/trace_functions.c    | 10 ++++-----
 kernel/trace/trace_sched_wakeup.c |  2 +-
 kernel/trace/trace_stack.c        |  2 +-
 7 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 832e65f06754..30ff2f8c5107 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -114,8 +114,15 @@ static __always_inline struct pt_regs *ftrace_get_regs(struct ftrace_regs *fregs
 	return arch_ftrace_get_regs(fregs);
 }
 
+#if ARCH_SUPPORTS_FTRACE_OPS
 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
 			      struct ftrace_ops *op, struct ftrace_regs *fregs);
+#else
+typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip);
+#endif
+
+typedef void (*ftrace_func_base_t)(void);
+#define CAST_FTRACE_FUNC(f) ((ftrace_func_t)((ftrace_func_base_t)(f)))
 
 ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);
 
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index b8a0d1d564fb..874eff384ca8 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -333,7 +333,7 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
 #endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */
 
 static struct ftrace_ops graph_ops = {
-	.func			= ftrace_stub,
+	.func			= CAST_FTRACE_FUNC(ftrace_stub),
 	.flags			= FTRACE_OPS_FL_INITIALIZED |
 				   FTRACE_OPS_FL_PID |
 				   FTRACE_OPS_FL_STUB,
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f15badf31f52..7c9f11920773 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -79,7 +79,7 @@ enum {
 };
 
 struct ftrace_ops ftrace_list_end __read_mostly = {
-	.func		= ftrace_stub,
+	.func		= CAST_FTRACE_FUNC(ftrace_stub),
 	.flags		= FTRACE_OPS_FL_STUB,
 	INIT_OPS_HASH(ftrace_list_end)
 };
@@ -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_func_t ftrace_trace_function __read_mostly = CAST_FTRACE_FUNC(ftrace_stub);
 struct ftrace_ops global_ops;
 
 #if ARCH_SUPPORTS_FTRACE_OPS
@@ -142,6 +142,7 @@ static inline void ftrace_ops_init(struct ftrace_ops *ops)
 static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip,
 			    struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
+#if ARCH_SUPPORTS_FTRACE_OPS
 	struct trace_array *tr = op->private;
 	int pid;
 
@@ -155,6 +156,7 @@ static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip,
 	}
 
 	op->saved_func(ip, parent_ip, op, fregs);
+#endif
 }
 
 static void ftrace_sync_ipi(void *data)
@@ -190,7 +192,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 = CAST_FTRACE_FUNC(ftrace_stub);
 
 	/*
 	 * If we are at the end of the list and this ops is
@@ -332,7 +334,7 @@ int __register_ftrace_function(struct ftrace_ops *ops)
 	ops->saved_func = ops->func;
 
 	if (ftrace_pids_enabled(ops))
-		ops->func = ftrace_pid_func;
+		ops->func = CAST_FTRACE_FUNC(ftrace_pid_func);
 
 	ftrace_update_trampoline(ops);
 
@@ -367,13 +369,13 @@ static void ftrace_update_pid_func(void)
 	struct ftrace_ops *op;
 
 	/* Only do something if we are tracing something */
-	if (ftrace_trace_function == ftrace_stub)
+	if (ftrace_trace_function == CAST_FTRACE_FUNC(ftrace_stub))
 		return;
 
 	do_for_each_ftrace_op(op, ftrace_ops_list) {
 		if (op->flags & FTRACE_OPS_FL_PID) {
 			op->func = ftrace_pids_enabled(op) ?
-				ftrace_pid_func : op->saved_func;
+				CAST_FTRACE_FUNC(ftrace_pid_func) : op->saved_func;
 			ftrace_update_trampoline(op);
 		}
 	} while_for_each_ftrace_op(op);
@@ -1036,7 +1038,7 @@ static const struct ftrace_hash empty_hash = {
 #define EMPTY_HASH	((struct ftrace_hash *)&empty_hash)
 
 struct ftrace_ops global_ops = {
-	.func				= ftrace_stub,
+	.func				= CAST_FTRACE_FUNC(ftrace_stub),
 	.local_hash.notrace_hash	= EMPTY_HASH,
 	.local_hash.filter_hash		= EMPTY_HASH,
 	INIT_OPS_HASH(global_ops)
@@ -4545,7 +4547,7 @@ register_ftrace_function_probe(char *glob, struct trace_array *tr,
 			return -ENOMEM;
 		}
 		probe->probe_ops = probe_ops;
-		probe->ops.func = function_trace_probe_call;
+		probe->ops.func = CAST_FTRACE_FUNC(function_trace_probe_call);
 		probe->tr = tr;
 		ftrace_ops_init(&probe->ops);
 		list_add(&probe->list, &tr->func_probes);
@@ -6956,7 +6958,7 @@ void ftrace_init_array_ops(struct trace_array *tr, ftrace_func_t func)
 {
 	/* If we filter on pids, update to use the pid function */
 	if (tr->flags & TRACE_ARRAY_FL_GLOBAL) {
-		if (WARN_ON(tr->ops->func != ftrace_stub))
+		if (WARN_ON(tr->ops->func != CAST_FTRACE_FUNC(ftrace_stub)))
 			printk("ftrace ops had %pS for function\n",
 			       tr->ops->func);
 	}
@@ -6966,7 +6968,7 @@ void ftrace_init_array_ops(struct trace_array *tr, ftrace_func_t func)
 
 void ftrace_reset_array_ops(struct trace_array *tr)
 {
-	tr->ops->func = ftrace_stub;
+	tr->ops->func = CAST_FTRACE_FUNC(ftrace_stub);
 }
 
 static nokprobe_inline void
@@ -7006,7 +7008,11 @@ __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
 				pr_warn("op=%p %pS\n", op, op);
 				goto out;
 			}
+#if ARCH_SUPPORTS_FTRACE_OPS
 			op->func(ip, parent_ip, op, fregs);
+#else
+			op->func(ip, parent_ip);
+#endif
 		}
 	} while_for_each_ftrace_op(op);
 out:
@@ -7050,6 +7056,7 @@ NOKPROBE_SYMBOL(ftrace_ops_no_ops);
 static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip,
 				   struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
+#if ARCH_SUPPORTS_FTRACE_OPS
 	int bit;
 
 	bit = trace_test_and_set_recursion(ip, parent_ip, TRACE_LIST_START, TRACE_LIST_MAX);
@@ -7063,6 +7070,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip,
 
 	preempt_enable_notrace();
 	trace_clear_recursion(bit);
+#endif
 }
 NOKPROBE_SYMBOL(ftrace_ops_assist_func);
 
@@ -7085,7 +7093,7 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops)
 	 */
 	if (ops->flags & (FTRACE_OPS_FL_RECURSION |
 			  FTRACE_OPS_FL_RCU))
-		return ftrace_ops_assist_func;
+		return CAST_FTRACE_FUNC(ftrace_ops_assist_func);
 
 	return ops->func;
 }
@@ -7521,7 +7529,7 @@ void ftrace_kill(void)
 {
 	ftrace_disabled = 1;
 	ftrace_enabled = 0;
-	ftrace_trace_function = ftrace_stub;
+	ftrace_trace_function = CAST_FTRACE_FUNC(ftrace_stub);
 }
 
 /**
@@ -7622,7 +7630,7 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
 		}
 
 		/* stopping ftrace calls (just send to ftrace_stub) */
-		ftrace_trace_function = ftrace_stub;
+		ftrace_trace_function = CAST_FTRACE_FUNC(ftrace_stub);
 
 		ftrace_shutdown_sysctl();
 	}
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 6aed10e2f7ce..507c9516eb28 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -484,7 +484,7 @@ static int perf_ftrace_function_register(struct perf_event *event)
 {
 	struct ftrace_ops *ops = &event->ftrace_ops;
 
-	ops->func    = perf_ftrace_function_call;
+	ops->func    = CAST_FTRACE_FUNC(perf_ftrace_function_call);
 	ops->private = (void *)(unsigned long)nr_cpu_ids;
 
 	return register_ftrace_function(ops);
diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
index 1f0e63f5d1f9..9ef630f0d4d9 100644
--- a/kernel/trace/trace_functions.c
+++ b/kernel/trace/trace_functions.c
@@ -62,7 +62,7 @@ int ftrace_allocate_ftrace_ops(struct trace_array *tr)
 		return -ENOMEM;
 
 	/* Currently only the non stack version is supported */
-	ops->func = function_trace_call;
+	ops->func = CAST_FTRACE_FUNC(function_trace_call);
 	ops->flags = FTRACE_OPS_FL_PID;
 
 	tr->ops = ops;
@@ -105,13 +105,13 @@ static ftrace_func_t select_trace_function(u32 flags_val)
 {
 	switch (flags_val & TRACE_FUNC_OPT_MASK) {
 	case TRACE_FUNC_NO_OPTS:
-		return function_trace_call;
+		return CAST_FTRACE_FUNC(function_trace_call);
 	case TRACE_FUNC_OPT_STACK:
-		return function_stack_trace_call;
+		return CAST_FTRACE_FUNC(function_stack_trace_call);
 	case TRACE_FUNC_OPT_NO_REPEATS:
-		return function_no_repeats_trace_call;
+		return CAST_FTRACE_FUNC(function_no_repeats_trace_call);
 	case TRACE_FUNC_OPT_STACK | TRACE_FUNC_OPT_NO_REPEATS:
-		return function_stack_no_repeats_trace_call;
+		return CAST_FTRACE_FUNC(function_stack_no_repeats_trace_call);
 	default:
 		return NULL;
 	}
diff --git a/kernel/trace/trace_sched_wakeup.c b/kernel/trace/trace_sched_wakeup.c
index 2402de520eca..abcdf48888f7 100644
--- a/kernel/trace/trace_sched_wakeup.c
+++ b/kernel/trace/trace_sched_wakeup.c
@@ -673,7 +673,7 @@ static int __wakeup_tracer_init(struct trace_array *tr)
 
 	tr->max_latency = 0;
 	wakeup_trace = tr;
-	ftrace_init_array_ops(tr, wakeup_tracer_call);
+	ftrace_init_array_ops(tr, CAST_FTRACE_FUNC(wakeup_tracer_call));
 	start_wakeup_tracer(tr);
 
 	wakeup_busy = true;
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 63c285042051..546f769d7de2 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -317,7 +317,7 @@ stack_trace_call(unsigned long ip, unsigned long parent_ip,
 
 static struct ftrace_ops trace_ops __read_mostly =
 {
-	.func = stack_trace_call,
+	.func = CAST_FTRACE_FUNC(stack_trace_call),
 };
 
 static ssize_t
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-05  5:39 [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64 Gustavo A. R. Silva
@ 2021-10-05 15:17 ` Steven Rostedt
  2021-10-05 16:18   ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2021-10-05 15:17 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel

On Tue, 5 Oct 2021 00:39:22 -0500
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> In order to make sure new function cast mismatches are not introduced
> in the kernel (to avoid tripping CFI checking), the kernel should be
> globally built with -Wcast-function-type.
> 
> So, fix the following -Wcast-function-type warnings on powerpc64
> (ppc64_defconfig):

I think I'll go back and add my linker magic.

  https://lore.kernel.org/all/20200617165616.52241bde@oasis.local.home/

I'll clean it up a bit too. I'll have a patch in a bit.

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-05 15:17 ` Steven Rostedt
@ 2021-10-05 16:18   ` Gustavo A. R. Silva
  2021-10-05 16:35     ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2021-10-05 16:18 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel

On Tue, Oct 05, 2021 at 11:17:14AM -0400, Steven Rostedt wrote:
> On Tue, 5 Oct 2021 00:39:22 -0500
> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> 
> > In order to make sure new function cast mismatches are not introduced
> > in the kernel (to avoid tripping CFI checking), the kernel should be
> > globally built with -Wcast-function-type.
> > 
> > So, fix the following -Wcast-function-type warnings on powerpc64
> > (ppc64_defconfig):
> 
> I think I'll go back and add my linker magic.
> 
>   https://lore.kernel.org/all/20200617165616.52241bde@oasis.local.home/
> 
> I'll clean it up a bit too. I'll have a patch in a bit.

Awesome. :)

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-05 16:18   ` Gustavo A. R. Silva
@ 2021-10-05 16:35     ` Steven Rostedt
  2021-10-05 16:50       ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2021-10-05 16:35 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel

On Tue, 5 Oct 2021 11:18:12 -0500
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> On Tue, Oct 05, 2021 at 11:17:14AM -0400, Steven Rostedt wrote:
> > On Tue, 5 Oct 2021 00:39:22 -0500
> > "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> >   
> > > In order to make sure new function cast mismatches are not introduced
> > > in the kernel (to avoid tripping CFI checking), the kernel should be
> > > globally built with -Wcast-function-type.
> > > 
> > > So, fix the following -Wcast-function-type warnings on powerpc64
> > > (ppc64_defconfig):  
> > 
> > I think I'll go back and add my linker magic.
> > 
> >   https://lore.kernel.org/all/20200617165616.52241bde@oasis.local.home/
> > 
> > I'll clean it up a bit too. I'll have a patch in a bit.  
> 
> Awesome. :)
> 
> Thanks
> --

Does this fix it for you?

Subject: [PATCH] tracing: Use linker magic instead of recasting ftrace_ops_list_func()
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(-)

Index: linux-trace.git/include/asm-generic/vmlinux.lds.h
===================================================================
--- linux-trace.git.orig/include/asm-generic/vmlinux.lds.h
+++ linux-trace.git/include/asm-generic/vmlinux.lds.h
@@ -164,13 +164,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;
Index: linux-trace.git/kernel/trace/ftrace.c
===================================================================
--- linux-trace.git.orig/kernel/trace/ftrace.c
+++ linux-trace.git/kernel/trace/ftrace.c
@@ -119,14 +119,9 @@ struct ftrace_ops __rcu *ftrace_ops_list
 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 ftrace_regs *fregs);
-#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 ftrace_regs *fregs);
 
 static inline void ftrace_ops_init(struct ftrace_ops *ops)
 {
@@ -7026,21 +7021,23 @@ out:
  * 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 ftrace_regs *fregs)
+void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
+			       struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
 	__ftrace_ops_list_func(ip, parent_ip, NULL, fregs);
 }
-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	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-05 16:35     ` Steven Rostedt
@ 2021-10-05 16:50       ` Gustavo A. R. Silva
  2021-10-05 16:51         ` Steven Rostedt
  2021-10-05 19:08         ` Steven Rostedt
  0 siblings, 2 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2021-10-05 16:50 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel

On Tue, Oct 05, 2021 at 12:35:22PM -0400, Steven Rostedt wrote:
> On Tue, 5 Oct 2021 11:18:12 -0500
> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> 
> > On Tue, Oct 05, 2021 at 11:17:14AM -0400, Steven Rostedt wrote:
> > > On Tue, 5 Oct 2021 00:39:22 -0500
> > > "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> > >   
> > > > In order to make sure new function cast mismatches are not introduced
> > > > in the kernel (to avoid tripping CFI checking), the kernel should be
> > > > globally built with -Wcast-function-type.
> > > > 
> > > > So, fix the following -Wcast-function-type warnings on powerpc64
> > > > (ppc64_defconfig):  
> > > 
> > > I think I'll go back and add my linker magic.
> > > 
> > >   https://lore.kernel.org/all/20200617165616.52241bde@oasis.local.home/
> > > 
> > > I'll clean it up a bit too. I'll have a patch in a bit.  
> > 
> > Awesome. :)
> > 
> > Thanks
> > --
> 
> Does this fix it for you?

Nop; there are still some warnings (ppc64_defconfig):

kernel/trace/ftrace.c: In function ‘ftrace_ops_get_list_func’:
kernel/trace/ftrace.c:171:10: error: returning ‘void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)’ from a function with incompatible return type ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’} [-Werror=incompatible-pointer-types]
  171 |   return ftrace_ops_list_func;
      |          ^~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c: In function ‘update_ftrace_function’:
kernel/trace/ftrace.c:204:8: error: assignment to ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’} from incompatible pointer type ‘void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)’ [-Werror=incompatible-pointer-types]
  204 |   func = ftrace_ops_list_func;
      |        ^
kernel/trace/ftrace.c:217:11: warning: comparison of distinct pointer types lacks a cast
  217 |  if (func == ftrace_ops_list_func) {
      |           ^~
kernel/trace/ftrace.c: In function ‘ftrace_modify_all_code’:
kernel/trace/ftrace.c:2695:35: error: passing argument 1 of ‘ftrace_update_ftrace_func’ from incompatible pointer type [-Werror=incompatible-pointer-types]
 2695 |   err = ftrace_update_ftrace_func(ftrace_ops_list_func);
      |                                   ^~~~~~~~~~~~~~~~~~~~
      |                                   |
      |                                   void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)
In file included from kernel/trace/ftrace.c:29:
./include/linux/ftrace.h:585:52: note: expected ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’} but argument is of type ‘void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)’
  585 | extern int ftrace_update_ftrace_func(ftrace_func_t func);
      |                                      ~~~~~~~~~~~~~~^~~~
kernel/trace/ftrace.c:2705:38: warning: comparison of distinct pointer types lacks a cast
 2705 |  if (update && ftrace_trace_function != ftrace_ops_list_func) {
      |                                      ^~

--
Gustavo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-05 16:50       ` Gustavo A. R. Silva
@ 2021-10-05 16:51         ` Steven Rostedt
  2021-10-05 19:08         ` Steven Rostedt
  1 sibling, 0 replies; 16+ messages in thread
From: Steven Rostedt @ 2021-10-05 16:51 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel

On Tue, 5 Oct 2021 11:50:27 -0500
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> Nop; there are still some warnings (ppc64_defconfig):

Thanks, I'll take a look, and compile it against ppc64_defconfig.

-- Steve

> 
> kernel/trace/ftrace.c: In function ‘ftrace_ops_get_list_func’:
> kernel/trace/ftrace.c:171:10: error: returning ‘void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)’ from a function with incompatible return type ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’} [-Werror=incompatible-pointer-types]
>   171 |   return ftrace_ops_list_func;
>       |          ^~~~~~~~~~~~~~~~~~~~
> kernel/trace/ftrace.c: In function ‘update_ftrace_function’:
> kernel/trace/ftrace.c:204:8: error: assignment to ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’} from incompatible pointer type ‘void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)’ [-Werror=incompatible-pointer-types]
>   204 |   func = ftrace_ops_list_func;
>       |        ^
> kernel/trace/ftrace.c:217:11: warning: comparison of distinct pointer types lacks a cast
>   217 |  if (func == ftrace_ops_list_func) {
>       |           ^~
> kernel/trace/ftrace.c: In function ‘ftrace_modify_all_code’:
> kernel/trace/ftrace.c:2695:35: error: passing argument 1 of ‘ftrace_update_ftrace_func’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>  2695 |   err = ftrace_update_ftrace_func(ftrace_ops_list_func);
>       |                                   ^~~~~~~~~~~~~~~~~~~~
>       |                                   |
>       |                                   void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)
> In file included from kernel/trace/ftrace.c:29:
> ./include/linux/ftrace.h:585:52: note: expected ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’} but argument is of type ‘void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)’
>   585 | extern int ftrace_update_ftrace_func(ftrace_func_t func);
>       |                                      ~~~~~~~~~~~~~~^~~~
> kernel/trace/ftrace.c:2705:38: warning: comparison of distinct pointer types lacks a cast
>  2705 |  if (update && ftrace_trace_function != ftrace_ops_list_func) {
>       |                                      ^~

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-05 16:50       ` Gustavo A. R. Silva
  2021-10-05 16:51         ` Steven Rostedt
@ 2021-10-05 19:08         ` Steven Rostedt
  2021-10-05 19:35           ` Gustavo A. R. Silva
  1 sibling, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2021-10-05 19:08 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel

On Tue, 5 Oct 2021 11:50:27 -0500
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> On Tue, Oct 05, 2021 at 12:35:22PM -0400, Steven Rostedt wrote:
> > On Tue, 5 Oct 2021 11:18:12 -0500
> > "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> >   
> > > On Tue, Oct 05, 2021 at 11:17:14AM -0400, Steven Rostedt wrote:  
> > > > On Tue, 5 Oct 2021 00:39:22 -0500
> > > > "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> > > >     
> > > > > In order to make sure new function cast mismatches are not introduced
> > > > > in the kernel (to avoid tripping CFI checking), the kernel should be
> > > > > globally built with -Wcast-function-type.
> > > > > 
> > > > > So, fix the following -Wcast-function-type warnings on powerpc64
> > > > > (ppc64_defconfig):    
> > > > 
> > > > I think I'll go back and add my linker magic.
> > > > 
> > > >   https://lore.kernel.org/all/20200617165616.52241bde@oasis.local.home/
> > > > 
> > > > I'll clean it up a bit too. I'll have a patch in a bit.    
> > > 
> > > Awesome. :)
> > > 
> > > Thanks
> > > --  
> > 
> > Does this fix it for you?  
> 
> Nop; there are still some warnings (ppc64_defconfig):

Sure you applied it?

Because I tested it on powerpc64 with the defconfig, and without the patch,
I get the error, but with it, I don't.

> 
> kernel/trace/ftrace.c: In function ‘ftrace_ops_get_list_func’:
> kernel/trace/ftrace.c:171:10: error: returning ‘void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)’ from a function with incompatible return type ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’} [-Werror=incompatible-pointer-types]
>   171 |   return ftrace_ops_list_func;
>       |          ^~~~~~~~~~~~~~~~~~~~

Or did you not remove your patch first?

Because, the above error shows:

    return type ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’

But my code has:

  typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
			      struct ftrace_ops *op, struct ftrace_regs *fregs);


Which is not the same type, but your patch has:

+#if ARCH_SUPPORTS_FTRACE_OPS
 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
 			      struct ftrace_ops *op, struct ftrace_regs *fregs);
+#else
+typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip);
+#endif
+
+typedef void (*ftrace_func_base_t)(void);
+#define CAST_FTRACE_FUNC(f) ((ftrace_func_t)((ftrace_func_base_t)(f)))

Which redefines ftrace_func_t.

-- Steve


> kernel/trace/ftrace.c: In function ‘update_ftrace_function’:
> kernel/trace/ftrace.c:204:8: error: assignment to ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’} from incompatible pointer type ‘void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)’ [-Werror=incompatible-pointer-types]
>   204 |   func = ftrace_ops_list_func;
>       |        ^
> kernel/trace/ftrace.c:217:11: warning: comparison of distinct pointer types lacks a cast
>   217 |  if (func == ftrace_ops_list_func) {
>       |           ^~
> kernel/trace/ftrace.c: In function ‘ftrace_modify_all_code’:
> kernel/trace/ftrace.c:2695:35: error: passing argument 1 of ‘ftrace_update_ftrace_func’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>  2695 |   err = ftrace_update_ftrace_func(ftrace_ops_list_func);
>       |                                   ^~~~~~~~~~~~~~~~~~~~
>       |                                   |
>       |                                   void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)
> In file included from kernel/trace/ftrace.c:29:
> ./include/linux/ftrace.h:585:52: note: expected ‘ftrace_func_t’ {aka ‘void (*)(long unsigned int,  long unsigned int)’} but argument is of type ‘void (*)(long unsigned int,  long unsigned int,  struct ftrace_ops *, struct ftrace_regs *)’
>   585 | extern int ftrace_update_ftrace_func(ftrace_func_t func);
>       |                                      ~~~~~~~~~~~~~~^~~~
> kernel/trace/ftrace.c:2705:38: warning: comparison of distinct pointer types lacks a cast
>  2705 |  if (update && ftrace_trace_function != ftrace_ops_list_func) {
>       |                                      ^~
> 
> --
> Gustavo


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-05 19:08         ` Steven Rostedt
@ 2021-10-05 19:35           ` Gustavo A. R. Silva
  2021-10-06  0:09             ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2021-10-05 19:35 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel

On Tue, Oct 05, 2021 at 03:08:07PM -0400, Steven Rostedt wrote:
[..]
> Or did you not remove your patch first?

Yep; that was the problem. 

I now applied it to a clean tree and the warnings went away.

However, I'm a bit concerned about the following Jann's comments:

"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?"[1]

"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?"[2]

"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."[3]

So, is this linker approach really a good solution to this problem? :)

What's the main problem with what Jann suggests?

Thanks!
--
Gustavo

[1] https://lore.kernel.org/all/CAG48ez2pOns4vF9M_4ubMJ+p9YFY29udMaH0wm8UuCwGQ4ZZAQ@mail.gmail.com/
[2] https://lore.kernel.org/all/CAG48ez04Fj=1p61KAxAQWZ3f_z073fVUr8LsQgtKA9c-kcHmDQ@mail.gmail.com/#t
[3] https://lore.kernel.org/all/CAG48ez1LoTLmHnAKFZCQFSvcb13Em6kc8y1xO8sNwyvzB=D2Lg@mail.gmail.com/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-05 19:35           ` Gustavo A. R. Silva
@ 2021-10-06  0:09             ` Steven Rostedt
  2021-10-06 21:14               ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2021-10-06  0:09 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel, Jann Horn

On Tue, 5 Oct 2021 14:35:57 -0500
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> On Tue, Oct 05, 2021 at 03:08:07PM -0400, Steven Rostedt wrote:
> [..]
> > Or did you not remove your patch first?  
> 
> Yep; that was the problem. 
> 
> I now applied it to a clean tree and the warnings went away.
> 
> However, I'm a bit concerned about the following Jann's comments:

I should have replied back then, but I'll do that now (and added Jann
to the CC) 

> 
> "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?"[1]

It's not dependent on the architecture. It's dependent on what the
architecture has implemented. There's nothing limiting the arch to use
the normal method, except that nobody implemented the updates.

As I changed the core API, it affected the architectures, and since I
don't know how to update all the architectures that use that API, and
do not have the hardware to test it, I made it so architectures can
slowly be updated when their maintainers get time to. This was years
ago, and not much has been done.

> 
> "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?"[2]
> 
> "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."[3]
> 
> So, is this linker approach really a good solution to this problem? :)
> 
> What's the main problem with what Jann suggests?

The main issue is I want no more #ifdef's in the main code. There's too
many already and it makes it difficult to maintain. I want to get rid
of them, not add more. So anything that adds more #ifdef's to the main
code, I will NACK.

Which I guess leaves us with either the linker trick, or having all
the archs get updated to support the latest ftrace features, and we can
remove the current #ifdefs.

-- Steve


> 
> Thanks!
> --
> Gustavo
> 
> [1] https://lore.kernel.org/all/CAG48ez2pOns4vF9M_4ubMJ+p9YFY29udMaH0wm8UuCwGQ4ZZAQ@mail.gmail.com/
> [2] https://lore.kernel.org/all/CAG48ez04Fj=1p61KAxAQWZ3f_z073fVUr8LsQgtKA9c-kcHmDQ@mail.gmail.com/#t
> [3] https://lore.kernel.org/all/CAG48ez1LoTLmHnAKFZCQFSvcb13Em6kc8y1xO8sNwyvzB=D2Lg@mail.gmail.com/


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-06  0:09             ` Steven Rostedt
@ 2021-10-06 21:14               ` Gustavo A. R. Silva
  2021-10-06 21:14                 ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2021-10-06 21:14 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel, Jann Horn

On Tue, Oct 05, 2021 at 08:09:35PM -0400, Steven Rostedt wrote:
> On Tue, 5 Oct 2021 14:35:57 -0500
> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> 
> > On Tue, Oct 05, 2021 at 03:08:07PM -0400, Steven Rostedt wrote:
> > [..]
> > > Or did you not remove your patch first?  
> > 
> > Yep; that was the problem. 
> > 
> > I now applied it to a clean tree and the warnings went away.
> > 
> > However, I'm a bit concerned about the following Jann's comments:
> 
> I should have replied back then, but I'll do that now (and added Jann
> to the CC) 
> 
> > 
> > "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?"[1]
> 
> It's not dependent on the architecture. It's dependent on what the
> architecture has implemented. There's nothing limiting the arch to use
> the normal method, except that nobody implemented the updates.
> 
> As I changed the core API, it affected the architectures, and since I
> don't know how to update all the architectures that use that API, and
> do not have the hardware to test it, I made it so architectures can
> slowly be updated when their maintainers get time to. This was years
> ago, and not much has been done.
> 
> > 
> > "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?"[2]
> > 
> > "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."[3]
> > 
> > So, is this linker approach really a good solution to this problem? :)
> > 
> > What's the main problem with what Jann suggests?
> 
> The main issue is I want no more #ifdef's in the main code. There's too
> many already and it makes it difficult to maintain. I want to get rid
> of them, not add more. So anything that adds more #ifdef's to the main
> code, I will NACK.
> 
> Which I guess leaves us with either the linker trick, or having all
> the archs get updated to support the latest ftrace features, and we can
> remove the current #ifdefs.

OK. Are you going to apply your patch any time soon? So, I can go and
enable -Wcast-function-type in my -next tree. :)

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-06 21:14               ` Gustavo A. R. Silva
@ 2021-10-06 21:14                 ` Steven Rostedt
  2021-10-06 21:23                   ` Gustavo A. R. Silva
  2021-10-13  1:40                   ` Gustavo A. R. Silva
  0 siblings, 2 replies; 16+ messages in thread
From: Steven Rostedt @ 2021-10-06 21:14 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel, Jann Horn

On Wed, 6 Oct 2021 16:14:26 -0500
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> > Which I guess leaves us with either the linker trick, or having all
> > the archs get updated to support the latest ftrace features, and we can
> > remove the current #ifdefs.  
> 
> OK. Are you going to apply your patch any time soon? So, I can go and
> enable -Wcast-function-type in my -next tree. :)

Sure. I only did not add it because of the issue Jann brought up. But if it
is needed, and I do not want more #ifdef all over the code, I'll add it,
and perhaps even mark it for stable.

I'm working on some other fixes now anyway.

-- Steve


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-06 21:14                 ` Steven Rostedt
@ 2021-10-06 21:23                   ` Gustavo A. R. Silva
  2021-10-13  1:40                   ` Gustavo A. R. Silva
  1 sibling, 0 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2021-10-06 21:23 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel, Jann Horn

On Wed, Oct 06, 2021 at 05:14:43PM -0400, Steven Rostedt wrote:
> > OK. Are you going to apply your patch any time soon? So, I can go and
> > enable -Wcast-function-type in my -next tree. :)
> 
> Sure. I only did not add it because of the issue Jann brought up. But if it
> is needed, and I do not want more #ifdef all over the code, I'll add it,
> and perhaps even mark it for stable.

Awesome. :)

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-06 21:14                 ` Steven Rostedt
  2021-10-06 21:23                   ` Gustavo A. R. Silva
@ 2021-10-13  1:40                   ` Gustavo A. R. Silva
  2021-10-13  2:23                     ` Steven Rostedt
  1 sibling, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2021-10-13  1:40 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel, Jann Horn

Steve,

On Wed, Oct 06, 2021 at 05:14:43PM -0400, Steven Rostedt wrote:
> On Wed, 6 Oct 2021 16:14:26 -0500
> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> 
> > > Which I guess leaves us with either the linker trick, or having all
> > > the archs get updated to support the latest ftrace features, and we can
> > > remove the current #ifdefs.  
> > 
> > OK. Are you going to apply your patch any time soon? So, I can go and
> > enable -Wcast-function-type in my -next tree. :)
> 
> Sure. I only did not add it because of the issue Jann brought up. But if it
> is needed, and I do not want more #ifdef all over the code, I'll add it,
> and perhaps even mark it for stable.
> 
> I'm working on some other fixes now anyway.

Do you mind if, in the meantime, I add your patch to my -next tree?
So, I can enable -Wcast-function-type in linux-next --I want to get
ready for the next merge window.

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-13  1:40                   ` Gustavo A. R. Silva
@ 2021-10-13  2:23                     ` Steven Rostedt
  2021-10-13  2:39                       ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2021-10-13  2:23 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel, Jann Horn

On Tue, 12 Oct 2021 20:40:42 -0500
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> Do you mind if, in the meantime, I add your patch to my -next tree?
> So, I can enable -Wcast-function-type in linux-next --I want to get
> ready for the next merge window.

You mean to push it to Linus as well? I'm not sure that's the best way.
I'm still working on some bugs that are going to go into this rc
release, and then I plan on pushing my for-next queue (which includes
this change).

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-13  2:23                     ` Steven Rostedt
@ 2021-10-13  2:39                       ` Gustavo A. R. Silva
  2021-10-13  2:44                         ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2021-10-13  2:39 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel, Jann Horn

On Tue, Oct 12, 2021 at 10:23:40PM -0400, Steven Rostedt wrote:
> On Tue, 12 Oct 2021 20:40:42 -0500
> "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> 
> > Do you mind if, in the meantime, I add your patch to my -next tree?
> > So, I can enable -Wcast-function-type in linux-next --I want to get
> > ready for the next merge window.
> 
> You mean to push it to Linus as well? I'm not sure that's the best way.

No. Just add it to my -next tree, temporarily. So, I can enable the
compiler option and it can be tested in linux-next (and no more of
these issues are introduced). The only part I'll send to Linus would
be the Makefile patch.

> I'm still working on some bugs that are going to go into this rc
> release, and then I plan on pushing my for-next queue (which includes
> this change).

Once you have it ready, I would remove it from my tree.

--
Gustavo



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64
  2021-10-13  2:39                       ` Gustavo A. R. Silva
@ 2021-10-13  2:44                         ` Steven Rostedt
  0 siblings, 0 replies; 16+ messages in thread
From: Steven Rostedt @ 2021-10-13  2:44 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Ingo Molnar, Daniel Bristot de Oliveira, linux-kernel, Jann Horn

On Tue, 12 Oct 2021 21:39:20 -0500
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:

> On Tue, Oct 12, 2021 at 10:23:40PM -0400, Steven Rostedt wrote:
> > On Tue, 12 Oct 2021 20:40:42 -0500
> > "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
> >   
> > > Do you mind if, in the meantime, I add your patch to my -next tree?
> > > So, I can enable -Wcast-function-type in linux-next --I want to get
> > > ready for the next merge window.  
> > 
> > You mean to push it to Linus as well? I'm not sure that's the best way.  
> 
> No. Just add it to my -next tree, temporarily. So, I can enable the
> compiler option and it can be tested in linux-next (and no more of
> these issues are introduced). The only part I'll send to Linus would
> be the Makefile patch.

Sure, I have no problem with that.

> 
> > I'm still working on some bugs that are going to go into this rc
> > release, and then I plan on pushing my for-next queue (which includes
> > this change).  
> 
> Once you have it ready, I would remove it from my tree.

Sounds like a plan ;-)

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-10-13  2:44 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-05  5:39 [PATCH][next] ftrace: Fix -Wcast-function-type warnings on powerpc64 Gustavo A. R. Silva
2021-10-05 15:17 ` Steven Rostedt
2021-10-05 16:18   ` Gustavo A. R. Silva
2021-10-05 16:35     ` Steven Rostedt
2021-10-05 16:50       ` Gustavo A. R. Silva
2021-10-05 16:51         ` Steven Rostedt
2021-10-05 19:08         ` Steven Rostedt
2021-10-05 19:35           ` Gustavo A. R. Silva
2021-10-06  0:09             ` Steven Rostedt
2021-10-06 21:14               ` Gustavo A. R. Silva
2021-10-06 21:14                 ` Steven Rostedt
2021-10-06 21:23                   ` Gustavo A. R. Silva
2021-10-13  1:40                   ` Gustavo A. R. Silva
2021-10-13  2:23                     ` Steven Rostedt
2021-10-13  2:39                       ` Gustavo A. R. Silva
2021-10-13  2:44                         ` Steven Rostedt

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