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@xxxxxxxxxxx> wrote:
> From: "Steven Rostedt (Red Hat)" <rostedt@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> Cc: Namhyung Kim <namhyung.kim@xxxxxxx>
> Fixes: 29ad23b00474 ("ftrace: Add set_graph_notrace filter")
> Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> ---
>  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
>
>