[PATCH] ftrace: fix en(dis)able graph caller when en(dis)abling record via sysctl

[PATCH] ftrace: fix en(dis)able graph caller when en(dis)abling record via sysctl

[Pratyush Anand]

When ftrace is enabled globally through proc interface, we must check if
ftrace_graph_active is set. If it is set, then we should also pass
FTRACE_START_FUNC_RET command to ftrace_run_update_code. Similarly, when
ftrace is disabled globally through proc interface, we must check if
ftrace_graph_active is set. If it is set, then we should also pass
FTRACE_STOP_FUNC_RET command to ftrace_run_update_code.

Consider following situation.

 # echo 0 > /proc/sys/kernel/ftrace_enabled

After this ftrace_enabled = 0.

 # echo function_graph > /sys/kernel/debug/tracing/current_tracer

Since ftrace_enabled = 0. Therefore, ftrace_enable_ftrace_graph_caller
is never called.

 # echo 1 > /proc/sys/kernel/ftrace_enabled

Now ftrace_enabled will be set to true, but still
ftrace_enable_ftrace_graph_caller will not be called, which is not
desired.

Further if we execute following after this:
  # echo nop > /sys/kernel/debug/tracing/current_tracer

Now since ftrace_enabled is set, so it will call
ftrace_disable_ftrace_graph_caller, which causes a kernel warning on ARM
platform.

On ARM platform, when ftrace_enable_ftrace_graph_caller is called, it
checks whether old instruction is nop or not. If its not nop, then it
returns error. If its nop then it replaces instruction at that address
with a branch to ftrace_graph_caller. ftrace_disable_ftrace_graph_caller
behaves just opposite. Therefore if generic ftrace code ever calls
either ftrace_enable_ftrace_graph_caller or
ftrace_disable_ftrace_graph_caller consecutively two times, then it will
return error, which will cause generic ftrace code to raise a warn.

This patch will fixes the issue described here.

Signed-off-by: Pratyush Anand <panand@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 010ab93660ec..bdd63154e724 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1058,6 +1058,7 @@ static __init void ftrace_profile_debugfs(struct dentry *d_tracer)
 #endif /* CONFIG_FUNCTION_PROFILER */
 
 static struct pid * const ftrace_swapper_pid = &init_struct_pid;
+static int ftrace_graph_active;
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 
@@ -2692,24 +2693,38 @@ static int ftrace_shutdown(struct ftrace_ops *ops, int command)
 
 static void ftrace_startup_sysctl(void)
 {
+	int command;
+
 	if (unlikely(ftrace_disabled))
 		return;
 
 	/* Force update next time */
 	saved_ftrace_func = NULL;
 	/* ftrace_start_up is true if we want ftrace running */
+	if (ftrace_start_up) {
+		command = FTRACE_UPDATE_CALLS;
+		if (ftrace_graph_active)
+			command |= FTRACE_START_FUNC_RET;
+		ftrace_run_update_code(command);
+	}
 	if (ftrace_start_up)
 		ftrace_run_update_code(FTRACE_UPDATE_CALLS);
 }
 
 static void ftrace_shutdown_sysctl(void)
 {
+	int command;
+
 	if (unlikely(ftrace_disabled))
 		return;
 
 	/* ftrace_start_up is true if ftrace is running */
-	if (ftrace_start_up)
-		ftrace_run_update_code(FTRACE_DISABLE_CALLS);
+	if (ftrace_start_up) {
+		command = FTRACE_DISABLE_CALLS;
+		if (ftrace_graph_active)
+			command |= FTRACE_STOP_FUNC_RET;
+		ftrace_run_update_code(command);
+	}
 }
 
 static cycle_t		ftrace_update_time;
@@ -5594,8 +5609,6 @@ static struct ftrace_ops graph_ops = {
 	ASSIGN_OPS_HASH(graph_ops, &global_ops.local_hash)
 };
 
-static int ftrace_graph_active;
-
 int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
 {
 	return 0;
-- 
2.1.0

Re: [PATCH] ftrace: fix en(dis)able graph caller when en(dis)abling record via sysctl

[Steven Rostedt]

On Fri,  6 Mar 2015 23:58:06 +0530
Pratyush Anand <panand@redhat.com> wrote:
 
>  	/* Force update next time */
>  	saved_ftrace_func = NULL;
>  	/* ftrace_start_up is true if we want ftrace running */
> +	if (ftrace_start_up) {
> +		command = FTRACE_UPDATE_CALLS;
> +		if (ftrace_graph_active)
> +			command |= FTRACE_START_FUNC_RET;
> +		ftrace_run_update_code(command);
> +	}
>  	if (ftrace_start_up)
>  		ftrace_run_update_code(FTRACE_UPDATE_CALLS);

Hmm, I guess you forgot to delete the above if statement (that gets
replaced by the if statement before it).

No need to send another patch, I removed it.

-- Steve

>  }
>  
>  static void ftrace_shutdown_sysctl(void)
>  {
> +	int command;
> +
>  	if (unlikely(ftrace_disabled))
>  		return;
>  
>  	/* ftrace_start_up is true if ftrace is running */
> -	if (ftrace_start_up)
> -		ftrace_run_update_code(FTRACE_DISABLE_CALLS);
> +	if (ftrace_start_up) {
> +		command = FTRACE_DISABLE_CALLS;
> +		if (ftrace_graph_active)
> +			command |= FTRACE_STOP_FUNC_RET;
> +		ftrace_run_update_code(command);
> +	}
>  }
>  
>  static cycle_t		ftrace_update_time;
> @@ -5594,8 +5609,6 @@ static struct ftrace_ops graph_ops = {
>  	ASSIGN_OPS_HASH(graph_ops, &global_ops.local_hash)
>  };
>  
> -static int ftrace_graph_active;
> -
>  int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
>  {
>  	return 0;

Re: [PATCH] ftrace: fix en(dis)able graph caller when en(dis)abling record via sysctl

[Pratyush Anand]

On Saturday 07 March 2015 02:59 AM, Steven Rostedt wrote:
>>   	if (ftrace_start_up)
>> >  		ftrace_run_update_code(FTRACE_UPDATE_CALLS);
> Hmm, I guess you forgot to delete the above if statement (that gets
> replaced by the if statement before it).
>
> No need to send another patch, I removed it.

Thanks :)