* [PATCH v3] arm64: fix unwind_frame() for filtered out fn for function graph tracing
@ 2018-01-12 10:48 Jerome Marchand
2018-01-16 17:57 ` Catalin Marinas
2018-02-05 13:28 ` [PATCH v4] " Jerome Marchand
0 siblings, 2 replies; 4+ messages in thread
From: Jerome Marchand @ 2018-01-12 10:48 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Catalin Marinas, Will Deacon, linux-kernel, Pratyush Anand,
Jerome Marchand
From: Pratyush Anand <panand@redhat.com>
Change since v2:
- Set frame.graph = current->curr_ret_stack in profile_pc
- Check that frame->graph != -1 in unwind_frame
do_task_stat() calls get_wchan(), which further does unwind_frame().
unwind_frame() restores frame->pc to original value in case function
graph tracer has modified a return address (LR) in a stack frame to hook
a function return. However, if function graph tracer has hit a filtered
function, then we can't unwind it as ftrace_push_return_trace() has
biased the index(frame->graph) with a 'huge negative'
offset(-FTRACE_NOTRACE_DEPTH).
Moreover, arm64 stack walker defines index(frame->graph) as unsigned
int, which can not compare a -ve number.
Similar problem we can have with calling of walk_stackframe() from
save_stack_trace_tsk() or dump_backtrace().
This patch fixes unwind_frame() to test the index for -ve value and
restore index accordingly before we can restore frame->pc.
Reproducer:
cd /sys/kernel/debug/tracing/
echo schedule > set_graph_notrace
echo 1 > options/display-graph
echo wakeup > current_tracer
ps -ef | grep -i agent
Above commands result in:
Unable to handle kernel paging request at virtual address ffff801bd3d1e000
pgd = ffff8003cbe97c00
[ffff801bd3d1e000] *pgd=0000000000000000, *pud=0000000000000000
Internal error: Oops: 96000006 [#1] SMP
[...]
CPU: 5 PID: 11696 Comm: ps Not tainted 4.11.0+ #33
[...]
task: ffff8003c21ba000 task.stack: ffff8003cc6c0000
PC is at unwind_frame+0x12c/0x180
LR is at get_wchan+0xd4/0x134
pc : [<ffff00000808892c>] lr : [<ffff0000080860b8>] pstate: 60000145
sp : ffff8003cc6c3ab0
x29: ffff8003cc6c3ab0 x28: 0000000000000001
x27: 0000000000000026 x26: 0000000000000026
x25: 00000000000012d8 x24: 0000000000000000
x23: ffff8003c1c04000 x22: ffff000008c83000
x21: ffff8003c1c00000 x20: 000000000000000f
x19: ffff8003c1bc0000 x18: 0000fffffc593690
x17: 0000000000000000 x16: 0000000000000001
x15: 0000b855670e2b60 x14: 0003e97f22cf1d0f
x13: 0000000000000001 x12: 0000000000000000
x11: 00000000e8f4883e x10: 0000000154f47ec8
x9 : 0000000070f367c0 x8 : 0000000000000000
x7 : 00008003f7290000 x6 : 0000000000000018
x5 : 0000000000000000 x4 : ffff8003c1c03cb0
x3 : ffff8003c1c03ca0 x2 : 00000017ffe80000
x1 : ffff8003cc6c3af8 x0 : ffff8003d3e9e000
Process ps (pid: 11696, stack limit = 0xffff8003cc6c0000)
Stack: (0xffff8003cc6c3ab0 to 0xffff8003cc6c4000)
[...]
[<ffff00000808892c>] unwind_frame+0x12c/0x180
[<ffff000008305008>] do_task_stat+0x864/0x870
[<ffff000008305c44>] proc_tgid_stat+0x3c/0x48
[<ffff0000082fde0c>] proc_single_show+0x5c/0xb8
[<ffff0000082b27e0>] seq_read+0x160/0x414
[<ffff000008289e6c>] __vfs_read+0x58/0x164
[<ffff00000828b164>] vfs_read+0x88/0x144
[<ffff00000828c2e8>] SyS_read+0x60/0xc0
[<ffff0000080834a0>] __sys_trace_return+0x0/0x4
fixes: 20380bb390a4 (arm64: ftrace: fix a stack tracer's output under function graph tracer)
Signed-off-by: Pratyush Anand <panand@redhat.com>
Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
arch/arm64/include/asm/stacktrace.h | 2 +-
arch/arm64/kernel/stacktrace.c | 4 ++++
arch/arm64/kernel/time.c | 2 +-
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 6ad30776e984..99390755c0c4 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -27,7 +27,7 @@ struct stackframe {
unsigned long fp;
unsigned long pc;
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- unsigned int graph;
+ int graph;
#endif
};
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 76809ccd309c..5a528c58ef68 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -59,6 +59,10 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
(frame->pc == (unsigned long)return_to_handler)) {
+ WARN_ON(frame->graph == -1);
+ if (frame->graph < -1)
+ frame->graph += FTRACE_NOTRACE_DEPTH;
+
/*
* This is a case where function graph tracer has
* modified a return address (LR) in a stack frame
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index a4391280fba9..f258636273c9 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -52,7 +52,7 @@ unsigned long profile_pc(struct pt_regs *regs)
frame.fp = regs->regs[29];
frame.pc = regs->pc;
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- frame.graph = -1; /* no task info */
+ frame.graph = current->curr_ret_stack;
#endif
do {
int ret = unwind_frame(NULL, &frame);
--
2.13.6
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] arm64: fix unwind_frame() for filtered out fn for function graph tracing
2018-01-12 10:48 [PATCH v3] arm64: fix unwind_frame() for filtered out fn for function graph tracing Jerome Marchand
@ 2018-01-16 17:57 ` Catalin Marinas
2018-01-17 15:03 ` Jerome Marchand
2018-02-05 13:28 ` [PATCH v4] " Jerome Marchand
1 sibling, 1 reply; 4+ messages in thread
From: Catalin Marinas @ 2018-01-16 17:57 UTC (permalink / raw)
To: Jerome Marchand
Cc: linux-arm-kernel, Pratyush Anand, Will Deacon, linux-kernel
On Fri, Jan 12, 2018 at 11:48:32AM +0100, Jerome Marchand wrote:
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 76809ccd309c..5a528c58ef68 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -59,6 +59,10 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> if (tsk->ret_stack &&
> (frame->pc == (unsigned long)return_to_handler)) {
> + WARN_ON(frame->graph == -1);
> + if (frame->graph < -1)
> + frame->graph += FTRACE_NOTRACE_DEPTH;
> +
> /*
> * This is a case where function graph tracer has
> * modified a return address (LR) in a stack frame
So do we still allow this to continue if graph == -1? The following line
doesn't seem safe:
frame->pc = tsk->ret_stack[frame->graph--].ret;
--
Catalin
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] arm64: fix unwind_frame() for filtered out fn for function graph tracing
2018-01-16 17:57 ` Catalin Marinas
@ 2018-01-17 15:03 ` Jerome Marchand
0 siblings, 0 replies; 4+ messages in thread
From: Jerome Marchand @ 2018-01-17 15:03 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arm-kernel, Pratyush Anand, Will Deacon, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1197 bytes --]
On 16/01/18 18:57, Catalin Marinas wrote:
> On Fri, Jan 12, 2018 at 11:48:32AM +0100, Jerome Marchand wrote:
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 76809ccd309c..5a528c58ef68 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -59,6 +59,10 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> if (tsk->ret_stack &&
>> (frame->pc == (unsigned long)return_to_handler)) {
>> + WARN_ON(frame->graph == -1);
>> + if (frame->graph < -1)
>> + frame->graph += FTRACE_NOTRACE_DEPTH;
>> +
>> /*
>> * This is a case where function graph tracer has
>> * modified a return address (LR) in a stack frame
>
> So do we still allow this to continue if graph == -1? The following line
> doesn't seem safe:
>
> frame->pc = tsk->ret_stack[frame->graph--].ret;
>
You're right. We probably should return a error (-EINVAL I guess) if
this happens. Note that this shouldn't happen here and if we're
confident enough that profile_pc() was the only faulty caller, we could
just drop the warning.
Jerome
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v4] arm64: fix unwind_frame() for filtered out fn for function graph tracing
2018-01-12 10:48 [PATCH v3] arm64: fix unwind_frame() for filtered out fn for function graph tracing Jerome Marchand
2018-01-16 17:57 ` Catalin Marinas
@ 2018-02-05 13:28 ` Jerome Marchand
1 sibling, 0 replies; 4+ messages in thread
From: Jerome Marchand @ 2018-02-05 13:28 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Catalin Marinas, Will Deacon, linux-kernel, Pratyush Anand,
Jerome Marchand
From: Pratyush Anand <panand@redhat.com>
Change since v3:
- Exit unwind_frame() if frame->graph != -1
Change since v2:
- Set frame.graph = current->curr_ret_stack in profile_pc
- Check that frame->graph != -1 in unwind_frame
do_task_stat() calls get_wchan(), which further does unwind_frame().
unwind_frame() restores frame->pc to original value in case function
graph tracer has modified a return address (LR) in a stack frame to hook
a function return. However, if function graph tracer has hit a filtered
function, then we can't unwind it as ftrace_push_return_trace() has
biased the index(frame->graph) with a 'huge negative'
offset(-FTRACE_NOTRACE_DEPTH).
Moreover, arm64 stack walker defines index(frame->graph) as unsigned
int, which can not compare a -ve number.
Similar problem we can have with calling of walk_stackframe() from
save_stack_trace_tsk() or dump_backtrace().
This patch fixes unwind_frame() to test the index for -ve value and
restore index accordingly before we can restore frame->pc.
Reproducer:
cd /sys/kernel/debug/tracing/
echo schedule > set_graph_notrace
echo 1 > options/display-graph
echo wakeup > current_tracer
ps -ef | grep -i agent
Above commands result in:
Unable to handle kernel paging request at virtual address ffff801bd3d1e000
pgd = ffff8003cbe97c00
[ffff801bd3d1e000] *pgd=0000000000000000, *pud=0000000000000000
Internal error: Oops: 96000006 [#1] SMP
[...]
CPU: 5 PID: 11696 Comm: ps Not tainted 4.11.0+ #33
[...]
task: ffff8003c21ba000 task.stack: ffff8003cc6c0000
PC is at unwind_frame+0x12c/0x180
LR is at get_wchan+0xd4/0x134
pc : [<ffff00000808892c>] lr : [<ffff0000080860b8>] pstate: 60000145
sp : ffff8003cc6c3ab0
x29: ffff8003cc6c3ab0 x28: 0000000000000001
x27: 0000000000000026 x26: 0000000000000026
x25: 00000000000012d8 x24: 0000000000000000
x23: ffff8003c1c04000 x22: ffff000008c83000
x21: ffff8003c1c00000 x20: 000000000000000f
x19: ffff8003c1bc0000 x18: 0000fffffc593690
x17: 0000000000000000 x16: 0000000000000001
x15: 0000b855670e2b60 x14: 0003e97f22cf1d0f
x13: 0000000000000001 x12: 0000000000000000
x11: 00000000e8f4883e x10: 0000000154f47ec8
x9 : 0000000070f367c0 x8 : 0000000000000000
x7 : 00008003f7290000 x6 : 0000000000000018
x5 : 0000000000000000 x4 : ffff8003c1c03cb0
x3 : ffff8003c1c03ca0 x2 : 00000017ffe80000
x1 : ffff8003cc6c3af8 x0 : ffff8003d3e9e000
Process ps (pid: 11696, stack limit = 0xffff8003cc6c0000)
Stack: (0xffff8003cc6c3ab0 to 0xffff8003cc6c4000)
[...]
[<ffff00000808892c>] unwind_frame+0x12c/0x180
[<ffff000008305008>] do_task_stat+0x864/0x870
[<ffff000008305c44>] proc_tgid_stat+0x3c/0x48
[<ffff0000082fde0c>] proc_single_show+0x5c/0xb8
[<ffff0000082b27e0>] seq_read+0x160/0x414
[<ffff000008289e6c>] __vfs_read+0x58/0x164
[<ffff00000828b164>] vfs_read+0x88/0x144
[<ffff00000828c2e8>] SyS_read+0x60/0xc0
[<ffff0000080834a0>] __sys_trace_return+0x0/0x4
fixes: 20380bb390a4 (arm64: ftrace: fix a stack tracer's output under function graph tracer)
Signed-off-by: Pratyush Anand <panand@redhat.com>
Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
arch/arm64/include/asm/stacktrace.h | 2 +-
arch/arm64/kernel/stacktrace.c | 7 +++++++
arch/arm64/kernel/time.c | 2 +-
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 6ad30776e984..99390755c0c4 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -27,7 +27,7 @@ struct stackframe {
unsigned long fp;
unsigned long pc;
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- unsigned int graph;
+ int graph;
#endif
};
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 76809ccd309c..2c74ff76b0cd 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -59,6 +59,13 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
if (tsk->ret_stack &&
(frame->pc == (unsigned long)return_to_handler)) {
+ if (frame->graph == -1) {
+ WARN_ON(1);
+ return -EINVAL;
+ }
+ if (frame->graph < -1)
+ frame->graph += FTRACE_NOTRACE_DEPTH;
+
/*
* This is a case where function graph tracer has
* modified a return address (LR) in a stack frame
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index a4391280fba9..f258636273c9 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -52,7 +52,7 @@ unsigned long profile_pc(struct pt_regs *regs)
frame.fp = regs->regs[29];
frame.pc = regs->pc;
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- frame.graph = -1; /* no task info */
+ frame.graph = current->curr_ret_stack;
#endif
do {
int ret = unwind_frame(NULL, &frame);
--
2.13.6
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-05 13:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-12 10:48 [PATCH v3] arm64: fix unwind_frame() for filtered out fn for function graph tracing Jerome Marchand
2018-01-16 17:57 ` Catalin Marinas
2018-01-17 15:03 ` Jerome Marchand
2018-02-05 13:28 ` [PATCH v4] " Jerome Marchand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®