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
>
>
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html