Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1539402 > unrolled thread
| Started by | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| First post | 2016-12-09 15:30 +0100 |
| Last post | 2016-12-12 19:10 +0100 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace Steven Rostedt <rostedt@goodmis.org> - 2016-12-09 15:30 +0100
Fwd: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace Namhyung Kim <namhyung@kernel.org> - 2016-12-12 17:40 +0100
Re: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace Steven Rostedt <rostedt@goodmis.org> - 2016-12-12 17:50 +0100
Re: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace Namhyung Kim <namhyung@kernel.org> - 2016-12-12 18:30 +0100
Re: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace Steven Rostedt <rostedt@goodmis.org> - 2016-12-12 19:10 +0100
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-12-09 15:30 +0100 |
| Subject | [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace |
| Message-ID | <sMuls-4n9-47@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2016-12-12 17:40 +0100 |
| Subject | Fwd: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace |
| Message-ID | <sNBNT-6Du-25@gated-at.bofh.it> |
| In reply to | #1539402 |
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
>
>
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-12-12 17:50 +0100 |
| Subject | Re: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace |
| Message-ID | <sNBXz-6GG-9@gated-at.bofh.it> |
| In reply to | #1540442 |
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
[toc] | [prev] | [next] | [standalone]
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2016-12-12 18:30 +0100 |
| Subject | Re: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace |
| Message-ID | <sNCAh-796-17@gated-at.bofh.it> |
| In reply to | #1540451 |
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
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-12-12 19:10 +0100 |
| Subject | Re: [for-next][PATCH 7/8] fgraph: Handle a case where a tracer ignores set_graph_notrace |
| Message-ID | <sNDcZ-7Bu-9@gated-at.bofh.it> |
| In reply to | #1540475 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web