[for-next][PATCH 5/8] ftrace/x86_32: Set ftrace_stub to weak to prevent gcc from using short jumps to it

[for-next][PATCH 5/8] ftrace/x86_32: Set ftrace_stub to weak to prevent gcc from using short jumps to it

[Steven Rostedt]

[-- Attachment #1: 0005-ftrace-x86_32-Set-ftrace_stub-to-weak-to-prevent-gcc.patch --]
[-- Type: text/plain, Size: 1377 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

With new binutils, gcc may get smart with its optimization and change a jmp
from a 5 byte jump to a 2 byte one even though it was jumping to a global
function. But that global function existed within a 2 byte radius, and gcc
was able to optimize it. Unfortunately, that jump was also being modified
when function graph tracing begins. Since ftrace expected that jump to be 5
bytes, but it was only two, it overwrote code after the jump, causing a
crash.

This was fixed for x86_64 with commit 8329e818f149, with the same subject as
this commit, but nothing was done for x86_32.

Cc: stable@vger.kernel.org
Fixes: d61f82d06672 ("ftrace: use dynamic patching for updating mcount calls")
Reported-by: Colin Ian King <colin.king@canonical.com>
Tested-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/entry/entry_32.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 21b352a11b49..edba8606b99a 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -889,8 +889,8 @@ ftrace_graph_call:
 	jmp	ftrace_stub
 #endif
 
-.globl ftrace_stub
-ftrace_stub:
+/* This is weak to keep gas from relaxing the jumps */
+WEAK(ftrace_stub)
 	ret
 END(ftrace_caller)
 
-- 
2.10.2

[for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace

[Steven Rostedt]

[-- Attachment #1: 0007-fgraph-Handle-a-case-where-a-tracer-ignores-set_grap.patch --]
[-- Type: text/plain, Size: 3711 bytes --]

From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>

Both the wakeup and irqsoff tracers can use the function graph tracer when
the display-graph option is set. The problem is that they ignore the notrace
file, and record the entry of functions that would be ignored by the
function_graph tracer. This causes the trace->depth to be recorded into the
ring buffer. The set_graph_notrace uses a trick by adding a large negative
number to the trace->depth when a graph function is to be ignored.

On trace output, the graph function uses the depth to record a stack of
functions. But since the depth is negative, it accesses the array with a
negative number and causes an out of bounds access that can cause a kernel
oops or corrupt data.

Have the print functions handle cases where a tracer still records functions
even when they are in set_graph_notrace.

Also add warnings if the depth is below zero before accessing the array.

Note, the function graph logic will still prevent the return of these
functions from being recorded, which means that they will be left hanging
without a return. For example:

   # echo '*spin*' > set_graph_notrace
   # echo 1 > options/display-graph
   # echo wakeup > current_tracer
   # cat trace
   [...]
      _raw_spin_lock() {
        preempt_count_add() {
        do_raw_spin_lock() {
      update_rq_clock();

Where it should look like:

      _raw_spin_lock() {
        preempt_count_add();
        do_raw_spin_lock();
      }
      update_rq_clock();

Cc: stable@vger.kernel.org
Cc: Namhyung Kim <namhyung.kim@lge.com>
Fixes: 29ad23b00474 ("ftrace: Add set_graph_notrace filter")
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_functions_graph.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 8e1a115439fa..566f7327c3aa 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -842,6 +842,10 @@ print_graph_entry_leaf(struct trace_iterator *iter,
 
 		cpu_data = per_cpu_ptr(data->cpu_data, cpu);
 
+		/* If a graph tracer ignored set_graph_notrace */
+		if (call->depth < -1)
+			call->depth += FTRACE_NOTRACE_DEPTH;
+
 		/*
 		 * Comments display at + 1 to depth. Since
 		 * this is a leaf function, keep the comments
@@ -850,7 +854,8 @@ print_graph_entry_leaf(struct trace_iterator *iter,
 		cpu_data->depth = call->depth - 1;
 
 		/* No need to keep this function around for this depth */
-		if (call->depth < FTRACE_RETFUNC_DEPTH)
+		if (call->depth < FTRACE_RETFUNC_DEPTH &&
+		    !WARN_ON_ONCE(call->depth < 0))
 			cpu_data->enter_funcs[call->depth] = 0;
 	}
 
@@ -880,11 +885,16 @@ print_graph_entry_nested(struct trace_iterator *iter,
 		struct fgraph_cpu_data *cpu_data;
 		int cpu = iter->cpu;
 
+		/* If a graph tracer ignored set_graph_notrace */
+		if (call->depth < -1)
+			call->depth += FTRACE_NOTRACE_DEPTH;
+
 		cpu_data = per_cpu_ptr(data->cpu_data, cpu);
 		cpu_data->depth = call->depth;
 
 		/* Save this function pointer to see if the exit matches */
-		if (call->depth < FTRACE_RETFUNC_DEPTH)
+		if (call->depth < FTRACE_RETFUNC_DEPTH &&
+		    !WARN_ON_ONCE(call->depth < 0))
 			cpu_data->enter_funcs[call->depth] = call->func;
 	}
 
@@ -1114,7 +1124,8 @@ print_graph_return(struct ftrace_graph_ret *trace, struct trace_seq *s,
 		 */
 		cpu_data->depth = trace->depth - 1;
 
-		if (trace->depth < FTRACE_RETFUNC_DEPTH) {
+		if (trace->depth < FTRACE_RETFUNC_DEPTH &&
+		    !WARN_ON_ONCE(trace->depth < 0)) {
 			if (cpu_data->enter_funcs[trace->depth] != trace->func)
 				func_match = 0;
 			cpu_data->enter_funcs[trace->depth] = 0;
-- 
2.10.2

Fwd: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace

[Namhyung Kim]

Hi Steve,

On Fri, Dec 9, 2016 at 11:27 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
>
> Both the wakeup and irqsoff tracers can use the function graph tracer when
> the display-graph option is set. The problem is that they ignore the notrace
> file, and record the entry of functions that would be ignored by the
> function_graph tracer. This causes the trace->depth to be recorded into the
> ring buffer. The set_graph_notrace uses a trick by adding a large negative
> number to the trace->depth when a graph function is to be ignored.
>
> On trace output, the graph function uses the depth to record a stack of
> functions. But since the depth is negative, it accesses the array with a
> negative number and causes an out of bounds access that can cause a kernel
> oops or corrupt data.

Sorry to miss updating those tracers.  I guess it's no more necessary once
the patch 8 is applied so that functions in the notrace filter will not be
recorded.

Or maybe we need to change the prepare_ftrace_return() so that the
graph_entry callback should be called after ftrace_push_return_trace() as
some archs do.

>
> Have the print functions handle cases where a tracer still records functions
> even when they are in set_graph_notrace.

I think it'd be better (or consistent, at least) not printing negative index
records rather than showing entry only.

>
> Also add warnings if the depth is below zero before accessing the array.
>
> Note, the function graph logic will still prevent the return of these
> functions from being recorded, which means that they will be left hanging
> without a return. For example:
>
>    # echo '*spin*' > set_graph_notrace
>    # echo 1 > options/display-graph
>    # echo wakeup > current_tracer
>    # cat trace
>    [...]
>       _raw_spin_lock() {
>         preempt_count_add() {
>         do_raw_spin_lock() {
>       update_rq_clock();
>
> Where it should look like:
>
>       _raw_spin_lock() {
>         preempt_count_add();
>         do_raw_spin_lock();
>       }
>       update_rq_clock();

If set_graph_notrace works correctly, it should be just:

         update_rq_clock();

Thanks,
Namhyung


>
> Cc: stable@vger.kernel.org
> Cc: Namhyung Kim <namhyung.kim@lge.com>
> Fixes: 29ad23b00474 ("ftrace: Add set_graph_notrace filter")
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_functions_graph.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 8e1a115439fa..566f7327c3aa 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -842,6 +842,10 @@ print_graph_entry_leaf(struct trace_iterator *iter,
>
>                 cpu_data = per_cpu_ptr(data->cpu_data, cpu);
>
> +               /* If a graph tracer ignored set_graph_notrace */
> +               if (call->depth < -1)
> +                       call->depth += FTRACE_NOTRACE_DEPTH;
> +
>                 /*
>                  * Comments display at + 1 to depth. Since
>                  * this is a leaf function, keep the comments
> @@ -850,7 +854,8 @@ print_graph_entry_leaf(struct trace_iterator *iter,
>                 cpu_data->depth = call->depth - 1;
>
>                 /* No need to keep this function around for this depth */
> -               if (call->depth < FTRACE_RETFUNC_DEPTH)
> +               if (call->depth < FTRACE_RETFUNC_DEPTH &&
> +                   !WARN_ON_ONCE(call->depth < 0))
>                         cpu_data->enter_funcs[call->depth] = 0;
>         }
>
> @@ -880,11 +885,16 @@ print_graph_entry_nested(struct trace_iterator *iter,
>                 struct fgraph_cpu_data *cpu_data;
>                 int cpu = iter->cpu;
>
> +               /* If a graph tracer ignored set_graph_notrace */
> +               if (call->depth < -1)
> +                       call->depth += FTRACE_NOTRACE_DEPTH;
> +
>                 cpu_data = per_cpu_ptr(data->cpu_data, cpu);
>                 cpu_data->depth = call->depth;
>
>                 /* Save this function pointer to see if the exit matches */
> -               if (call->depth < FTRACE_RETFUNC_DEPTH)
> +               if (call->depth < FTRACE_RETFUNC_DEPTH &&
> +                   !WARN_ON_ONCE(call->depth < 0))
>                         cpu_data->enter_funcs[call->depth] = call->func;
>         }
>
> @@ -1114,7 +1124,8 @@ print_graph_return(struct ftrace_graph_ret *trace, struct trace_seq *s,
>                  */
>                 cpu_data->depth = trace->depth - 1;
>
> -               if (trace->depth < FTRACE_RETFUNC_DEPTH) {
> +               if (trace->depth < FTRACE_RETFUNC_DEPTH &&
> +                   !WARN_ON_ONCE(trace->depth < 0)) {
>                         if (cpu_data->enter_funcs[trace->depth] != trace->func)
>                                 func_match = 0;
>                         cpu_data->enter_funcs[trace->depth] = 0;
> --
> 2.10.2
>
>

Re: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace

[Steven Rostedt]

On Tue, 13 Dec 2016 01:30:01 +0900
Namhyung Kim <namhyung@kernel.org> wrote:


> Sorry to miss updating those tracers.  I guess it's no more necessary once
> the patch 8 is applied so that functions in the notrace filter will not be
> recorded.
> 
> Or maybe we need to change the prepare_ftrace_return() so that the
> graph_entry callback should be called after ftrace_push_return_trace() as
> some archs do.

I plan on updating fgraph in general so this should all be handled then.

> 
> >
> > Have the print functions handle cases where a tracer still records functions
> > even when they are in set_graph_notrace.  
> 
> I think it'd be better (or consistent, at least) not printing negative index
> records rather than showing entry only.

I thought about this too, but I'm more concerned about it not crashing
the kernel than to show a proper trace. The fix will just make sure it
doesn't crash.

> 
> >
> > Also add warnings if the depth is below zero before accessing the array.
> >
> > Note, the function graph logic will still prevent the return of these
> > functions from being recorded, which means that they will be left hanging
> > without a return. For example:
> >
> >    # echo '*spin*' > set_graph_notrace
> >    # echo 1 > options/display-graph
> >    # echo wakeup > current_tracer
> >    # cat trace
> >    [...]
> >       _raw_spin_lock() {
> >         preempt_count_add() {
> >         do_raw_spin_lock() {
> >       update_rq_clock();
> >
> > Where it should look like:
> >
> >       _raw_spin_lock() {
> >         preempt_count_add();
> >         do_raw_spin_lock();
> >       }
> >       update_rq_clock();  
> 
> If set_graph_notrace works correctly, it should be just:
> 
>          update_rq_clock();

Which is what it should look like after patch 8. But I didn't mark 8 as
stable as that's more of a feature. As wakeup and irqsoff doesn't use
notrace yet. Yeah, notrace may break it a bit, but since this is the
first someone noticed it, I don't think it's used much.

I wanted the simplest fix for stable.

-- Steve

Re: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace

[Namhyung Kim]

On Mon, Dec 12, 2016 at 11:49:20AM -0500, Steven Rostedt wrote:
> On Tue, 13 Dec 2016 01:30:01 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
> 
> 
> > Sorry to miss updating those tracers.  I guess it's no more necessary once
> > the patch 8 is applied so that functions in the notrace filter will not be
> > recorded.
> > 
> > Or maybe we need to change the prepare_ftrace_return() so that the
> > graph_entry callback should be called after ftrace_push_return_trace() as
> > some archs do.
> 
> I plan on updating fgraph in general so this should all be handled then.

ok

> 
> > 
> > >
> > > Have the print functions handle cases where a tracer still records functions
> > > even when they are in set_graph_notrace.  
> > 
> > I think it'd be better (or consistent, at least) not printing negative index
> > records rather than showing entry only.
> 
> I thought about this too, but I'm more concerned about it not crashing
> the kernel than to show a proper trace. The fix will just make sure it
> doesn't crash.

ok

> 
> > 
> > >
> > > Also add warnings if the depth is below zero before accessing the array.
> > >
> > > Note, the function graph logic will still prevent the return of these
> > > functions from being recorded, which means that they will be left hanging
> > > without a return. For example:
> > >
> > >    # echo '*spin*' > set_graph_notrace
> > >    # echo 1 > options/display-graph
> > >    # echo wakeup > current_tracer
> > >    # cat trace
> > >    [...]
> > >       _raw_spin_lock() {
> > >         preempt_count_add() {
> > >         do_raw_spin_lock() {
> > >       update_rq_clock();
> > >
> > > Where it should look like:
> > >
> > >       _raw_spin_lock() {
> > >         preempt_count_add();
> > >         do_raw_spin_lock();
> > >       }
> > >       update_rq_clock();  
> > 
> > If set_graph_notrace works correctly, it should be just:
> > 
> >          update_rq_clock();
> 
> Which is what it should look like after patch 8. But I didn't mark 8 as
> stable as that's more of a feature. As wakeup and irqsoff doesn't use
> notrace yet. Yeah, notrace may break it a bit, but since this is the
> first someone noticed it, I don't think it's used much.
> 
> I wanted the simplest fix for stable.

I think a simpler fix is just to return when it sees a negative record..


diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 52fb1e21b86b..2fb73c2e35b5 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -844,7 +844,7 @@ print_graph_entry_leaf(struct trace_iterator *iter,
 
                /* If a graph tracer ignored set_graph_notrace */
                if (call->depth < -1)
-                       call->depth += FTRACE_NOTRACE_DEPTH;
+                       return TRACE_TYPE_HANDLED;
 
                /*
                 * Comments display at + 1 to depth. Since
@@ -887,7 +887,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
 
                /* If a graph tracer ignored set_graph_notrace */
                if (call->depth < -1)
-                       call->depth += FTRACE_NOTRACE_DEPTH;
+                       return TRACE_TYPE_HANDLED;
 
                cpu_data = per_cpu_ptr(data->cpu_data, cpu);
                cpu_data->depth = call->depth;


Thanks,
Namhyung

Re: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace

[Steven Rostedt]

On Tue, 13 Dec 2016 02:09:04 +0900
Namhyung Kim <namhyung@kernel.org> wrote:


> > I wanted the simplest fix for stable.  
> 
> I think a simpler fix is just to return when it sees a negative record..

You're right, but I guess I was trying to get it someone closer to the
final change too.

-- Steve

> 
> 
> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 52fb1e21b86b..2fb73c2e35b5 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -844,7 +844,7 @@ print_graph_entry_leaf(struct trace_iterator *iter,
>  
>                 /* If a graph tracer ignored set_graph_notrace */
>                 if (call->depth < -1)
> -                       call->depth += FTRACE_NOTRACE_DEPTH;
> +                       return TRACE_TYPE_HANDLED;
>  
>                 /*
>                  * Comments display at + 1 to depth. Since
> @@ -887,7 +887,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
>  
>                 /* If a graph tracer ignored set_graph_notrace */
>                 if (call->depth < -1)
> -                       call->depth += FTRACE_NOTRACE_DEPTH;
> +                       return TRACE_TYPE_HANDLED;
>  
>                 cpu_data = per_cpu_ptr(data->cpu_data, cpu);
>                 cpu_data->depth = call->depth;
> 
> 
> Thanks,
> Namhyung