Re: [BUG] tracing: dynamic ftrace selftest detected failures

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Wed, 21 Aug 2024 08:43:51 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Tue, 20 Aug 2024 18:11:09 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Wed, 21 Aug 2024 07:05:39 +0900
> > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > 
> > 
> > > Does the noinline attribute prevent embedding callsite too? I mean
> > > 
> > > extern callee()
> > > 
> > > noinline callee()
> > > {
> > > ...
> > > }
> > > 
> > > caller()
> > > {
> > > 	callee() // (*)
> > > }
> > > 
> > > In this case, does noinline prevent LTO to embed the callee at the callsite(*)
> > > or prevent LTO remove the callee() symbol?
> > >
> > 
> > Even though we have it passed as a parameter, I think the compiler and
> > linker is smart enough to see that and notice its use, and that the
> > function passed in is a nop, which doesn't break the flow.
> > 
> > Can you add the __used and see if it fixes it?
> 
> Adding __used to DYN_FTRACE_TEST_NAME() and DYN_FTRACE_TEST_NAME2() does
> not change, the test still fails. Hmm, what about makes the caller
> (trace_selftest_startup_dynamic_tracing()) called via a function pointer?
> In that case, wouldn't it be subject to constant propagetion?
> 
> Let me try.

OK, it is succeeded! Calling `caller` via global function pointer makes
it run as we expected. It passed the dynamic_ftrace test, but other tests
still fails. Those need to be called via function pointer too.
                             
[    1.851324] Testing dynamic ftrace: PASSED                                     
[    2.083329] Testing dynamic ftrace ops #1:                                     
[    2.173751] (0 0 0 0 0) FAILED!                                                
[    2.182337] ------------[ cut here ]------------                               
[    2.183323] WARNING: CPU: 0 PID: 1 at kernel/trace/trace.c:2143 run_tracer_sel0
[    2.184323] Modules linked in:                              

Anyway, here is what I did.

diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index 97f1e4bc47dc..9663bc777888 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -353,9 +353,10 @@ static int trace_selftest_ops(struct trace_array *tr, int cnt)
 }
 
 /* Test dynamic code modification and ftrace filters */
-static int trace_selftest_startup_dynamic_tracing(struct tracer *trace,
-						  struct trace_array *tr,
-						  int (*func)(void))
+static int noinline
+trace_selftest_startup_dynamic_tracing(struct tracer *trace,
+					struct trace_array *tr,
+					int (*func)(void))
 {
 	int save_ftrace_enabled = ftrace_enabled;
 	unsigned long count;
@@ -569,10 +570,22 @@ trace_selftest_function_recursion(void)
 	return ret;
 }
 #else
-# define trace_selftest_startup_dynamic_tracing(trace, tr, func) ({ 0; })
+static int trace_selftest_startup_dynamic_tracing(struct tracer *trace,
+					struct trace_array *tr,
+					int (*func)(void))
+{
+	if (!trace || !tr || !func)
+		return -EINVAL;
+	return 0;
+}
 # define trace_selftest_function_recursion() ({ 0; })
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
+int (*global_trace_selftest_startup_dynamic_tracing)(struct tracer *trace,
+					struct trace_array *tr,
+					int (*func)(void))
+	= trace_selftest_startup_dynamic_tracing;
+
 static enum {
 	TRACE_SELFTEST_REGS_START,
 	TRACE_SELFTEST_REGS_FOUND,
@@ -732,7 +745,7 @@ trace_selftest_startup_function(struct tracer *trace, struct trace_array *tr)
 		goto out;
 	}
 
-	ret = trace_selftest_startup_dynamic_tracing(trace, tr,
+	ret = global_trace_selftest_startup_dynamic_tracing(trace, tr,
 						     DYN_FTRACE_TEST_NAME);
 	if (ret)
 		goto out;

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>