* [PATCH v6 0/6] arm64: ftrace: fix incorrect output from stack tracer
@ 2015-11-18 6:43 AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 1/6] arm64: ftrace: modify a stack frame in a safe way AKASHI Takahiro
` (5 more replies)
0 siblings, 6 replies; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-18 6:43 UTC (permalink / raw)
To: linux-arm-kernel
This is the sixth patch series for fixing stack tracer on arm64.
The original issue was reported by Jungseok[1], and then I found more
issues[2].
patch1 is a proactive improvement of function_graph tracer.
patch2 and 3 correspond to II-4(functions under function_graph tracer).
patch4, 5 and 6 correspond to II-1(slurping stack) and II-2(differences
between x86 and arm64).
patch6 is a function prologue analyzer test. This won't attest
the correctness of the functionality, but it can suggest that all
the traced functions are treated properly by this function.
We don't have to care about the original issue because the root cause
(patch "ARM64: unwind: Fix PC calculation") has been reverted in v4.3.
Among the issues in [2], this patchset doesn't address
- II-3(interrupted frame):
Recent discussions[3] about introducing a dedicated interrupt stack
suggests that we can avoid walking through from interrupt stack to
process stack.
(Please note that, even on x86, interrupt stack is not supported by
stack tracer.)
So recent interrupt-stack patch[4] is a prerequisite here.
- II-5(leaf function):
I don't remember why I thought this was a problem, but anyhow "-pg"
seems to disable omit-leaf-stack-frame.
I tested the code with v4.4-rc1 + Jungseok's/James' patch v7[4].
Changes from v5:
- removed a patch ("ftrace: allow arch-specific stack tracer")
which is already in v4.4-rc1
- handle a "return_to_handler" entry in call stack lists in more commonr
way by fixing such entries in unwind_frame(). This will cover all
the cases, a) stack tracer, b) perf call graph and c) dump_backtrace.
(patch 2, 3)
- fixed aarch64_insn_is_eret(). Thanks to Jungseok. (patch 4)
- removed some hunks (offseting AARCH64_INSN_SIZE) due to having reverted
a patch ("ARM64: unwind: Fix PC calculation") (patch 3)
- fixed function prologue analyzer on big-endian kernel. Thanks to Yalin.
(patch 5)
- fixed a stack size of the top function in stack tracer's output
(its size was reported 16 bytes bigger than actual size due to
mishandled ftrace_caller.) (patch 3)
Changes from v4:
- removed a patch("arm64: ftrace: adjust callsite addresses examined
by stack tracer")
- added a function prologue analyzer test(patch 6)
Changes from v3:
- fixed build errors/warnings reported by kbuild test robot
- addressed Steven's comments around check_stack()
- removed a patch ("arm64: ftrace: allow for tracing leaf functions")
I don't remember why I thought this was necessary, but anyhow "-pg" seems
to disable omit-leaf-stack-frame.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/369316.html
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368003.html
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/385337.html
AKASHI Takahiro (6):
arm64: ftrace: modify a stack frame in a safe way
arm64: pass a task parameter to unwind_frame()
arm64: ftrace: fix a stack tracer's output under function graph
tracer
arm64: insn: add instruction decoders for ldp/stp and add/sub
arm64: ftrace: add arch-specific stack tracer
arm64: ftrace: add a test of function prologue analyzer
arch/arm64/include/asm/ftrace.h | 4 +-
arch/arm64/include/asm/insn.h | 18 +++
arch/arm64/include/asm/stacktrace.h | 13 +-
arch/arm64/kernel/ftrace.c | 75 +++++++++-
arch/arm64/kernel/insn.c | 102 +++++++++++++
arch/arm64/kernel/perf_callchain.c | 5 +-
arch/arm64/kernel/process.c | 5 +-
arch/arm64/kernel/return_address.c | 5 +-
arch/arm64/kernel/stacktrace.c | 267 ++++++++++++++++++++++++++++++++++-
arch/arm64/kernel/time.c | 5 +-
arch/arm64/kernel/traps.c | 5 +-
11 files changed, 482 insertions(+), 22 deletions(-)
--
1.7.9.5
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 1/6] arm64: ftrace: modify a stack frame in a safe way
2015-11-18 6:43 [PATCH v6 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
@ 2015-11-18 6:43 ` AKASHI Takahiro
2015-11-24 14:22 ` Jungseok Lee
2015-11-18 6:43 ` [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame() AKASHI Takahiro
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-18 6:43 UTC (permalink / raw)
To: linux-arm-kernel
Function graph tracer modifies a return address (LR) in a stack frame by
calling ftrace_prepare_return() in a traced function's function prologue.
The current code does this modification before preserving an original
address at ftrace_push_return_trace() and there is always a small window
of inconsistency when an interrupt occurs.
This doesn't matter, as far as an interrupt stack is introduced, because
stack tracer won't be invoked in an interrupt context. But it would be
better to proactively minimize such a window by moving the LR modification
after ftrace_push_return_trace().
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
arch/arm64/kernel/ftrace.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index c851be7..314f82d 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -125,23 +125,20 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
* on other archs. It's unlikely on AArch64.
*/
old = *parent;
- *parent = return_hooker;
trace.func = self_addr;
trace.depth = current->curr_ret_stack + 1;
/* Only trace if the calling function expects to */
- if (!ftrace_graph_entry(&trace)) {
- *parent = old;
+ if (!ftrace_graph_entry(&trace))
return;
- }
err = ftrace_push_return_trace(old, self_addr, &trace.depth,
frame_pointer);
- if (err == -EBUSY) {
- *parent = old;
+ if (err == -EBUSY)
return;
- }
+ else
+ *parent = return_hooker;
}
#ifdef CONFIG_DYNAMIC_FTRACE
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame()
2015-11-18 6:43 [PATCH v6 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 1/6] arm64: ftrace: modify a stack frame in a safe way AKASHI Takahiro
@ 2015-11-18 6:43 ` AKASHI Takahiro
2015-11-24 13:42 ` Jungseok Lee
2015-11-18 6:43 ` [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer AKASHI Takahiro
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-18 6:43 UTC (permalink / raw)
To: linux-arm-kernel
The change here doesn't make any difference in this patch, but is
a preparation for later fixing a problem where stacktrace functions,
unwind_frame() and walk_stackframe(), may return useless call stacks
under function graph tracer.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
arch/arm64/include/asm/stacktrace.h | 6 ++++--
arch/arm64/kernel/perf_callchain.c | 2 +-
arch/arm64/kernel/process.c | 2 +-
arch/arm64/kernel/return_address.c | 2 +-
arch/arm64/kernel/stacktrace.c | 8 ++++----
arch/arm64/kernel/time.c | 2 +-
arch/arm64/kernel/traps.c | 2 +-
7 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 7318f6d..6fb61c5 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -16,14 +16,16 @@
#ifndef __ASM_STACKTRACE_H
#define __ASM_STACKTRACE_H
+struct task_struct;
+
struct stackframe {
unsigned long fp;
unsigned long sp;
unsigned long pc;
};
-extern int unwind_frame(struct stackframe *frame);
-extern void walk_stackframe(struct stackframe *frame,
+extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
+extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
int (*fn)(struct stackframe *, void *), void *data);
#endif /* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
index 3aa7483..797220d 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -165,7 +165,7 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
frame.sp = regs->sp;
frame.pc = regs->pc;
- walk_stackframe(&frame, callchain_trace, entry);
+ walk_stackframe(current, &frame, callchain_trace, entry);
}
unsigned long perf_instruction_pointer(struct pt_regs *regs)
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index f75b540..98bf546 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -348,7 +348,7 @@ unsigned long get_wchan(struct task_struct *p)
do {
if (frame.sp < stack_page ||
frame.sp >= stack_page + THREAD_SIZE ||
- unwind_frame(&frame))
+ unwind_frame(p, &frame))
return 0;
if (!in_sched_functions(frame.pc))
return frame.pc;
diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
index 6c4fd28..07b37ac 100644
--- a/arch/arm64/kernel/return_address.c
+++ b/arch/arm64/kernel/return_address.c
@@ -44,7 +44,7 @@ void *return_address(unsigned int level)
frame.sp = current_stack_pointer;
frame.pc = (unsigned long)return_address; /* dummy */
- walk_stackframe(&frame, save_return_addr, &data);
+ walk_stackframe(current, &frame, save_return_addr, &data);
if (!data.level)
return data.addr;
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index a159851..9c7acf8 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -36,7 +36,7 @@
* ldp x29, x30, [sp]
* add sp, sp, #0x10
*/
-int notrace unwind_frame(struct stackframe *frame)
+int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
{
unsigned long high, low;
unsigned long fp = frame->fp;
@@ -78,7 +78,7 @@ int notrace unwind_frame(struct stackframe *frame)
return 0;
}
-void notrace walk_stackframe(struct stackframe *frame,
+void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
int (*fn)(struct stackframe *, void *), void *data)
{
while (1) {
@@ -86,7 +86,7 @@ void notrace walk_stackframe(struct stackframe *frame,
if (fn(frame, data))
break;
- ret = unwind_frame(frame);
+ ret = unwind_frame(tsk, frame);
if (ret < 0)
break;
}
@@ -138,7 +138,7 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
frame.pc = (unsigned long)save_stack_trace_tsk;
}
- walk_stackframe(&frame, save_trace, &data);
+ walk_stackframe(tsk, &frame, save_trace, &data);
if (trace->nr_entries < trace->max_entries)
trace->entries[trace->nr_entries++] = ULONG_MAX;
}
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index 13339b6..6e5c521 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -53,7 +53,7 @@ unsigned long profile_pc(struct pt_regs *regs)
frame.sp = regs->sp;
frame.pc = regs->pc;
do {
- int ret = unwind_frame(&frame);
+ int ret = unwind_frame(NULL, &frame);
if (ret < 0)
return 0;
} while (in_lock_functions(frame.pc));
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index cdfa2f9..da45224 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -177,7 +177,7 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
int ret;
dump_backtrace_entry(where);
- ret = unwind_frame(&frame);
+ ret = unwind_frame(tsk, &frame);
if (ret < 0)
break;
stack = frame.sp;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer
2015-11-18 6:43 [PATCH v6 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 1/6] arm64: ftrace: modify a stack frame in a safe way AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame() AKASHI Takahiro
@ 2015-11-18 6:43 ` AKASHI Takahiro
2015-11-24 13:37 ` Jungseok Lee
2015-11-18 6:43 ` [PATCH v6 4/6] arm64: insn: add instruction decoders for ldp/stp and add/sub AKASHI Takahiro
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-18 6:43 UTC (permalink / raw)
To: linux-arm-kernel
Function graph tracer modifies a return address (LR) in a stack frame
to hook a function return. This will result in many useless entries
(return_to_handler) showing up in
a) a stack tracer's output
b) perf call graph (with perf record -g)
c) dump_backtrace (at panic et al.)
For example, in case of a),
$ echo function_graph > /sys/kernel/debug/tracing/current_tracer
$ echo 1 > /proc/sys/kernel/stack_trace_enabled
$ cat /sys/kernel/debug/tracing/stack_trace
Depth Size Location (54 entries)
----- ---- --------
0) 4504 16 gic_raise_softirq+0x28/0x150
1) 4488 80 smp_cross_call+0x38/0xb8
2) 4408 48 return_to_handler+0x0/0x40
3) 4360 32 return_to_handler+0x0/0x40
...
In case of b),
$ echo function_graph > /sys/kernel/debug/tracing/current_tracer
$ perf record -e mem:XXX:x -ag -- sleep 10
$ perf report
...
| | |--0.22%-- 0x550f8
| | | 0x10888
| | | el0_svc_naked
| | | sys_openat
| | | return_to_handler
| | | return_to_handler
...
In case of c),
$ echo function_graph > /sys/kernel/debug/tracing/current_tracer
$ echo c > /proc/sysrq-trigger
...
Call trace:
[<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
[<ffffffc000092250>] return_to_handler+0x0/0x40
[<ffffffc000092250>] return_to_handler+0x0/0x40
...
This patch replaces such entries with real addresses preserved in
current->ret_stack[] at unwind_frame(). This way, we can cover all
the cases.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
arch/arm64/include/asm/ftrace.h | 2 ++
arch/arm64/include/asm/stacktrace.h | 3 +++
arch/arm64/kernel/perf_callchain.c | 3 +++
arch/arm64/kernel/process.c | 3 +++
arch/arm64/kernel/return_address.c | 3 +++
arch/arm64/kernel/stacktrace.c | 17 +++++++++++++++++
arch/arm64/kernel/time.c | 3 +++
arch/arm64/kernel/traps.c | 3 +++
8 files changed, 37 insertions(+)
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index c5534fa..3c60f37 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -28,6 +28,8 @@ struct dyn_arch_ftrace {
extern unsigned long ftrace_graph_call;
+extern void return_to_handler(void);
+
static inline unsigned long ftrace_call_adjust(unsigned long addr)
{
/*
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 6fb61c5..801a16db 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -22,6 +22,9 @@ struct stackframe {
unsigned long fp;
unsigned long sp;
unsigned long pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ unsigned int graph;
+#endif
};
extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
index 797220d..ff46654 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -164,6 +164,9 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
frame.fp = regs->regs[29];
frame.sp = regs->sp;
frame.pc = regs->pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ frame.graph = current->curr_ret_stack;
+#endif
walk_stackframe(current, &frame, callchain_trace, entry);
}
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 98bf546..88d742b 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -344,6 +344,9 @@ unsigned long get_wchan(struct task_struct *p)
frame.fp = thread_saved_fp(p);
frame.sp = thread_saved_sp(p);
frame.pc = thread_saved_pc(p);
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ frame.graph = p->curr_ret_stack;
+#endif
stack_page = (unsigned long)task_stack_page(p);
do {
if (frame.sp < stack_page ||
diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
index 07b37ac..1718706 100644
--- a/arch/arm64/kernel/return_address.c
+++ b/arch/arm64/kernel/return_address.c
@@ -43,6 +43,9 @@ void *return_address(unsigned int level)
frame.fp = (unsigned long)__builtin_frame_address(0);
frame.sp = current_stack_pointer;
frame.pc = (unsigned long)return_address; /* dummy */
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ frame.graph = current->curr_ret_stack;
+#endif
walk_stackframe(current, &frame, save_return_addr, &data);
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 9c7acf8..0a39049 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -17,6 +17,7 @@
*/
#include <linux/kernel.h>
#include <linux/export.h>
+#include <linux/ftrace.h>
#include <linux/sched.h>
#include <linux/stacktrace.h>
@@ -66,6 +67,19 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
frame->fp = *(unsigned long *)(fp);
frame->pc = *(unsigned long *)(fp + 8);
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ if (tsk && tsk->ret_stack &&
+ (frame->pc == (unsigned long)return_to_handler)) {
+ /*
+ * This is a case where function graph tracer has
+ * modified a return address (LR) in a stack frame
+ * to hook a function return.
+ * So replace it to an original value.
+ */
+ frame->pc = tsk->ret_stack[frame->graph--].ret;
+ }
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
/*
* Check whether we are going to walk through from interrupt stack
* to task stack.
@@ -137,6 +151,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
frame.sp = current_stack_pointer;
frame.pc = (unsigned long)save_stack_trace_tsk;
}
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ frame.graph = tsk->curr_ret_stack;
+#endif
walk_stackframe(tsk, &frame, save_trace, &data);
if (trace->nr_entries < trace->max_entries)
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index 6e5c521..5977969 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -52,6 +52,9 @@ unsigned long profile_pc(struct pt_regs *regs)
frame.fp = regs->regs[29];
frame.sp = regs->sp;
frame.pc = regs->pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ frame.graph = -1; /* no task info */
+#endif
do {
int ret = unwind_frame(NULL, &frame);
if (ret < 0)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index da45224..46053c2 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -169,6 +169,9 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
frame.sp = thread_saved_sp(tsk);
frame.pc = thread_saved_pc(tsk);
}
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ frame.graph = tsk->curr_ret_stack;
+#endif
pr_emerg("Call trace:\n");
while (1) {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 4/6] arm64: insn: add instruction decoders for ldp/stp and add/sub
2015-11-18 6:43 [PATCH v6 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
` (2 preceding siblings ...)
2015-11-18 6:43 ` [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer AKASHI Takahiro
@ 2015-11-18 6:43 ` AKASHI Takahiro
2015-12-08 18:15 ` Will Deacon
2015-11-18 6:43 ` [PATCH v6 5/6] arm64: ftrace: add arch-specific stack tracer AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 6/6] arm64: ftrace: add a test of function prologue analyzer AKASHI Takahiro
5 siblings, 1 reply; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-18 6:43 UTC (permalink / raw)
To: linux-arm-kernel
A function prologue analyzer is a requisite for implementing stack tracer
and getting better views of stack usages on arm64.
To implement a function prologue analyzer, we have to be able to decode,
at least, stp, add, sub and mov instructions.
This patch adds decoders for those instructions, that are used solely
by stack tracer for now, but generic enough for other uses.
Reviewed-by: Jungseok Lee <jungseoklee85@gmail.com>
Tested-by: Jungseok Lee <jungseoklee85@gmail.com>
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
arch/arm64/include/asm/insn.h | 18 ++++++++
arch/arm64/kernel/insn.c | 102 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 120 insertions(+)
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 30e50eb..8d5c538 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -165,6 +165,8 @@ enum aarch64_insn_ldst_type {
AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
+ AARCH64_INSN_LDST_LOAD_PAIR,
+ AARCH64_INSN_LDST_STORE_PAIR,
};
enum aarch64_insn_adsb_type {
@@ -225,6 +227,8 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
__AARCH64_INSN_FUNCS(str_reg, 0x3FE0EC00, 0x38206800)
__AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
+__AARCH64_INSN_FUNCS(stp, 0x7FC00000, 0x29000000)
+__AARCH64_INSN_FUNCS(ldp, 0x7FC00000, 0x29400000)
__AARCH64_INSN_FUNCS(stp_post, 0x7FC00000, 0x28800000)
__AARCH64_INSN_FUNCS(ldp_post, 0x7FC00000, 0x28C00000)
__AARCH64_INSN_FUNCS(stp_pre, 0x7FC00000, 0x29800000)
@@ -277,6 +281,7 @@ __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F)
__AARCH64_INSN_FUNCS(br, 0xFFFFFC1F, 0xD61F0000)
__AARCH64_INSN_FUNCS(blr, 0xFFFFFC1F, 0xD63F0000)
__AARCH64_INSN_FUNCS(ret, 0xFFFFFC1F, 0xD65F0000)
+__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F00E0)
#undef __AARCH64_INSN_FUNCS
@@ -370,6 +375,19 @@ bool aarch32_insn_is_wide(u32 insn);
u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
u32 aarch32_insn_mcr_extract_opc2(u32 insn);
u32 aarch32_insn_mcr_extract_crm(u32 insn);
+int aarch64_insn_decode_add_sub_imm(u32 insn,
+ enum aarch64_insn_register *dst,
+ enum aarch64_insn_register *src,
+ int *imm,
+ enum aarch64_insn_variant *variant,
+ enum aarch64_insn_adsb_type *type);
+int aarch64_insn_decode_load_store_pair(u32 insn,
+ enum aarch64_insn_register *reg1,
+ enum aarch64_insn_register *reg2,
+ enum aarch64_insn_register *base,
+ int *offset,
+ enum aarch64_insn_variant *variant,
+ enum aarch64_insn_ldst_type *type);
#endif /* __ASSEMBLY__ */
#endif /* __ASM_INSN_H */
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index c08b9ad..b56a66c 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -33,6 +33,7 @@
#include <asm/insn.h>
#define AARCH64_INSN_SF_BIT BIT(31)
+#define AARCH64_INSN_S_BIT BIT(29)
#define AARCH64_INSN_N_BIT BIT(22)
static int aarch64_insn_encoding_class[] = {
@@ -1141,3 +1142,104 @@ u32 aarch32_insn_mcr_extract_crm(u32 insn)
{
return insn & CRM_MASK;
}
+
+enum aarch64_insn_register aarch64_insn_extract_reg_num(u32 insn,
+ enum aarch64_insn_register_type type)
+{
+ int shift;
+
+ switch (type) {
+ case AARCH64_INSN_REGTYPE_RT:
+ case AARCH64_INSN_REGTYPE_RD:
+ shift = 0;
+ break;
+ case AARCH64_INSN_REGTYPE_RN:
+ shift = 5;
+ break;
+ case AARCH64_INSN_REGTYPE_RT2:
+ case AARCH64_INSN_REGTYPE_RA:
+ shift = 10;
+ break;
+ case AARCH64_INSN_REGTYPE_RM:
+ shift = 16;
+ break;
+ default:
+ pr_err("%s: unknown register type decoding %d\n", __func__,
+ type);
+ return ~0L;
+ }
+
+ return (insn & (GENMASK(4, 0) << shift)) >> shift;
+}
+
+int aarch64_insn_decode_add_sub_imm(u32 insn,
+ enum aarch64_insn_register *dst,
+ enum aarch64_insn_register *src,
+ int *imm,
+ enum aarch64_insn_variant *variant,
+ enum aarch64_insn_adsb_type *type)
+{
+ if (aarch64_insn_is_add_imm(insn))
+ *type = ((insn) & AARCH64_INSN_S_BIT) ?
+ AARCH64_INSN_ADSB_ADD_SETFLAGS :
+ AARCH64_INSN_ADSB_ADD;
+ else if (aarch64_insn_is_sub_imm(insn))
+ *type = ((insn) & AARCH64_INSN_S_BIT) ?
+ AARCH64_INSN_ADSB_SUB_SETFLAGS :
+ AARCH64_INSN_ADSB_SUB;
+ else
+ return 0;
+
+ *variant = (insn & AARCH64_INSN_SF_BIT) ? AARCH64_INSN_VARIANT_64BIT :
+ AARCH64_INSN_VARIANT_32BIT;
+
+ *dst = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RD);
+
+ *src = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
+
+ /* TODO: ignore shilft field[23:22] */
+ *imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_12, insn);
+
+ return 1;
+}
+
+int aarch64_insn_decode_load_store_pair(u32 insn,
+ enum aarch64_insn_register *reg1,
+ enum aarch64_insn_register *reg2,
+ enum aarch64_insn_register *base,
+ int *offset,
+ enum aarch64_insn_variant *variant,
+ enum aarch64_insn_ldst_type *type)
+{
+ int imm;
+
+ if (aarch64_insn_is_stp(insn))
+ *type = AARCH64_INSN_LDST_STORE_PAIR;
+ else if (aarch64_insn_is_stp_post(insn))
+ *type = AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX;
+ else if (aarch64_insn_is_stp_pre(insn))
+ *type = AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX;
+ else if (aarch64_insn_is_ldp(insn))
+ *type = AARCH64_INSN_LDST_LOAD_PAIR;
+ else if (aarch64_insn_is_ldp_post(insn))
+ *type = AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX;
+ else if (aarch64_insn_is_ldp_pre(insn))
+ *type = AARCH64_INSN_LDST_LOAD_PAIR_PRE_INDEX;
+ else
+ return 0;
+
+ *variant = (insn & AARCH64_INSN_S_BIT) ? AARCH64_INSN_VARIANT_64BIT :
+ AARCH64_INSN_VARIANT_32BIT;
+
+ *reg1 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT);
+
+ *reg2 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT2);
+
+ *base = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
+
+ imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_7, insn);
+ asm("sbfm %0, %0, 0, 6" : "+r" (imm));
+ *offset = imm * 8;
+
+ return 1;
+}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 5/6] arm64: ftrace: add arch-specific stack tracer
2015-11-18 6:43 [PATCH v6 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
` (3 preceding siblings ...)
2015-11-18 6:43 ` [PATCH v6 4/6] arm64: insn: add instruction decoders for ldp/stp and add/sub AKASHI Takahiro
@ 2015-11-18 6:43 ` AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 6/6] arm64: ftrace: add a test of function prologue analyzer AKASHI Takahiro
5 siblings, 0 replies; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-18 6:43 UTC (permalink / raw)
To: linux-arm-kernel
Background and issues on generic check_stack():
1) slurping stack
Assume that a given function A was invoked, and it was invoked again in
another context, then it called another function B which allocated
a large size of local variables on the stack, but it has not modified
the variable(s) yet.
When stack tracer, check_stack(), examines the stack looking for B,
then A, we may have a chance to accidentally find a stale, not current,
stack frame for A because the old frame might reside on the memory for
the variable which has not been overwritten.
(issue) The stack_trace output may have stale entries.
2) differences between x86 and arm64
On x86, "call" instruction automatically pushes a return address on
the top of the stack and decrements a stack pointer. Then child
function allocates its local variables on the stack.
On arm64, a child function is responsible for allocating memory for
local variables as well as a stack frame, and explicitly pushes
a return address (LR) and old frame pointer in its function prologue
*after* decreasing a stack pointer.
Generic check_stack() recogizes 'idxB,' which is the next address of
the location where 'fpB' is found, in the picture below as an estimated
stack pointer. This seems to fine with x86, but on arm64, 'idxB' is
not appropriate just because it contains child function's "local
variables."
We should instead use spB, if possible, for better interpretation of
func_B's stack usage.
LOW | ... |
fpA +--------+ func_A (pcA, fpA, spA)
| fpB |
idxB + - - - -+
| pcB |
| ... <----------- static local variables in func_A
| ... | and extra function args to func_A
spB + - - - -+
| ... <----------- dynamically allocated variables in func_B
fpB +--------+ func_B (pcB, fpB, spB)
| fpC |
idxC + - - - -+
| pcC |
| ... <----------- static local variables in func_B
| ... | and extra function args to func_B
spC + - - - -+
| ... |
fpC +--------+ func_C (pcC, fpC, spC)
HIGH | |
(issue) Stack size for a function in stack_trace output is inaccurate,
or rather wrong. It looks as if <Size> field is one-line
offset against <Location>.
Depth Size Location (49 entries)
----- ---- --------
40) 1416 64 path_openat+0x128/0xe00 -> 176
41) 1352 176 do_filp_open+0x74/0xf0 -> 256
42) 1176 256 do_open_execat+0x74/0x1c8 -> 80
43) 920 80 open_exec+0x3c/0x70 -> 32
44) 840 32 load_elf_binary+0x294/0x10c8
Stack tracer on arm64:
Our approach in check_stack() is uniqeue in the following points:
* analyze a function prologue of a traced function to estimate a more
accurate stack pointer value, replacing naive '<child's fp> + 0x10.'
* use walk_stackframe(), instead of slurping stack contents as orignal
check_stack() does, to identify a stack frame and a stack index (height)
for every callsite.
Regarding a function prologue analyzer, there is no guarantee that we can
handle all the possible patterns of function prologue as gcc does not use
any fixed templates to generate them. 'Instruction scheduling' is another
issue here.
Nevertheless, the current version will surely cover almost all the cases
in the kernel image and give us useful information on stack pointers.
So this patch utilizes a function prologue only for stack tracer, and
does not affect the behaviors of existing stacktrace functions.
Reviewed-by: Jungseok Lee <jungseoklee85@gmail.com>
Tested-by: Jungseok Lee <jungseoklee85@gmail.com>
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
arch/arm64/include/asm/ftrace.h | 2 +-
arch/arm64/include/asm/stacktrace.h | 4 +
arch/arm64/kernel/ftrace.c | 64 ++++++++++++
arch/arm64/kernel/stacktrace.c | 190 ++++++++++++++++++++++++++++++++++-
4 files changed, 256 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 3c60f37..6795219 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -26,7 +26,7 @@ struct dyn_arch_ftrace {
/* No extra data needed for arm64 */
};
-extern unsigned long ftrace_graph_call;
+extern u32 ftrace_graph_call;
extern void return_to_handler(void);
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 801a16db..0eee008 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -30,5 +30,9 @@ struct stackframe {
extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
int (*fn)(struct stackframe *, void *), void *data);
+#ifdef CONFIG_STACK_TRACER
+struct stack_trace;
+extern void save_stack_trace_sp(struct stack_trace *trace, unsigned long *sp);
+#endif
#endif /* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 314f82d..102ed59 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -9,6 +9,7 @@
* published by the Free Software Foundation.
*/
+#include <linux/bug.h>
#include <linux/ftrace.h>
#include <linux/swab.h>
#include <linux/uaccess.h>
@@ -16,6 +17,7 @@
#include <asm/cacheflush.h>
#include <asm/ftrace.h>
#include <asm/insn.h>
+#include <asm/stacktrace.h>
#ifdef CONFIG_DYNAMIC_FTRACE
/*
@@ -173,3 +175,65 @@ int ftrace_disable_ftrace_graph_caller(void)
}
#endif /* CONFIG_DYNAMIC_FTRACE */
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
+#ifdef CONFIG_STACK_TRACER
+static unsigned long stack_trace_sp[STACK_TRACE_ENTRIES];
+static unsigned long raw_stack_trace_max_size;
+
+void check_stack(unsigned long ip, unsigned long *stack)
+{
+ unsigned long this_size, flags;
+ unsigned long top;
+ int i, j;
+
+ this_size = ((unsigned long)stack) & (THREAD_SIZE-1);
+ this_size = THREAD_SIZE - this_size;
+
+ if (this_size <= raw_stack_trace_max_size)
+ return;
+
+ /* we do not handle an interrupt stack yet */
+ if (!object_is_on_stack(stack))
+ return;
+
+ local_irq_save(flags);
+ arch_spin_lock(&stack_trace_max_lock);
+
+ /* check again */
+ if (this_size <= raw_stack_trace_max_size)
+ goto out;
+
+ /* find out stack frames */
+ stack_trace_max.nr_entries = 0;
+ stack_trace_max.skip = 0;
+ save_stack_trace_sp(&stack_trace_max, stack_trace_sp);
+ stack_trace_max.nr_entries--; /* for the last entry ('-1') */
+
+ /* calculate a stack index for each function */
+ top = ((unsigned long)stack & ~(THREAD_SIZE-1)) + THREAD_SIZE;
+ for (i = 0; i < stack_trace_max.nr_entries; i++)
+ stack_trace_index[i] = top - stack_trace_sp[i];
+ raw_stack_trace_max_size = this_size;
+
+ /* Skip over the overhead of the stack tracer itself */
+ for (i = 0; i < stack_trace_max.nr_entries; i++)
+ if (stack_trace_max.entries[i] == ip)
+ break;
+
+ stack_trace_max.nr_entries -= i;
+ for (j = 0; j < stack_trace_max.nr_entries; j++) {
+ stack_trace_index[j] = stack_trace_index[j + i];
+ stack_trace_max.entries[j] = stack_trace_max.entries[j + i];
+ }
+ stack_trace_max_size = stack_trace_index[0];
+
+ if (task_stack_end_corrupted(current)) {
+ WARN(1, "task stack is corrupted.\n");
+ stack_trace_print();
+ }
+
+ out:
+ arch_spin_unlock(&stack_trace_max_lock);
+ local_irq_restore(flags);
+}
+#endif /* CONFIG_STACK_TRACER */
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 0a39049..1d18bc4 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -24,6 +24,149 @@
#include <asm/irq.h>
#include <asm/stacktrace.h>
+#ifdef CONFIG_STACK_TRACER
+/*
+ * This function parses a function prologue of a traced function and
+ * determines its stack size.
+ * A return value indicates a location of @pc in a function prologue.
+ * @return value:
+ * <case 1> <case 1'>
+ * 1:
+ * sub sp, sp, #XX sub sp, sp, #XX
+ * 2:
+ * stp x29, x30, [sp, #YY] stp x29, x30, [sp, #--ZZ]!
+ * 3:
+ * add x29, sp, #YY mov x29, sp
+ * 0:
+ *
+ * <case 2>
+ * 1:
+ * stp x29, x30, [sp, #-XX]!
+ * 3:
+ * mov x29, sp
+ * 0:
+ *
+ * @size: sp offset from calller's sp (XX or XX + ZZ)
+ * @size2: fp offset from new sp (YY or 0)
+ */
+static int analyze_function_prologue(unsigned long pc,
+ unsigned long *size, unsigned long *size2)
+{
+ unsigned long offset;
+ u32 *addr, insn;
+ int pos = -1;
+ enum aarch64_insn_register src, dst, reg1, reg2, base;
+ int imm;
+ enum aarch64_insn_variant variant;
+ enum aarch64_insn_adsb_type adsb_type;
+ enum aarch64_insn_ldst_type ldst_type;
+
+ *size = *size2 = 0;
+
+ if (!pc)
+ goto out;
+
+ if (unlikely(!kallsyms_lookup_size_offset(pc, NULL, &offset)))
+ goto out;
+
+ addr = (u32 *)(pc - offset);
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ if (addr == (u32 *)ftrace_graph_caller)
+#ifdef CONFIG_DYNAMIC_FTRACE
+ addr = (u32 *)ftrace_caller;
+#else
+ addr = (u32 *)_mcount;
+#endif
+ else
+#endif
+#ifdef CONFIG_DYNAMIC_FTRACE
+ if (addr == (u32 *)ftrace_call)
+ addr = (u32 *)ftrace_caller;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+ else if (addr == &ftrace_graph_call)
+ addr = (u32 *)ftrace_caller;
+#endif
+#endif
+
+ insn = le32_to_cpu(*addr);
+ pos = 1;
+
+ /* analyze a function prologue */
+ while ((unsigned long)addr < pc) {
+ if (aarch64_insn_is_branch_imm(insn) ||
+ aarch64_insn_is_br(insn) ||
+ aarch64_insn_is_blr(insn) ||
+ aarch64_insn_is_ret(insn) ||
+ aarch64_insn_is_eret(insn))
+ /* exiting a basic block */
+ goto out;
+
+ if (aarch64_insn_decode_add_sub_imm(insn, &dst, &src,
+ &imm, &variant, &adsb_type)) {
+ if ((adsb_type == AARCH64_INSN_ADSB_SUB) &&
+ (dst == AARCH64_INSN_REG_SP) &&
+ (src == AARCH64_INSN_REG_SP)) {
+ /*
+ * Starting the following sequence:
+ * sub sp, sp, #xx
+ * stp x29, x30, [sp, #yy]
+ * add x29, sp, #yy
+ */
+ WARN_ON(pos != 1);
+ pos = 2;
+ *size += imm;
+ } else if ((adsb_type == AARCH64_INSN_ADSB_ADD) &&
+ (dst == AARCH64_INSN_REG_29) &&
+ (src == AARCH64_INSN_REG_SP)) {
+ /*
+ * add x29, sp, #yy
+ * or
+ * mov x29, sp
+ */
+ WARN_ON(pos != 3);
+ pos = 0;
+ *size2 = imm;
+
+ break;
+ }
+ } else if (aarch64_insn_decode_load_store_pair(insn,
+ ®1, ®2, &base, &imm,
+ &variant, &ldst_type)) {
+ if ((ldst_type ==
+ AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX) &&
+ (reg1 == AARCH64_INSN_REG_29) &&
+ (reg2 == AARCH64_INSN_REG_30) &&
+ (base == AARCH64_INSN_REG_SP)) {
+ /*
+ * Starting the following sequence:
+ * stp x29, x30, [sp, #-xx]!
+ * mov x29, sp
+ */
+ WARN_ON(!((pos == 1) || (pos == 2)));
+ pos = 3;
+ *size += -imm;
+ } else if ((ldst_type ==
+ AARCH64_INSN_LDST_STORE_PAIR) &&
+ (reg1 == AARCH64_INSN_REG_29) &&
+ (reg2 == AARCH64_INSN_REG_30) &&
+ (base == AARCH64_INSN_REG_SP)) {
+ /*
+ * stp x29, x30, [sp, #yy]
+ */
+ WARN_ON(pos != 2);
+ pos = 3;
+ }
+ }
+
+ addr++;
+ insn = le32_to_cpu(*addr);
+ }
+
+out:
+ return pos;
+}
+#endif
+
/*
* AArch64 PCS assigns the frame pointer to x29.
*
@@ -112,6 +255,9 @@ struct stack_trace_data {
struct stack_trace *trace;
unsigned int no_sched_functions;
unsigned int skip;
+#ifdef CONFIG_STACK_TRACER
+ unsigned long *sp;
+#endif
};
static int save_trace(struct stackframe *frame, void *d)
@@ -127,18 +273,42 @@ static int save_trace(struct stackframe *frame, void *d)
return 0;
}
+#ifdef CONFIG_STACK_TRACER
+ if (data->sp) {
+ if (trace->nr_entries) {
+ unsigned long child_pc, sp_off, fp_off;
+ int pos;
+
+ child_pc = trace->entries[trace->nr_entries - 1];
+ pos = analyze_function_prologue(child_pc,
+ &sp_off, &fp_off);
+ /*
+ * frame->sp - 0x10 is actually a child's fp.
+ * See above.
+ */
+ data->sp[trace->nr_entries] = (pos < 0 ? frame->sp :
+ (frame->sp - 0x10) + sp_off - fp_off);
+ } else {
+ data->sp[0] = frame->sp;
+ }
+ }
+#endif
trace->entries[trace->nr_entries++] = addr;
return trace->nr_entries >= trace->max_entries;
}
-void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
+static void __save_stack_trace_tsk(struct task_struct *tsk,
+ struct stack_trace *trace, unsigned long *stack_dump_sp)
{
struct stack_trace_data data;
struct stackframe frame;
data.trace = trace;
data.skip = trace->skip;
+#ifdef CONFIG_STACK_TRACER
+ data.sp = stack_dump_sp;
+#endif
if (tsk != current) {
data.no_sched_functions = 1;
@@ -149,7 +319,8 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
data.no_sched_functions = 0;
frame.fp = (unsigned long)__builtin_frame_address(0);
frame.sp = current_stack_pointer;
- frame.pc = (unsigned long)save_stack_trace_tsk;
+ asm("1:");
+ asm("ldr %0, =1b" : "=r" (frame.pc));
}
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
frame.graph = tsk->curr_ret_stack;
@@ -160,9 +331,22 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
trace->entries[trace->nr_entries++] = ULONG_MAX;
}
+void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
+{
+ __save_stack_trace_tsk(tsk, trace, NULL);
+}
+
void save_stack_trace(struct stack_trace *trace)
{
- save_stack_trace_tsk(current, trace);
+ __save_stack_trace_tsk(current, trace, NULL);
}
EXPORT_SYMBOL_GPL(save_stack_trace);
+
+#ifdef CONFIG_STACK_TRACER
+void save_stack_trace_sp(struct stack_trace *trace,
+ unsigned long *stack_dump_sp)
+{
+ __save_stack_trace_tsk(current, trace, stack_dump_sp);
+}
+#endif /* CONFIG_STACK_TRACER */
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 6/6] arm64: ftrace: add a test of function prologue analyzer
2015-11-18 6:43 [PATCH v6 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
` (4 preceding siblings ...)
2015-11-18 6:43 ` [PATCH v6 5/6] arm64: ftrace: add arch-specific stack tracer AKASHI Takahiro
@ 2015-11-18 6:43 ` AKASHI Takahiro
2015-11-24 13:50 ` Jungseok Lee
5 siblings, 1 reply; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-18 6:43 UTC (permalink / raw)
To: linux-arm-kernel
The patch ("arm64: ftrace: add arch-specific stack tracer") introduced
a function prologue analyzer.
Given that there is no fixed template for a function prologue, at least
on gcc for aarch64, a function prologue analyzer may be rather heuristic.
So this patch adds a kernel command line option,
function_prologue_analyzer_test, in order to run a basic test at startup
by executing an analyzer against all the *traceable* functions.
For function_prologue_analyzer_test=2, the output looks like:
po sp fp symbol
== == == ======
0: 0 0x040 0x000 gic_handle_irq+0x20/0xa4
1: 0 0x040 0x000 gic_handle_irq+0x34/0x114
2: 0 0x030 0x000 run_init_process+0x14/0x48
3: 0 0x020 0x000 try_to_run_init_process+0x14/0x58
4: 0 0x080 0x000 do_one_initcall+0x1c/0x194
...
22959: 0 0x020 0x000 tty_lock_slave+0x14/0x3c
22960: 0 0x020 0x000 tty_unlock_slave+0x14/0x3c
function prologue analyzer test: 0 errors
"po" indicates a position of callsite of mcount(), and should be zero
if an analyzer has parsed a function prologue successfully and reached
a location where fp is properly updated.
"sp" is a final offset to a parent's fp at the exit of function prologue.
"fp" is also an ofset against sp at the exit of function prologue.
So normally,
<new sp> = <old fp> + <"sp">
<new fp> = <new sp> - <"fp">
And the last line shows the number of possible errors in the result.
For function_prologue_analyzer_test=1, only the last line will be shown.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
arch/arm64/include/asm/insn.h | 2 +-
arch/arm64/kernel/stacktrace.c | 52 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 8d5c538..1dfae16 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -281,7 +281,7 @@ __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F)
__AARCH64_INSN_FUNCS(br, 0xFFFFFC1F, 0xD61F0000)
__AARCH64_INSN_FUNCS(blr, 0xFFFFFC1F, 0xD63F0000)
__AARCH64_INSN_FUNCS(ret, 0xFFFFFC1F, 0xD65F0000)
-__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F00E0)
+__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F03E0)
#undef __AARCH64_INSN_FUNCS
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 1d18bc4..19dad62 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -18,6 +18,7 @@
#include <linux/kernel.h>
#include <linux/export.h>
#include <linux/ftrace.h>
+#include <linux/kallsyms.h>
#include <linux/sched.h>
#include <linux/stacktrace.h>
@@ -348,5 +349,56 @@ void save_stack_trace_sp(struct stack_trace *trace,
{
__save_stack_trace_tsk(current, trace, stack_dump_sp);
}
+
+static int start_analyzer_test __initdata;
+
+static int __init enable_analyzer_test(char *str)
+{
+ get_option(&str, &start_analyzer_test);
+ return 0;
+}
+early_param("function_prologue_analyzer_test", enable_analyzer_test);
+
+static void __init do_test_function_prologue_analyzer(void)
+{
+ extern unsigned long __start_mcount_loc[];
+ extern unsigned long __stop_mcount_loc[];
+ unsigned long count, i, errors;
+ int print_once;
+
+ count = __stop_mcount_loc - __start_mcount_loc;
+ errors = print_once = 0;
+ for (i = 0; i < count; i++) {
+ unsigned long addr, sp_off, fp_off;
+ int pos;
+ bool check;
+ char buf[60];
+
+ addr = __start_mcount_loc[i];
+ pos = analyze_function_prologue(addr, &sp_off, &fp_off);
+ check = ((pos != 0) || !sp_off || (sp_off <= fp_off));
+ if (check)
+ errors++;
+ if (check || (start_analyzer_test > 1)) {
+ if (!print_once) {
+ pr_debug(" po sp fp symbol\n");
+ pr_debug(" == == == ======\n");
+ print_once++;
+ }
+ sprint_symbol(buf, addr);
+ pr_debug("%5ld: %d 0x%03lx 0x%03lx %s\n",
+ i, pos, sp_off, fp_off, buf);
+ }
+ }
+ pr_debug("function prologue analyzer test: %ld errors\n", errors);
+}
+
+static int __init test_function_prologue_analyzer(void)
+{
+ if (start_analyzer_test)
+ do_test_function_prologue_analyzer();
+ return 0;
+}
+late_initcall(test_function_prologue_analyzer);
#endif /* CONFIG_STACK_TRACER */
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer
2015-11-18 6:43 ` [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer AKASHI Takahiro
@ 2015-11-24 13:37 ` Jungseok Lee
2015-11-25 5:29 ` AKASHI Takahiro
0 siblings, 1 reply; 23+ messages in thread
From: Jungseok Lee @ 2015-11-24 13:37 UTC (permalink / raw)
To: linux-arm-kernel
On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
Hi Akashi,
> Function graph tracer modifies a return address (LR) in a stack frame
> to hook a function return. This will result in many useless entries
> (return_to_handler) showing up in
> a) a stack tracer's output
> b) perf call graph (with perf record -g)
> c) dump_backtrace (at panic et al.)
>
> For example, in case of a),
> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
> $ echo 1 > /proc/sys/kernel/stack_trace_enabled
> $ cat /sys/kernel/debug/tracing/stack_trace
> Depth Size Location (54 entries)
> ----- ---- --------
> 0) 4504 16 gic_raise_softirq+0x28/0x150
> 1) 4488 80 smp_cross_call+0x38/0xb8
> 2) 4408 48 return_to_handler+0x0/0x40
> 3) 4360 32 return_to_handler+0x0/0x40
> ...
>
> In case of b),
> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
> $ perf record -e mem:XXX:x -ag -- sleep 10
> $ perf report
> ...
> | | |--0.22%-- 0x550f8
> | | | 0x10888
> | | | el0_svc_naked
> | | | sys_openat
> | | | return_to_handler
> | | | return_to_handler
> ...
>
> In case of c),
> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
> $ echo c > /proc/sysrq-trigger
> ...
> Call trace:
> [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
> [<ffffffc000092250>] return_to_handler+0x0/0x40
> [<ffffffc000092250>] return_to_handler+0x0/0x40
> ...
>
> This patch replaces such entries with real addresses preserved in
> current->ret_stack[] at unwind_frame(). This way, we can cover all
> the cases.
I've observed a strange behavior when playing with case c). Call trace
is as follows when function_graph is not used.
Call trace:
[<fffffe00003dc738>] sysrq_handle_crash+0x24/0x30 <- (1)
[<fffffe00003dd2ac>] __handle_sysrq+0x128/0x19c <- (2)
[<fffffe00003dd730>] write_sysrq_trigger+0x60/0x74
[<fffffe0000249fc4>] proc_reg_write+0x84/0xc0
[<fffffe00001f2638>] __vfs_write+0x44/0x104 <- (3)
[<fffffe00001f2e60>] vfs_write+0x98/0x1a8
[<fffffe00001f3730>] SyS_write+0x50/0xb0
[<fffffe00000939ec>] el0_svc_naked+0x20/0x28 <- (4)
When function_graph is set, some entries, such as do_mem_abort, are added
between (1) and (2). In addition, entries from (3) to (4) are not printed
out. As tracking down elements of ret_stack[], I realize dump_backtrace()
has been terminated before reaching to ret_stack[0]. Have you seen this
kind of behavior? I believe push & pop operations work correctly.
Please correct me if I'm wrong.
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/include/asm/ftrace.h | 2 ++
> arch/arm64/include/asm/stacktrace.h | 3 +++
> arch/arm64/kernel/perf_callchain.c | 3 +++
> arch/arm64/kernel/process.c | 3 +++
> arch/arm64/kernel/return_address.c | 3 +++
> arch/arm64/kernel/stacktrace.c | 17 +++++++++++++++++
> arch/arm64/kernel/time.c | 3 +++
> arch/arm64/kernel/traps.c | 3 +++
> 8 files changed, 37 insertions(+)
>
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index c5534fa..3c60f37 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -28,6 +28,8 @@ struct dyn_arch_ftrace {
>
> extern unsigned long ftrace_graph_call;
>
> +extern void return_to_handler(void);
> +
> static inline unsigned long ftrace_call_adjust(unsigned long addr)
> {
> /*
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 6fb61c5..801a16db 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -22,6 +22,9 @@ struct stackframe {
> unsigned long fp;
> unsigned long sp;
> unsigned long pc;
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> + unsigned int graph;
> +#endif
> };
How about using int instead of unsigned int to align with cure_ret_stack
of struct task_struct?
> extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
> index 797220d..ff46654 100644
> --- a/arch/arm64/kernel/perf_callchain.c
> +++ b/arch/arm64/kernel/perf_callchain.c
> @@ -164,6 +164,9 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
> frame.fp = regs->regs[29];
> frame.sp = regs->sp;
> frame.pc = regs->pc;
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> + frame.graph = current->curr_ret_stack;
> +#endif
>
> walk_stackframe(current, &frame, callchain_trace, entry);
> }
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 98bf546..88d742b 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -344,6 +344,9 @@ unsigned long get_wchan(struct task_struct *p)
> frame.fp = thread_saved_fp(p);
> frame.sp = thread_saved_sp(p);
> frame.pc = thread_saved_pc(p);
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> + frame.graph = p->curr_ret_stack;
> +#endif
> stack_page = (unsigned long)task_stack_page(p);
> do {
> if (frame.sp < stack_page ||
> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
> index 07b37ac..1718706 100644
> --- a/arch/arm64/kernel/return_address.c
> +++ b/arch/arm64/kernel/return_address.c
> @@ -43,6 +43,9 @@ void *return_address(unsigned int level)
> frame.fp = (unsigned long)__builtin_frame_address(0);
> frame.sp = current_stack_pointer;
> frame.pc = (unsigned long)return_address; /* dummy */
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> + frame.graph = current->curr_ret_stack;
> +#endif
>
> walk_stackframe(current, &frame, save_return_addr, &data);
>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 9c7acf8..0a39049 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -17,6 +17,7 @@
> */
> #include <linux/kernel.h>
> #include <linux/export.h>
> +#include <linux/ftrace.h>
> #include <linux/sched.h>
> #include <linux/stacktrace.h>
>
> @@ -66,6 +67,19 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> frame->fp = *(unsigned long *)(fp);
> frame->pc = *(unsigned long *)(fp + 8);
>
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> + if (tsk && tsk->ret_stack &&
> + (frame->pc == (unsigned long)return_to_handler)) {
> + /*
> + * This is a case where function graph tracer has
> + * modified a return address (LR) in a stack frame
> + * to hook a function return.
> + * So replace it to an original value.
> + */
> + frame->pc = tsk->ret_stack[frame->graph--].ret;
> + }
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
There is an index check of ret_stack[] in case of x86 [1]. Even though
graph is unsigned int, I think we need to check the value of frame->graph
before accessing ret_stack[].
> +
> /*
> * Check whether we are going to walk through from interrupt stack
> * to task stack.
> @@ -137,6 +151,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
> frame.sp = current_stack_pointer;
> frame.pc = (unsigned long)save_stack_trace_tsk;
> }
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> + frame.graph = tsk->curr_ret_stack;
> +#endif
>
> walk_stackframe(tsk, &frame, save_trace, &data);
> if (trace->nr_entries < trace->max_entries)
> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
> index 6e5c521..5977969 100644
> --- a/arch/arm64/kernel/time.c
> +++ b/arch/arm64/kernel/time.c
> @@ -52,6 +52,9 @@ unsigned long profile_pc(struct pt_regs *regs)
> frame.fp = regs->regs[29];
> frame.sp = regs->sp;
> frame.pc = regs->pc;
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> + frame.graph = -1; /* no task info */
> +#endif
graph is unsigned int type. Is this intentional?
Best Regards
Jungseok Lee
[1] arch/x86/kernel/dumpstack.c
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame()
2015-11-18 6:43 ` [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame() AKASHI Takahiro
@ 2015-11-24 13:42 ` Jungseok Lee
2015-11-25 4:39 ` AKASHI Takahiro
0 siblings, 1 reply; 23+ messages in thread
From: Jungseok Lee @ 2015-11-24 13:42 UTC (permalink / raw)
To: linux-arm-kernel
On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
> The change here doesn't make any difference in this patch, but is
> a preparation for later fixing a problem where stacktrace functions,
> unwind_frame() and walk_stackframe(), may return useless call stacks
> under function graph tracer.
I'm aligned with the argument. The case cannot be handled correctly
without ret_stack[] of struct task_struct.
Best Regards
Jungseok Lee
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 6/6] arm64: ftrace: add a test of function prologue analyzer
2015-11-18 6:43 ` [PATCH v6 6/6] arm64: ftrace: add a test of function prologue analyzer AKASHI Takahiro
@ 2015-11-24 13:50 ` Jungseok Lee
2015-11-25 5:33 ` AKASHI Takahiro
0 siblings, 1 reply; 23+ messages in thread
From: Jungseok Lee @ 2015-11-24 13:50 UTC (permalink / raw)
To: linux-arm-kernel
On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
> The patch ("arm64: ftrace: add arch-specific stack tracer") introduced
> a function prologue analyzer.
>
> Given that there is no fixed template for a function prologue, at least
> on gcc for aarch64, a function prologue analyzer may be rather heuristic.
> So this patch adds a kernel command line option,
> function_prologue_analyzer_test, in order to run a basic test at startup
> by executing an analyzer against all the *traceable* functions.
>
> For function_prologue_analyzer_test=2, the output looks like:
>
> po sp fp symbol
> == == == ======
> 0: 0 0x040 0x000 gic_handle_irq+0x20/0xa4
> 1: 0 0x040 0x000 gic_handle_irq+0x34/0x114
> 2: 0 0x030 0x000 run_init_process+0x14/0x48
> 3: 0 0x020 0x000 try_to_run_init_process+0x14/0x58
> 4: 0 0x080 0x000 do_one_initcall+0x1c/0x194
> ...
> 22959: 0 0x020 0x000 tty_lock_slave+0x14/0x3c
> 22960: 0 0x020 0x000 tty_unlock_slave+0x14/0x3c
> function prologue analyzer test: 0 errors
>
> "po" indicates a position of callsite of mcount(), and should be zero
> if an analyzer has parsed a function prologue successfully and reached
> a location where fp is properly updated.
> "sp" is a final offset to a parent's fp at the exit of function prologue.
> "fp" is also an ofset against sp at the exit of function prologue.
> So normally,
> <new sp> = <old fp> + <"sp">
> <new fp> = <new sp> - <"fp">
>
> And the last line shows the number of possible errors in the result.
>
> For function_prologue_analyzer_test=1, only the last line will be shown.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/include/asm/insn.h | 2 +-
> arch/arm64/kernel/stacktrace.c | 52 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 8d5c538..1dfae16 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -281,7 +281,7 @@ __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F)
> __AARCH64_INSN_FUNCS(br, 0xFFFFFC1F, 0xD61F0000)
> __AARCH64_INSN_FUNCS(blr, 0xFFFFFC1F, 0xD63F0000)
> __AARCH64_INSN_FUNCS(ret, 0xFFFFFC1F, 0xD65F0000)
> -__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F00E0)
> +__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F03E0)
How about folding(squashing) this hunk into [PATCH 4/6]?
> #undef __AARCH64_INSN_FUNCS
>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 1d18bc4..19dad62 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -18,6 +18,7 @@
> #include <linux/kernel.h>
> #include <linux/export.h>
> #include <linux/ftrace.h>
> +#include <linux/kallsyms.h>
> #include <linux/sched.h>
> #include <linux/stacktrace.h>
>
> @@ -348,5 +349,56 @@ void save_stack_trace_sp(struct stack_trace *trace,
> {
> __save_stack_trace_tsk(current, trace, stack_dump_sp);
> }
> +
> +static int start_analyzer_test __initdata;
> +
> +static int __init enable_analyzer_test(char *str)
> +{
> + get_option(&str, &start_analyzer_test);
> + return 0;
> +}
> +early_param("function_prologue_analyzer_test", enable_analyzer_test);
> +
> +static void __init do_test_function_prologue_analyzer(void)
> +{
> + extern unsigned long __start_mcount_loc[];
> + extern unsigned long __stop_mcount_loc[];
> + unsigned long count, i, errors;
> + int print_once;
> +
> + count = __stop_mcount_loc - __start_mcount_loc;
> + errors = print_once = 0;
> + for (i = 0; i < count; i++) {
> + unsigned long addr, sp_off, fp_off;
> + int pos;
> + bool check;
> + char buf[60];
> +
> + addr = __start_mcount_loc[i];
> + pos = analyze_function_prologue(addr, &sp_off, &fp_off);
> + check = ((pos != 0) || !sp_off || (sp_off <= fp_off));
> + if (check)
> + errors++;
> + if (check || (start_analyzer_test > 1)) {
> + if (!print_once) {
> + pr_debug(" po sp fp symbol\n");
> + pr_debug(" == == == ======\n");
> + print_once++;
> + }
> + sprint_symbol(buf, addr);
> + pr_debug("%5ld: %d 0x%03lx 0x%03lx %s\n",
> + i, pos, sp_off, fp_off, buf);
> + }
It would be hard to find out which entry has an error if start_analyer_test is
set to 2. How about adding one more column to mark 'OK' or 'NG'?
BTW, as I mentioned in previous thread, I'm also waiting for feedbacks on the
analyzer itself :)
Best Regards
Jungseok Lee
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 1/6] arm64: ftrace: modify a stack frame in a safe way
2015-11-18 6:43 ` [PATCH v6 1/6] arm64: ftrace: modify a stack frame in a safe way AKASHI Takahiro
@ 2015-11-24 14:22 ` Jungseok Lee
0 siblings, 0 replies; 23+ messages in thread
From: Jungseok Lee @ 2015-11-24 14:22 UTC (permalink / raw)
To: linux-arm-kernel
On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
> Function graph tracer modifies a return address (LR) in a stack frame by
> calling ftrace_prepare_return() in a traced function's function prologue.
> The current code does this modification before preserving an original
> address at ftrace_push_return_trace() and there is always a small window
> of inconsistency when an interrupt occurs.
>
> This doesn't matter, as far as an interrupt stack is introduced, because
> stack tracer won't be invoked in an interrupt context. But it would be
> better to proactively minimize such a window by moving the LR modification
> after ftrace_push_return_trace().
There are two statements in my mind: 1)IRQ stack will be introduced on ARM64
in the future and 2)This change makes subtle variance compared to ARM and x86.
I'm not sure about this patch from those perspectives.
However, I have no objection to the change. I will piggyback on other folks
regarding this patch :)
Best Regards
Jungseok Lee
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame()
2015-11-24 13:42 ` Jungseok Lee
@ 2015-11-25 4:39 ` AKASHI Takahiro
2015-12-02 13:22 ` Will Deacon
0 siblings, 1 reply; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-25 4:39 UTC (permalink / raw)
To: linux-arm-kernel
On 11/24/2015 10:42 PM, Jungseok Lee wrote:
> On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
>> The change here doesn't make any difference in this patch, but is
>> a preparation for later fixing a problem where stacktrace functions,
>> unwind_frame() and walk_stackframe(), may return useless call stacks
>> under function graph tracer.
>
> I'm aligned with the argument. The case cannot be handled correctly
> without ret_stack[] of struct task_struct.
Thanks. I will add some description about why we need 'tsk' in a commit message.
-Takahiro AKASHI
> Best Regards
> Jungseok Lee
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer
2015-11-24 13:37 ` Jungseok Lee
@ 2015-11-25 5:29 ` AKASHI Takahiro
2015-11-25 11:48 ` Jungseok Lee
0 siblings, 1 reply; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-25 5:29 UTC (permalink / raw)
To: linux-arm-kernel
On 11/24/2015 10:37 PM, Jungseok Lee wrote:
> On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
>
> Hi Akashi,
>
>> Function graph tracer modifies a return address (LR) in a stack frame
>> to hook a function return. This will result in many useless entries
>> (return_to_handler) showing up in
>> a) a stack tracer's output
>> b) perf call graph (with perf record -g)
>> c) dump_backtrace (at panic et al.)
>>
>> For example, in case of a),
>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>> $ echo 1 > /proc/sys/kernel/stack_trace_enabled
>> $ cat /sys/kernel/debug/tracing/stack_trace
>> Depth Size Location (54 entries)
>> ----- ---- --------
>> 0) 4504 16 gic_raise_softirq+0x28/0x150
>> 1) 4488 80 smp_cross_call+0x38/0xb8
>> 2) 4408 48 return_to_handler+0x0/0x40
>> 3) 4360 32 return_to_handler+0x0/0x40
>> ...
>>
>> In case of b),
>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>> $ perf record -e mem:XXX:x -ag -- sleep 10
>> $ perf report
>> ...
>> | | |--0.22%-- 0x550f8
>> | | | 0x10888
>> | | | el0_svc_naked
>> | | | sys_openat
>> | | | return_to_handler
>> | | | return_to_handler
>> ...
>>
>> In case of c),
>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>> $ echo c > /proc/sysrq-trigger
>> ...
>> Call trace:
>> [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
>> [<ffffffc000092250>] return_to_handler+0x0/0x40
>> [<ffffffc000092250>] return_to_handler+0x0/0x40
>> ...
>>
>> This patch replaces such entries with real addresses preserved in
>> current->ret_stack[] at unwind_frame(). This way, we can cover all
>> the cases.
>
> I've observed a strange behavior when playing with case c). Call trace
> is as follows when function_graph is not used.
>
> Call trace:
> [<fffffe00003dc738>] sysrq_handle_crash+0x24/0x30 <- (1)
> [<fffffe00003dd2ac>] __handle_sysrq+0x128/0x19c <- (2)
> [<fffffe00003dd730>] write_sysrq_trigger+0x60/0x74
> [<fffffe0000249fc4>] proc_reg_write+0x84/0xc0
> [<fffffe00001f2638>] __vfs_write+0x44/0x104 <- (3)
> [<fffffe00001f2e60>] vfs_write+0x98/0x1a8
> [<fffffe00001f3730>] SyS_write+0x50/0xb0
> [<fffffe00000939ec>] el0_svc_naked+0x20/0x28 <- (4)
>
> When function_graph is set, some entries, such as do_mem_abort, are added
> between (1) and (2). In addition, entries from (3) to (4) are not printed
> out. As tracking down elements of ret_stack[], I realize dump_backtrace()
> has been terminated before reaching to ret_stack[0]. Have you seen this
> kind of behavior? I believe push & pop operations work correctly.
>
> Please correct me if I'm wrong.
Oops, I mis-interpreted the result output.
You are right.
This can happen because the original dump_backtrace() wants to trace a stack
from a function where an exception has taken place, sysrq_handle_crash(), while
ret_stack[curr_ret_stack] doesn't point to that function, but the very top of
traced functions in callchains, that is, __do_kernel_fault in your case, probably.
So it results in replacing entries of return_to_handler to wrong function addresses.
A fixup! patch attached below (informative only) fixes this issue by once tracing
all the functions on a stack, but preventing a top few ones from being printed.
But there is a tricky thing here: we have to use 'regs->pc' instead of frame.pc
as a trapped function because, as I've already mentioned before, we always miss
the function when walking through a stack from an exception handler to functions
in a thread context.
(Please note that we will introduce a dummy stack frame at interrupt, but not
at exception.)
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>> arch/arm64/include/asm/ftrace.h | 2 ++
>> arch/arm64/include/asm/stacktrace.h | 3 +++
>> arch/arm64/kernel/perf_callchain.c | 3 +++
>> arch/arm64/kernel/process.c | 3 +++
>> arch/arm64/kernel/return_address.c | 3 +++
>> arch/arm64/kernel/stacktrace.c | 17 +++++++++++++++++
>> arch/arm64/kernel/time.c | 3 +++
>> arch/arm64/kernel/traps.c | 3 +++
>> 8 files changed, 37 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
>> index c5534fa..3c60f37 100644
>> --- a/arch/arm64/include/asm/ftrace.h
>> +++ b/arch/arm64/include/asm/ftrace.h
>> @@ -28,6 +28,8 @@ struct dyn_arch_ftrace {
>>
>> extern unsigned long ftrace_graph_call;
>>
>> +extern void return_to_handler(void);
>> +
>> static inline unsigned long ftrace_call_adjust(unsigned long addr)
>> {
>> /*
>> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
>> index 6fb61c5..801a16db 100644
>> --- a/arch/arm64/include/asm/stacktrace.h
>> +++ b/arch/arm64/include/asm/stacktrace.h
>> @@ -22,6 +22,9 @@ struct stackframe {
>> unsigned long fp;
>> unsigned long sp;
>> unsigned long pc;
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> + unsigned int graph;
>> +#endif
>> };
>
> How about using int instead of unsigned int to align with cure_ret_stack
> of struct task_struct?
>
>> extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
>> diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
>> index 797220d..ff46654 100644
>> --- a/arch/arm64/kernel/perf_callchain.c
>> +++ b/arch/arm64/kernel/perf_callchain.c
>> @@ -164,6 +164,9 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
>> frame.fp = regs->regs[29];
>> frame.sp = regs->sp;
>> frame.pc = regs->pc;
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> + frame.graph = current->curr_ret_stack;
>> +#endif
>>
>> walk_stackframe(current, &frame, callchain_trace, entry);
>> }
>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>> index 98bf546..88d742b 100644
>> --- a/arch/arm64/kernel/process.c
>> +++ b/arch/arm64/kernel/process.c
>> @@ -344,6 +344,9 @@ unsigned long get_wchan(struct task_struct *p)
>> frame.fp = thread_saved_fp(p);
>> frame.sp = thread_saved_sp(p);
>> frame.pc = thread_saved_pc(p);
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> + frame.graph = p->curr_ret_stack;
>> +#endif
>> stack_page = (unsigned long)task_stack_page(p);
>> do {
>> if (frame.sp < stack_page ||
>> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
>> index 07b37ac..1718706 100644
>> --- a/arch/arm64/kernel/return_address.c
>> +++ b/arch/arm64/kernel/return_address.c
>> @@ -43,6 +43,9 @@ void *return_address(unsigned int level)
>> frame.fp = (unsigned long)__builtin_frame_address(0);
>> frame.sp = current_stack_pointer;
>> frame.pc = (unsigned long)return_address; /* dummy */
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> + frame.graph = current->curr_ret_stack;
>> +#endif
>>
>> walk_stackframe(current, &frame, save_return_addr, &data);
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 9c7acf8..0a39049 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -17,6 +17,7 @@
>> */
>> #include <linux/kernel.h>
>> #include <linux/export.h>
>> +#include <linux/ftrace.h>
>> #include <linux/sched.h>
>> #include <linux/stacktrace.h>
>>
>> @@ -66,6 +67,19 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>> frame->fp = *(unsigned long *)(fp);
>> frame->pc = *(unsigned long *)(fp + 8);
>>
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> + if (tsk && tsk->ret_stack &&
>> + (frame->pc == (unsigned long)return_to_handler)) {
>> + /*
>> + * This is a case where function graph tracer has
>> + * modified a return address (LR) in a stack frame
>> + * to hook a function return.
>> + * So replace it to an original value.
>> + */
>> + frame->pc = tsk->ret_stack[frame->graph--].ret;
>> + }
>> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>
> There is an index check of ret_stack[] in case of x86 [1]. Even though
> graph is unsigned int, I think we need to check the value of frame->graph
> before accessing ret_stack[].
I'm not sure that the checking is very useful because, if it happens,
it is a bug. It might make sense to avoid a possible panic though.
>> +
>> /*
>> * Check whether we are going to walk through from interrupt stack
>> * to task stack.
>> @@ -137,6 +151,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
>> frame.sp = current_stack_pointer;
>> frame.pc = (unsigned long)save_stack_trace_tsk;
>> }
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> + frame.graph = tsk->curr_ret_stack;
>> +#endif
>>
>> walk_stackframe(tsk, &frame, save_trace, &data);
>> if (trace->nr_entries < trace->max_entries)
>> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
>> index 6e5c521..5977969 100644
>> --- a/arch/arm64/kernel/time.c
>> +++ b/arch/arm64/kernel/time.c
>> @@ -52,6 +52,9 @@ unsigned long profile_pc(struct pt_regs *regs)
>> frame.fp = regs->regs[29];
>> frame.sp = regs->sp;
>> frame.pc = regs->pc;
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> + frame.graph = -1; /* no task info */
>> +#endif
>
> graph is unsigned int type. Is this intentional?
No. This initialization is, I believe, redundant as it is not checked anywhere,
but I will re-think of it along with the checking above.
Thanks,
-Takahiro AKASHI
> Best Regards
> Jungseok Lee
>
> [1] arch/x86/kernel/dumpstack.c
>
----8<----
From 9ebba7167f7838daf68d8231f04141d2f4d4b7b5 Mon Sep 17 00:00:00 2001
From: AKASHI Takahiro <takahiro.akashi@linaro.org>
Date: Wed, 25 Nov 2015 13:29:54 +0900
Subject: [PATCH] fixup! arm64: ftrace: fix a stack tracer's output under
function graph tracer
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
arch/arm64/kernel/traps.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 46053c2..f140029 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -147,17 +147,14 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
{
struct stackframe frame;
unsigned long _irq_stack_ptr = per_cpu(irq_stack_ptr, smp_processor_id());
+ int skip;
pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
if (!tsk)
tsk = current;
- if (regs) {
- frame.fp = regs->regs[29];
- frame.sp = regs->sp;
- frame.pc = regs->pc;
- } else if (tsk == current) {
+ if (tsk == current) {
frame.fp = (unsigned long)__builtin_frame_address(0);
frame.sp = current_stack_pointer;
frame.pc = (unsigned long)dump_backtrace;
@@ -173,13 +170,27 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
frame.graph = tsk->curr_ret_stack;
#endif
+ skip = (regs ? 1 : 0);
pr_emerg("Call trace:\n");
while (1) {
unsigned long where = frame.pc;
unsigned long stack;
int ret;
- dump_backtrace_entry(where);
+ /* skip until specified stack frame */
+ if (!skip)
+ dump_backtrace_entry(where);
+ else if (frame.fp == regs->regs[29]) {
+ skip = 0;
+ /*
+ * Mostly, this is the case where this function is
+ * called in panic/abort. As exception handler's
+ * stack frame does not contain the corresponding pc
+ * at which an exception has taken place, use regs->pc
+ * instead.
+ */
+ dump_backtrace_entry(regs->pc);
+ }
ret = unwind_frame(tsk, &frame);
if (ret < 0)
break;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 6/6] arm64: ftrace: add a test of function prologue analyzer
2015-11-24 13:50 ` Jungseok Lee
@ 2015-11-25 5:33 ` AKASHI Takahiro
0 siblings, 0 replies; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-25 5:33 UTC (permalink / raw)
To: linux-arm-kernel
On 11/24/2015 10:50 PM, Jungseok Lee wrote:
> On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
>> The patch ("arm64: ftrace: add arch-specific stack tracer") introduced
>> a function prologue analyzer.
>>
>> Given that there is no fixed template for a function prologue, at least
>> on gcc for aarch64, a function prologue analyzer may be rather heuristic.
>> So this patch adds a kernel command line option,
>> function_prologue_analyzer_test, in order to run a basic test at startup
>> by executing an analyzer against all the *traceable* functions.
>>
>> For function_prologue_analyzer_test=2, the output looks like:
>>
>> po sp fp symbol
>> == == == ======
>> 0: 0 0x040 0x000 gic_handle_irq+0x20/0xa4
>> 1: 0 0x040 0x000 gic_handle_irq+0x34/0x114
>> 2: 0 0x030 0x000 run_init_process+0x14/0x48
>> 3: 0 0x020 0x000 try_to_run_init_process+0x14/0x58
>> 4: 0 0x080 0x000 do_one_initcall+0x1c/0x194
>> ...
>> 22959: 0 0x020 0x000 tty_lock_slave+0x14/0x3c
>> 22960: 0 0x020 0x000 tty_unlock_slave+0x14/0x3c
>> function prologue analyzer test: 0 errors
>>
>> "po" indicates a position of callsite of mcount(), and should be zero
>> if an analyzer has parsed a function prologue successfully and reached
>> a location where fp is properly updated.
>> "sp" is a final offset to a parent's fp at the exit of function prologue.
>> "fp" is also an ofset against sp at the exit of function prologue.
>> So normally,
>> <new sp> = <old fp> + <"sp">
>> <new fp> = <new sp> - <"fp">
>>
>> And the last line shows the number of possible errors in the result.
>>
>> For function_prologue_analyzer_test=1, only the last line will be shown.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>> arch/arm64/include/asm/insn.h | 2 +-
>> arch/arm64/kernel/stacktrace.c | 52 ++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index 8d5c538..1dfae16 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -281,7 +281,7 @@ __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F)
>> __AARCH64_INSN_FUNCS(br, 0xFFFFFC1F, 0xD61F0000)
>> __AARCH64_INSN_FUNCS(blr, 0xFFFFFC1F, 0xD63F0000)
>> __AARCH64_INSN_FUNCS(ret, 0xFFFFFC1F, 0xD65F0000)
>> -__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F00E0)
>> +__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F03E0)
>
> How about folding(squashing) this hunk into [PATCH 4/6]?
>
>> #undef __AARCH64_INSN_FUNCS
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 1d18bc4..19dad62 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -18,6 +18,7 @@
>> #include <linux/kernel.h>
>> #include <linux/export.h>
>> #include <linux/ftrace.h>
>> +#include <linux/kallsyms.h>
>> #include <linux/sched.h>
>> #include <linux/stacktrace.h>
>>
>> @@ -348,5 +349,56 @@ void save_stack_trace_sp(struct stack_trace *trace,
>> {
>> __save_stack_trace_tsk(current, trace, stack_dump_sp);
>> }
>> +
>> +static int start_analyzer_test __initdata;
>> +
>> +static int __init enable_analyzer_test(char *str)
>> +{
>> + get_option(&str, &start_analyzer_test);
>> + return 0;
>> +}
>> +early_param("function_prologue_analyzer_test", enable_analyzer_test);
>> +
>> +static void __init do_test_function_prologue_analyzer(void)
>> +{
>> + extern unsigned long __start_mcount_loc[];
>> + extern unsigned long __stop_mcount_loc[];
>> + unsigned long count, i, errors;
>> + int print_once;
>> +
>> + count = __stop_mcount_loc - __start_mcount_loc;
>> + errors = print_once = 0;
>> + for (i = 0; i < count; i++) {
>> + unsigned long addr, sp_off, fp_off;
>> + int pos;
>> + bool check;
>> + char buf[60];
>> +
>> + addr = __start_mcount_loc[i];
>> + pos = analyze_function_prologue(addr, &sp_off, &fp_off);
>> + check = ((pos != 0) || !sp_off || (sp_off <= fp_off));
>> + if (check)
>> + errors++;
>> + if (check || (start_analyzer_test > 1)) {
>> + if (!print_once) {
>> + pr_debug(" po sp fp symbol\n");
>> + pr_debug(" == == == ======\n");
>> + print_once++;
>> + }
>> + sprint_symbol(buf, addr);
>> + pr_debug("%5ld: %d 0x%03lx 0x%03lx %s\n",
>> + i, pos, sp_off, fp_off, buf);
>> + }
>
> It would be hard to find out which entry has an error if start_analyer_test is
> set to 2. How about adding one more column to mark 'OK' or 'NG'?
Thank you for the comment.
> BTW, as I mentioned in previous thread, I'm also waiting for feedbacks on the
> analyzer itself :)
Me, too.
-Takahiro AKASHI
> Best Regards
> Jungseok Lee
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer
2015-11-25 5:29 ` AKASHI Takahiro
@ 2015-11-25 11:48 ` Jungseok Lee
2015-11-26 3:05 ` AKASHI Takahiro
0 siblings, 1 reply; 23+ messages in thread
From: Jungseok Lee @ 2015-11-25 11:48 UTC (permalink / raw)
To: linux-arm-kernel
On Nov 25, 2015, at 2:29 PM, AKASHI Takahiro wrote:
> On 11/24/2015 10:37 PM, Jungseok Lee wrote:
>> On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
>>
>> Hi Akashi,
>>
>>> Function graph tracer modifies a return address (LR) in a stack frame
>>> to hook a function return. This will result in many useless entries
>>> (return_to_handler) showing up in
>>> a) a stack tracer's output
>>> b) perf call graph (with perf record -g)
>>> c) dump_backtrace (at panic et al.)
>>>
>>> For example, in case of a),
>>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>>> $ echo 1 > /proc/sys/kernel/stack_trace_enabled
>>> $ cat /sys/kernel/debug/tracing/stack_trace
>>> Depth Size Location (54 entries)
>>> ----- ---- --------
>>> 0) 4504 16 gic_raise_softirq+0x28/0x150
>>> 1) 4488 80 smp_cross_call+0x38/0xb8
>>> 2) 4408 48 return_to_handler+0x0/0x40
>>> 3) 4360 32 return_to_handler+0x0/0x40
>>> ...
>>>
>>> In case of b),
>>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>>> $ perf record -e mem:XXX:x -ag -- sleep 10
>>> $ perf report
>>> ...
>>> | | |--0.22%-- 0x550f8
>>> | | | 0x10888
>>> | | | el0_svc_naked
>>> | | | sys_openat
>>> | | | return_to_handler
>>> | | | return_to_handler
>>> ...
>>>
>>> In case of c),
>>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>>> $ echo c > /proc/sysrq-trigger
>>> ...
>>> Call trace:
>>> [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
>>> [<ffffffc000092250>] return_to_handler+0x0/0x40
>>> [<ffffffc000092250>] return_to_handler+0x0/0x40
>>> ...
>>>
>>> This patch replaces such entries with real addresses preserved in
>>> current->ret_stack[] at unwind_frame(). This way, we can cover all
>>> the cases.
>>
>> I've observed a strange behavior when playing with case c). Call trace
>> is as follows when function_graph is not used.
>>
>> Call trace:
>> [<fffffe00003dc738>] sysrq_handle_crash+0x24/0x30 <- (1)
>> [<fffffe00003dd2ac>] __handle_sysrq+0x128/0x19c <- (2)
>> [<fffffe00003dd730>] write_sysrq_trigger+0x60/0x74
>> [<fffffe0000249fc4>] proc_reg_write+0x84/0xc0
>> [<fffffe00001f2638>] __vfs_write+0x44/0x104 <- (3)
>> [<fffffe00001f2e60>] vfs_write+0x98/0x1a8
>> [<fffffe00001f3730>] SyS_write+0x50/0xb0
>> [<fffffe00000939ec>] el0_svc_naked+0x20/0x28 <- (4)
>>
>> When function_graph is set, some entries, such as do_mem_abort, are added
>> between (1) and (2). In addition, entries from (3) to (4) are not printed
>> out. As tracking down elements of ret_stack[], I realize dump_backtrace()
>> has been terminated before reaching to ret_stack[0]. Have you seen this
>> kind of behavior? I believe push & pop operations work correctly.
>>
>> Please correct me if I'm wrong.
>
> Oops, I mis-interpreted the result output.
> You are right.
> This can happen because the original dump_backtrace() wants to trace a stack
> from a function where an exception has taken place, sysrq_handle_crash(), while
> ret_stack[curr_ret_stack] doesn't point to that function, but the very top of
> traced functions in callchains, that is, __do_kernel_fault in your case, probably.
Yes, __do_kernel_fault.
> So it results in replacing entries of return_to_handler to wrong function addresses.
>
> A fixup! patch attached below (informative only) fixes this issue by once tracing
> all the functions on a stack, but preventing a top few ones from being printed.
> But there is a tricky thing here: we have to use 'regs->pc' instead of frame.pc
> as a trapped function because, as I've already mentioned before, we always miss
> the function when walking through a stack from an exception handler to functions
> in a thread context.
> (Please note that we will introduce a dummy stack frame at interrupt, but not
> at exception.)
Thanks for clear explanation!
Since I'm not the only person to experience the above case, I report one more and
the last observation ;)
PC and LR information is printed out when function_graph is set.
PC is at sysrq_handle_crash+0x24/0x30
LR is at sysrq_handle_crash+0x10/0x30
The logs are as follows when function_graph turns off.
PC is at sysrq_handle_crash+0x24/0x30
LR is at __handle_sysrq+0x128/0x19c
I think __show_regs() might have a similar problem when retrieving LR according to
the below informative patch. Thoughts?
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>> arch/arm64/include/asm/ftrace.h | 2 ++
>>> arch/arm64/include/asm/stacktrace.h | 3 +++
>>> arch/arm64/kernel/perf_callchain.c | 3 +++
>>> arch/arm64/kernel/process.c | 3 +++
>>> arch/arm64/kernel/return_address.c | 3 +++
>>> arch/arm64/kernel/stacktrace.c | 17 +++++++++++++++++
>>> arch/arm64/kernel/time.c | 3 +++
>>> arch/arm64/kernel/traps.c | 3 +++
>>> 8 files changed, 37 insertions(+)
>>>
>>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
>>> index c5534fa..3c60f37 100644
>>> --- a/arch/arm64/include/asm/ftrace.h
>>> +++ b/arch/arm64/include/asm/ftrace.h
>>> @@ -28,6 +28,8 @@ struct dyn_arch_ftrace {
>>>
>>> extern unsigned long ftrace_graph_call;
>>>
>>> +extern void return_to_handler(void);
>>> +
>>> static inline unsigned long ftrace_call_adjust(unsigned long addr)
>>> {
>>> /*
>>> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
>>> index 6fb61c5..801a16db 100644
>>> --- a/arch/arm64/include/asm/stacktrace.h
>>> +++ b/arch/arm64/include/asm/stacktrace.h
>>> @@ -22,6 +22,9 @@ struct stackframe {
>>> unsigned long fp;
>>> unsigned long sp;
>>> unsigned long pc;
>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>> + unsigned int graph;
>>> +#endif
>>> };
>>
>> How about using int instead of unsigned int to align with cure_ret_stack
>> of struct task_struct?
>>
>>> extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
>>> diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
>>> index 797220d..ff46654 100644
>>> --- a/arch/arm64/kernel/perf_callchain.c
>>> +++ b/arch/arm64/kernel/perf_callchain.c
>>> @@ -164,6 +164,9 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
>>> frame.fp = regs->regs[29];
>>> frame.sp = regs->sp;
>>> frame.pc = regs->pc;
>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>> + frame.graph = current->curr_ret_stack;
>>> +#endif
>>>
>>> walk_stackframe(current, &frame, callchain_trace, entry);
>>> }
>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>>> index 98bf546..88d742b 100644
>>> --- a/arch/arm64/kernel/process.c
>>> +++ b/arch/arm64/kernel/process.c
>>> @@ -344,6 +344,9 @@ unsigned long get_wchan(struct task_struct *p)
>>> frame.fp = thread_saved_fp(p);
>>> frame.sp = thread_saved_sp(p);
>>> frame.pc = thread_saved_pc(p);
>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>> + frame.graph = p->curr_ret_stack;
>>> +#endif
>>> stack_page = (unsigned long)task_stack_page(p);
>>> do {
>>> if (frame.sp < stack_page ||
>>> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
>>> index 07b37ac..1718706 100644
>>> --- a/arch/arm64/kernel/return_address.c
>>> +++ b/arch/arm64/kernel/return_address.c
>>> @@ -43,6 +43,9 @@ void *return_address(unsigned int level)
>>> frame.fp = (unsigned long)__builtin_frame_address(0);
>>> frame.sp = current_stack_pointer;
>>> frame.pc = (unsigned long)return_address; /* dummy */
>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>> + frame.graph = current->curr_ret_stack;
>>> +#endif
>>>
>>> walk_stackframe(current, &frame, save_return_addr, &data);
>>>
>>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>>> index 9c7acf8..0a39049 100644
>>> --- a/arch/arm64/kernel/stacktrace.c
>>> +++ b/arch/arm64/kernel/stacktrace.c
>>> @@ -17,6 +17,7 @@
>>> */
>>> #include <linux/kernel.h>
>>> #include <linux/export.h>
>>> +#include <linux/ftrace.h>
>>> #include <linux/sched.h>
>>> #include <linux/stacktrace.h>
>>>
>>> @@ -66,6 +67,19 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>>> frame->fp = *(unsigned long *)(fp);
>>> frame->pc = *(unsigned long *)(fp + 8);
>>>
>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>> + if (tsk && tsk->ret_stack &&
>>> + (frame->pc == (unsigned long)return_to_handler)) {
>>> + /*
>>> + * This is a case where function graph tracer has
>>> + * modified a return address (LR) in a stack frame
>>> + * to hook a function return.
>>> + * So replace it to an original value.
>>> + */
>>> + frame->pc = tsk->ret_stack[frame->graph--].ret;
>>> + }
>>> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>>
>> There is an index check of ret_stack[] in case of x86 [1]. Even though
>> graph is unsigned int, I think we need to check the value of frame->graph
>> before accessing ret_stack[].
>
> I'm not sure that the checking is very useful because, if it happens,
> it is a bug. It might make sense to avoid a possible panic though.
>
>>> +
>>> /*
>>> * Check whether we are going to walk through from interrupt stack
>>> * to task stack.
>>> @@ -137,6 +151,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
>>> frame.sp = current_stack_pointer;
>>> frame.pc = (unsigned long)save_stack_trace_tsk;
>>> }
>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>> + frame.graph = tsk->curr_ret_stack;
>>> +#endif
>>>
>>> walk_stackframe(tsk, &frame, save_trace, &data);
>>> if (trace->nr_entries < trace->max_entries)
>>> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
>>> index 6e5c521..5977969 100644
>>> --- a/arch/arm64/kernel/time.c
>>> +++ b/arch/arm64/kernel/time.c
>>> @@ -52,6 +52,9 @@ unsigned long profile_pc(struct pt_regs *regs)
>>> frame.fp = regs->regs[29];
>>> frame.sp = regs->sp;
>>> frame.pc = regs->pc;
>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>> + frame.graph = -1; /* no task info */
>>> +#endif
>>
>> graph is unsigned int type. Is this intentional?
>
> No. This initialization is, I believe, redundant as it is not checked anywhere,
> but I will re-think of it along with the checking above.
>
> Thanks,
> -Takahiro AKASHI
>
>> Best Regards
>> Jungseok Lee
>>
>> [1] arch/x86/kernel/dumpstack.c
>>
> ----8<----
> From 9ebba7167f7838daf68d8231f04141d2f4d4b7b5 Mon Sep 17 00:00:00 2001
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Date: Wed, 25 Nov 2015 13:29:54 +0900
> Subject: [PATCH] fixup! arm64: ftrace: fix a stack tracer's output under
> function graph tracer
>
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/kernel/traps.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 46053c2..f140029 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -147,17 +147,14 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> {
> struct stackframe frame;
> unsigned long _irq_stack_ptr = per_cpu(irq_stack_ptr, smp_processor_id());
> + int skip;
>
> pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>
> if (!tsk)
> tsk = current;
>
> - if (regs) {
> - frame.fp = regs->regs[29];
> - frame.sp = regs->sp;
> - frame.pc = regs->pc;
> - } else if (tsk == current) {
> + if (tsk == current) {
> frame.fp = (unsigned long)__builtin_frame_address(0);
> frame.sp = current_stack_pointer;
> frame.pc = (unsigned long)dump_backtrace;
> @@ -173,13 +170,27 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> frame.graph = tsk->curr_ret_stack;
> #endif
>
> + skip = (regs ? 1 : 0);
> pr_emerg("Call trace:\n");
> while (1) {
> unsigned long where = frame.pc;
> unsigned long stack;
> int ret;
>
> - dump_backtrace_entry(where);
> + /* skip until specified stack frame */
> + if (!skip)
> + dump_backtrace_entry(where);
> + else if (frame.fp == regs->regs[29]) {
> + skip = 0;
> + /*
> + * Mostly, this is the case where this function is
> + * called in panic/abort. As exception handler's
> + * stack frame does not contain the corresponding pc
> + * at which an exception has taken place, use regs->pc
> + * instead.
> + */
> + dump_backtrace_entry(regs->pc);
> + }
> ret = unwind_frame(tsk, &frame);
> if (ret < 0)
> break;
> --
> 1.7.9.5
This fixes up the issue I mentioned.
Best Regards
Jungseok Lee
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer
2015-11-25 11:48 ` Jungseok Lee
@ 2015-11-26 3:05 ` AKASHI Takahiro
2015-11-26 14:05 ` Jungseok Lee
0 siblings, 1 reply; 23+ messages in thread
From: AKASHI Takahiro @ 2015-11-26 3:05 UTC (permalink / raw)
To: linux-arm-kernel
Jungseok,
On 11/25/2015 08:48 PM, Jungseok Lee wrote:
> On Nov 25, 2015, at 2:29 PM, AKASHI Takahiro wrote:
>> On 11/24/2015 10:37 PM, Jungseok Lee wrote:
>>> On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
>>>
>>> Hi Akashi,
>>>
>>>> Function graph tracer modifies a return address (LR) in a stack frame
>>>> to hook a function return. This will result in many useless entries
>>>> (return_to_handler) showing up in
>>>> a) a stack tracer's output
>>>> b) perf call graph (with perf record -g)
>>>> c) dump_backtrace (at panic et al.)
>>>>
>>>> For example, in case of a),
>>>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>>>> $ echo 1 > /proc/sys/kernel/stack_trace_enabled
>>>> $ cat /sys/kernel/debug/tracing/stack_trace
>>>> Depth Size Location (54 entries)
>>>> ----- ---- --------
>>>> 0) 4504 16 gic_raise_softirq+0x28/0x150
>>>> 1) 4488 80 smp_cross_call+0x38/0xb8
>>>> 2) 4408 48 return_to_handler+0x0/0x40
>>>> 3) 4360 32 return_to_handler+0x0/0x40
>>>> ...
>>>>
>>>> In case of b),
>>>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>>>> $ perf record -e mem:XXX:x -ag -- sleep 10
>>>> $ perf report
>>>> ...
>>>> | | |--0.22%-- 0x550f8
>>>> | | | 0x10888
>>>> | | | el0_svc_naked
>>>> | | | sys_openat
>>>> | | | return_to_handler
>>>> | | | return_to_handler
>>>> ...
>>>>
>>>> In case of c),
>>>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>>>> $ echo c > /proc/sysrq-trigger
>>>> ...
>>>> Call trace:
>>>> [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
>>>> [<ffffffc000092250>] return_to_handler+0x0/0x40
>>>> [<ffffffc000092250>] return_to_handler+0x0/0x40
>>>> ...
>>>>
>>>> This patch replaces such entries with real addresses preserved in
>>>> current->ret_stack[] at unwind_frame(). This way, we can cover all
>>>> the cases.
>>>
>>> I've observed a strange behavior when playing with case c). Call trace
>>> is as follows when function_graph is not used.
>>>
>>> Call trace:
>>> [<fffffe00003dc738>] sysrq_handle_crash+0x24/0x30 <- (1)
>>> [<fffffe00003dd2ac>] __handle_sysrq+0x128/0x19c <- (2)
>>> [<fffffe00003dd730>] write_sysrq_trigger+0x60/0x74
>>> [<fffffe0000249fc4>] proc_reg_write+0x84/0xc0
>>> [<fffffe00001f2638>] __vfs_write+0x44/0x104 <- (3)
>>> [<fffffe00001f2e60>] vfs_write+0x98/0x1a8
>>> [<fffffe00001f3730>] SyS_write+0x50/0xb0
>>> [<fffffe00000939ec>] el0_svc_naked+0x20/0x28 <- (4)
>>>
>>> When function_graph is set, some entries, such as do_mem_abort, are added
>>> between (1) and (2). In addition, entries from (3) to (4) are not printed
>>> out. As tracking down elements of ret_stack[], I realize dump_backtrace()
>>> has been terminated before reaching to ret_stack[0]. Have you seen this
>>> kind of behavior? I believe push & pop operations work correctly.
>>>
>>> Please correct me if I'm wrong.
>>
>> Oops, I mis-interpreted the result output.
>> You are right.
>> This can happen because the original dump_backtrace() wants to trace a stack
>> from a function where an exception has taken place, sysrq_handle_crash(), while
>> ret_stack[curr_ret_stack] doesn't point to that function, but the very top of
>> traced functions in callchains, that is, __do_kernel_fault in your case, probably.
>
> Yes, __do_kernel_fault.
>
>> So it results in replacing entries of return_to_handler to wrong function addresses.
>>
>> A fixup! patch attached below (informative only) fixes this issue by once tracing
>> all the functions on a stack, but preventing a top few ones from being printed.
>> But there is a tricky thing here: we have to use 'regs->pc' instead of frame.pc
>> as a trapped function because, as I've already mentioned before, we always miss
>> the function when walking through a stack from an exception handler to functions
>> in a thread context.
>> (Please note that we will introduce a dummy stack frame at interrupt, but not
>> at exception.)
>
> Thanks for clear explanation!
>
> Since I'm not the only person to experience the above case, I report one more and
> the last observation ;)
>
> PC and LR information is printed out when function_graph is set.
>
> PC is at sysrq_handle_crash+0x24/0x30
> LR is at sysrq_handle_crash+0x10/0x30
>
> The logs are as follows when function_graph turns off.
>
> PC is at sysrq_handle_crash+0x24/0x30
> LR is at __handle_sysrq+0x128/0x19c
>
> I think __show_regs() might have a similar problem when retrieving LR according to
> the below informative patch. Thoughts?
Well, I believe that it is normal.
Sysrq_handle_crash() is basically a leaf function, but once function or function graph
tracer is turned on, ftrace_caller() is called before its function body and so LR has
been modified when panic/show_regs().
Thanks,
-Takahiro AKASHI
>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>> ---
>>>> arch/arm64/include/asm/ftrace.h | 2 ++
>>>> arch/arm64/include/asm/stacktrace.h | 3 +++
>>>> arch/arm64/kernel/perf_callchain.c | 3 +++
>>>> arch/arm64/kernel/process.c | 3 +++
>>>> arch/arm64/kernel/return_address.c | 3 +++
>>>> arch/arm64/kernel/stacktrace.c | 17 +++++++++++++++++
>>>> arch/arm64/kernel/time.c | 3 +++
>>>> arch/arm64/kernel/traps.c | 3 +++
>>>> 8 files changed, 37 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
>>>> index c5534fa..3c60f37 100644
>>>> --- a/arch/arm64/include/asm/ftrace.h
>>>> +++ b/arch/arm64/include/asm/ftrace.h
>>>> @@ -28,6 +28,8 @@ struct dyn_arch_ftrace {
>>>>
>>>> extern unsigned long ftrace_graph_call;
>>>>
>>>> +extern void return_to_handler(void);
>>>> +
>>>> static inline unsigned long ftrace_call_adjust(unsigned long addr)
>>>> {
>>>> /*
>>>> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
>>>> index 6fb61c5..801a16db 100644
>>>> --- a/arch/arm64/include/asm/stacktrace.h
>>>> +++ b/arch/arm64/include/asm/stacktrace.h
>>>> @@ -22,6 +22,9 @@ struct stackframe {
>>>> unsigned long fp;
>>>> unsigned long sp;
>>>> unsigned long pc;
>>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>>> + unsigned int graph;
>>>> +#endif
>>>> };
>>>
>>> How about using int instead of unsigned int to align with cure_ret_stack
>>> of struct task_struct?
>>>
>>>> extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
>>>> diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
>>>> index 797220d..ff46654 100644
>>>> --- a/arch/arm64/kernel/perf_callchain.c
>>>> +++ b/arch/arm64/kernel/perf_callchain.c
>>>> @@ -164,6 +164,9 @@ void perf_callchain_kernel(struct perf_callchain_entry *entry,
>>>> frame.fp = regs->regs[29];
>>>> frame.sp = regs->sp;
>>>> frame.pc = regs->pc;
>>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>>> + frame.graph = current->curr_ret_stack;
>>>> +#endif
>>>>
>>>> walk_stackframe(current, &frame, callchain_trace, entry);
>>>> }
>>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>>>> index 98bf546..88d742b 100644
>>>> --- a/arch/arm64/kernel/process.c
>>>> +++ b/arch/arm64/kernel/process.c
>>>> @@ -344,6 +344,9 @@ unsigned long get_wchan(struct task_struct *p)
>>>> frame.fp = thread_saved_fp(p);
>>>> frame.sp = thread_saved_sp(p);
>>>> frame.pc = thread_saved_pc(p);
>>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>>> + frame.graph = p->curr_ret_stack;
>>>> +#endif
>>>> stack_page = (unsigned long)task_stack_page(p);
>>>> do {
>>>> if (frame.sp < stack_page ||
>>>> diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
>>>> index 07b37ac..1718706 100644
>>>> --- a/arch/arm64/kernel/return_address.c
>>>> +++ b/arch/arm64/kernel/return_address.c
>>>> @@ -43,6 +43,9 @@ void *return_address(unsigned int level)
>>>> frame.fp = (unsigned long)__builtin_frame_address(0);
>>>> frame.sp = current_stack_pointer;
>>>> frame.pc = (unsigned long)return_address; /* dummy */
>>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>>> + frame.graph = current->curr_ret_stack;
>>>> +#endif
>>>>
>>>> walk_stackframe(current, &frame, save_return_addr, &data);
>>>>
>>>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>>>> index 9c7acf8..0a39049 100644
>>>> --- a/arch/arm64/kernel/stacktrace.c
>>>> +++ b/arch/arm64/kernel/stacktrace.c
>>>> @@ -17,6 +17,7 @@
>>>> */
>>>> #include <linux/kernel.h>
>>>> #include <linux/export.h>
>>>> +#include <linux/ftrace.h>
>>>> #include <linux/sched.h>
>>>> #include <linux/stacktrace.h>
>>>>
>>>> @@ -66,6 +67,19 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>>>> frame->fp = *(unsigned long *)(fp);
>>>> frame->pc = *(unsigned long *)(fp + 8);
>>>>
>>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>>> + if (tsk && tsk->ret_stack &&
>>>> + (frame->pc == (unsigned long)return_to_handler)) {
>>>> + /*
>>>> + * This is a case where function graph tracer has
>>>> + * modified a return address (LR) in a stack frame
>>>> + * to hook a function return.
>>>> + * So replace it to an original value.
>>>> + */
>>>> + frame->pc = tsk->ret_stack[frame->graph--].ret;
>>>> + }
>>>> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>>>
>>> There is an index check of ret_stack[] in case of x86 [1]. Even though
>>> graph is unsigned int, I think we need to check the value of frame->graph
>>> before accessing ret_stack[].
>>
>> I'm not sure that the checking is very useful because, if it happens,
>> it is a bug. It might make sense to avoid a possible panic though.
>>
>>>> +
>>>> /*
>>>> * Check whether we are going to walk through from interrupt stack
>>>> * to task stack.
>>>> @@ -137,6 +151,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
>>>> frame.sp = current_stack_pointer;
>>>> frame.pc = (unsigned long)save_stack_trace_tsk;
>>>> }
>>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>>> + frame.graph = tsk->curr_ret_stack;
>>>> +#endif
>>>>
>>>> walk_stackframe(tsk, &frame, save_trace, &data);
>>>> if (trace->nr_entries < trace->max_entries)
>>>> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
>>>> index 6e5c521..5977969 100644
>>>> --- a/arch/arm64/kernel/time.c
>>>> +++ b/arch/arm64/kernel/time.c
>>>> @@ -52,6 +52,9 @@ unsigned long profile_pc(struct pt_regs *regs)
>>>> frame.fp = regs->regs[29];
>>>> frame.sp = regs->sp;
>>>> frame.pc = regs->pc;
>>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>>> + frame.graph = -1; /* no task info */
>>>> +#endif
>>>
>>> graph is unsigned int type. Is this intentional?
>>
>> No. This initialization is, I believe, redundant as it is not checked anywhere,
>> but I will re-think of it along with the checking above.
>>
>> Thanks,
>> -Takahiro AKASHI
>>
>>> Best Regards
>>> Jungseok Lee
>>>
>>> [1] arch/x86/kernel/dumpstack.c
>>>
>> ----8<----
>> From 9ebba7167f7838daf68d8231f04141d2f4d4b7b5 Mon Sep 17 00:00:00 2001
>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> Date: Wed, 25 Nov 2015 13:29:54 +0900
>> Subject: [PATCH] fixup! arm64: ftrace: fix a stack tracer's output under
>> function graph tracer
>>
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>> arch/arm64/kernel/traps.c | 23 +++++++++++++++++------
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index 46053c2..f140029 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -147,17 +147,14 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>> {
>> struct stackframe frame;
>> unsigned long _irq_stack_ptr = per_cpu(irq_stack_ptr, smp_processor_id());
>> + int skip;
>>
>> pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>>
>> if (!tsk)
>> tsk = current;
>>
>> - if (regs) {
>> - frame.fp = regs->regs[29];
>> - frame.sp = regs->sp;
>> - frame.pc = regs->pc;
>> - } else if (tsk == current) {
>> + if (tsk == current) {
>> frame.fp = (unsigned long)__builtin_frame_address(0);
>> frame.sp = current_stack_pointer;
>> frame.pc = (unsigned long)dump_backtrace;
>> @@ -173,13 +170,27 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>> frame.graph = tsk->curr_ret_stack;
>> #endif
>>
>> + skip = (regs ? 1 : 0);
>> pr_emerg("Call trace:\n");
>> while (1) {
>> unsigned long where = frame.pc;
>> unsigned long stack;
>> int ret;
>>
>> - dump_backtrace_entry(where);
>> + /* skip until specified stack frame */
>> + if (!skip)
>> + dump_backtrace_entry(where);
>> + else if (frame.fp == regs->regs[29]) {
>> + skip = 0;
>> + /*
>> + * Mostly, this is the case where this function is
>> + * called in panic/abort. As exception handler's
>> + * stack frame does not contain the corresponding pc
>> + * at which an exception has taken place, use regs->pc
>> + * instead.
>> + */
>> + dump_backtrace_entry(regs->pc);
>> + }
>> ret = unwind_frame(tsk, &frame);
>> if (ret < 0)
>> break;
>> --
>> 1.7.9.5
>
> This fixes up the issue I mentioned.
>
> Best Regards
> Jungseok Lee
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer
2015-11-26 3:05 ` AKASHI Takahiro
@ 2015-11-26 14:05 ` Jungseok Lee
0 siblings, 0 replies; 23+ messages in thread
From: Jungseok Lee @ 2015-11-26 14:05 UTC (permalink / raw)
To: linux-arm-kernel
On Nov 26, 2015, at 12:05 PM, AKASHI Takahiro wrote:
> Jungseok,
Hi Akashi,
> On 11/25/2015 08:48 PM, Jungseok Lee wrote:
>> On Nov 25, 2015, at 2:29 PM, AKASHI Takahiro wrote:
>>> On 11/24/2015 10:37 PM, Jungseok Lee wrote:
>>>> On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
>>>>
>>>> Hi Akashi,
>>>>
>>>>> Function graph tracer modifies a return address (LR) in a stack frame
>>>>> to hook a function return. This will result in many useless entries
>>>>> (return_to_handler) showing up in
>>>>> a) a stack tracer's output
>>>>> b) perf call graph (with perf record -g)
>>>>> c) dump_backtrace (at panic et al.)
>>>>>
>>>>> For example, in case of a),
>>>>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>>>>> $ echo 1 > /proc/sys/kernel/stack_trace_enabled
>>>>> $ cat /sys/kernel/debug/tracing/stack_trace
>>>>> Depth Size Location (54 entries)
>>>>> ----- ---- --------
>>>>> 0) 4504 16 gic_raise_softirq+0x28/0x150
>>>>> 1) 4488 80 smp_cross_call+0x38/0xb8
>>>>> 2) 4408 48 return_to_handler+0x0/0x40
>>>>> 3) 4360 32 return_to_handler+0x0/0x40
>>>>> ...
>>>>>
>>>>> In case of b),
>>>>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>>>>> $ perf record -e mem:XXX:x -ag -- sleep 10
>>>>> $ perf report
>>>>> ...
>>>>> | | |--0.22%-- 0x550f8
>>>>> | | | 0x10888
>>>>> | | | el0_svc_naked
>>>>> | | | sys_openat
>>>>> | | | return_to_handler
>>>>> | | | return_to_handler
>>>>> ...
>>>>>
>>>>> In case of c),
>>>>> $ echo function_graph > /sys/kernel/debug/tracing/current_tracer
>>>>> $ echo c > /proc/sysrq-trigger
>>>>> ...
>>>>> Call trace:
>>>>> [<ffffffc00044d3ac>] sysrq_handle_crash+0x24/0x30
>>>>> [<ffffffc000092250>] return_to_handler+0x0/0x40
>>>>> [<ffffffc000092250>] return_to_handler+0x0/0x40
>>>>> ...
>>>>>
>>>>> This patch replaces such entries with real addresses preserved in
>>>>> current->ret_stack[] at unwind_frame(). This way, we can cover all
>>>>> the cases.
>>>>
>>>> I've observed a strange behavior when playing with case c). Call trace
>>>> is as follows when function_graph is not used.
>>>>
>>>> Call trace:
>>>> [<fffffe00003dc738>] sysrq_handle_crash+0x24/0x30 <- (1)
>>>> [<fffffe00003dd2ac>] __handle_sysrq+0x128/0x19c <- (2)
>>>> [<fffffe00003dd730>] write_sysrq_trigger+0x60/0x74
>>>> [<fffffe0000249fc4>] proc_reg_write+0x84/0xc0
>>>> [<fffffe00001f2638>] __vfs_write+0x44/0x104 <- (3)
>>>> [<fffffe00001f2e60>] vfs_write+0x98/0x1a8
>>>> [<fffffe00001f3730>] SyS_write+0x50/0xb0
>>>> [<fffffe00000939ec>] el0_svc_naked+0x20/0x28 <- (4)
>>>>
>>>> When function_graph is set, some entries, such as do_mem_abort, are added
>>>> between (1) and (2). In addition, entries from (3) to (4) are not printed
>>>> out. As tracking down elements of ret_stack[], I realize dump_backtrace()
>>>> has been terminated before reaching to ret_stack[0]. Have you seen this
>>>> kind of behavior? I believe push & pop operations work correctly.
>>>>
>>>> Please correct me if I'm wrong.
>>>
>>> Oops, I mis-interpreted the result output.
>>> You are right.
>>> This can happen because the original dump_backtrace() wants to trace a stack
>>> from a function where an exception has taken place, sysrq_handle_crash(), while
>>> ret_stack[curr_ret_stack] doesn't point to that function, but the very top of
>>> traced functions in callchains, that is, __do_kernel_fault in your case, probably.
>>
>> Yes, __do_kernel_fault.
>>
>>> So it results in replacing entries of return_to_handler to wrong function addresses.
>>>
>>> A fixup! patch attached below (informative only) fixes this issue by once tracing
>>> all the functions on a stack, but preventing a top few ones from being printed.
>>> But there is a tricky thing here: we have to use 'regs->pc' instead of frame.pc
>>> as a trapped function because, as I've already mentioned before, we always miss
>>> the function when walking through a stack from an exception handler to functions
>>> in a thread context.
>>> (Please note that we will introduce a dummy stack frame at interrupt, but not
>>> at exception.)
>>
>> Thanks for clear explanation!
>>
>> Since I'm not the only person to experience the above case, I report one more and
>> the last observation ;)
>>
>> PC and LR information is printed out when function_graph is set.
>>
>> PC is at sysrq_handle_crash+0x24/0x30
>> LR is at sysrq_handle_crash+0x10/0x30
>>
>> The logs are as follows when function_graph turns off.
>>
>> PC is at sysrq_handle_crash+0x24/0x30
>> LR is at __handle_sysrq+0x128/0x19c
>>
>> I think __show_regs() might have a similar problem when retrieving LR according to
>> the below informative patch. Thoughts?
>
> Well, I believe that it is normal.
> Sysrq_handle_crash() is basically a leaf function, but once function or function graph
> tracer is turned on, ftrace_caller() is called before its function body and so LR has
> been modified when panic/show_regs().
Got it. All aspects of this patch is clear now. Thanks!
Best Regards
Jungseok Lee
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame()
2015-11-25 4:39 ` AKASHI Takahiro
@ 2015-12-02 13:22 ` Will Deacon
2015-12-02 15:33 ` Jungseok Lee
0 siblings, 1 reply; 23+ messages in thread
From: Will Deacon @ 2015-12-02 13:22 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 25, 2015 at 01:39:36PM +0900, AKASHI Takahiro wrote:
> On 11/24/2015 10:42 PM, Jungseok Lee wrote:
> >On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
> >>The change here doesn't make any difference in this patch, but is
> >>a preparation for later fixing a problem where stacktrace functions,
> >>unwind_frame() and walk_stackframe(), may return useless call stacks
> >>under function graph tracer.
> >
> >I'm aligned with the argument. The case cannot be handled correctly
> >without ret_stack[] of struct task_struct.
>
> Thanks. I will add some description about why we need 'tsk' in a commit
> message.
Ok, so are you planning to repost this series?
Will
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame()
2015-12-02 13:22 ` Will Deacon
@ 2015-12-02 15:33 ` Jungseok Lee
2015-12-10 6:33 ` AKASHI Takahiro
0 siblings, 1 reply; 23+ messages in thread
From: Jungseok Lee @ 2015-12-02 15:33 UTC (permalink / raw)
To: linux-arm-kernel
On Dec 2, 2015, at 10:22 PM, Will Deacon wrote:
> On Wed, Nov 25, 2015 at 01:39:36PM +0900, AKASHI Takahiro wrote:
>> On 11/24/2015 10:42 PM, Jungseok Lee wrote:
>>> On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
>>>> The change here doesn't make any difference in this patch, but is
>>>> a preparation for later fixing a problem where stacktrace functions,
>>>> unwind_frame() and walk_stackframe(), may return useless call stacks
>>>> under function graph tracer.
>>>
>>> I'm aligned with the argument. The case cannot be handled correctly
>>> without ret_stack[] of struct task_struct.
>>
>> Thanks. I will add some description about why we need 'tsk' in a commit
>> message.
>
> Ok, so are you planning to repost this series?
I think this function_graph changes could be factored out from this series.
It would be helpful for maintainers and reviewers to take a look at them.
Best Regards
Jungseok Lee
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 4/6] arm64: insn: add instruction decoders for ldp/stp and add/sub
2015-11-18 6:43 ` [PATCH v6 4/6] arm64: insn: add instruction decoders for ldp/stp and add/sub AKASHI Takahiro
@ 2015-12-08 18:15 ` Will Deacon
2015-12-08 23:17 ` Jungseok Lee
2015-12-10 7:10 ` AKASHI Takahiro
0 siblings, 2 replies; 23+ messages in thread
From: Will Deacon @ 2015-12-08 18:15 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 18, 2015 at 03:43:07PM +0900, AKASHI Takahiro wrote:
> A function prologue analyzer is a requisite for implementing stack tracer
> and getting better views of stack usages on arm64.
> To implement a function prologue analyzer, we have to be able to decode,
> at least, stp, add, sub and mov instructions.
>
> This patch adds decoders for those instructions, that are used solely
> by stack tracer for now, but generic enough for other uses.
>
> Reviewed-by: Jungseok Lee <jungseoklee85@gmail.com>
> Tested-by: Jungseok Lee <jungseoklee85@gmail.com>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/include/asm/insn.h | 18 ++++++++
> arch/arm64/kernel/insn.c | 102 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 120 insertions(+)
>
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 30e50eb..8d5c538 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -165,6 +165,8 @@ enum aarch64_insn_ldst_type {
> AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
> AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
> AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
> + AARCH64_INSN_LDST_LOAD_PAIR,
> + AARCH64_INSN_LDST_STORE_PAIR,
For consistency with the rest of this header, we should be calling these
AARCH64_INSN_LDST_{LOAD,STORE}_PAIR_REG_OFFSET...
> };
>
> enum aarch64_insn_adsb_type {
> @@ -225,6 +227,8 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
>
> __AARCH64_INSN_FUNCS(str_reg, 0x3FE0EC00, 0x38206800)
> __AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
> +__AARCH64_INSN_FUNCS(stp, 0x7FC00000, 0x29000000)
> +__AARCH64_INSN_FUNCS(ldp, 0x7FC00000, 0x29400000)
... and using stp_reg/ldp_reg here.
> __AARCH64_INSN_FUNCS(stp_post, 0x7FC00000, 0x28800000)
> __AARCH64_INSN_FUNCS(ldp_post, 0x7FC00000, 0x28C00000)
> __AARCH64_INSN_FUNCS(stp_pre, 0x7FC00000, 0x29800000)
> @@ -277,6 +281,7 @@ __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F)
> __AARCH64_INSN_FUNCS(br, 0xFFFFFC1F, 0xD61F0000)
> __AARCH64_INSN_FUNCS(blr, 0xFFFFFC1F, 0xD63F0000)
> __AARCH64_INSN_FUNCS(ret, 0xFFFFFC1F, 0xD65F0000)
> +__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F00E0)
Is this encoding correct? I ended up with 0xd69f03e0.
> #undef __AARCH64_INSN_FUNCS
>
> @@ -370,6 +375,19 @@ bool aarch32_insn_is_wide(u32 insn);
> u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
> u32 aarch32_insn_mcr_extract_opc2(u32 insn);
> u32 aarch32_insn_mcr_extract_crm(u32 insn);
> +int aarch64_insn_decode_add_sub_imm(u32 insn,
> + enum aarch64_insn_register *dst,
> + enum aarch64_insn_register *src,
> + int *imm,
> + enum aarch64_insn_variant *variant,
> + enum aarch64_insn_adsb_type *type);
> +int aarch64_insn_decode_load_store_pair(u32 insn,
> + enum aarch64_insn_register *reg1,
> + enum aarch64_insn_register *reg2,
> + enum aarch64_insn_register *base,
> + int *offset,
> + enum aarch64_insn_variant *variant,
> + enum aarch64_insn_ldst_type *type);
> #endif /* __ASSEMBLY__ */
>
> #endif /* __ASM_INSN_H */
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index c08b9ad..b56a66c 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -33,6 +33,7 @@
> #include <asm/insn.h>
>
> #define AARCH64_INSN_SF_BIT BIT(31)
> +#define AARCH64_INSN_S_BIT BIT(29)
> #define AARCH64_INSN_N_BIT BIT(22)
>
> static int aarch64_insn_encoding_class[] = {
> @@ -1141,3 +1142,104 @@ u32 aarch32_insn_mcr_extract_crm(u32 insn)
> {
> return insn & CRM_MASK;
> }
> +
> +enum aarch64_insn_register aarch64_insn_extract_reg_num(u32 insn,
> + enum aarch64_insn_register_type type)
Maybe call this aarch64_insn_decode_register? You're basically building
decoders for some of the *_encode_* functions we already have.
> +{
> + int shift;
> +
> + switch (type) {
> + case AARCH64_INSN_REGTYPE_RT:
> + case AARCH64_INSN_REGTYPE_RD:
> + shift = 0;
> + break;
> + case AARCH64_INSN_REGTYPE_RN:
> + shift = 5;
> + break;
> + case AARCH64_INSN_REGTYPE_RT2:
> + case AARCH64_INSN_REGTYPE_RA:
> + shift = 10;
> + break;
> + case AARCH64_INSN_REGTYPE_RM:
> + shift = 16;
> + break;
> + default:
> + pr_err("%s: unknown register type decoding %d\n", __func__,
> + type);
> + return ~0L;
> + }
This is largely a copy-paste from aarch64_insn_encode_register. Can you
either work with what we have or factor out the common parts, please?
> + return (insn & (GENMASK(4, 0) << shift)) >> shift;
> +}
> +
> +int aarch64_insn_decode_add_sub_imm(u32 insn,
This one could be aarch64_insn_extract_add_sub_imm, if you like.
Then extract is the converse of gen and decode is the converse of encode.
> + enum aarch64_insn_register *dst,
> + enum aarch64_insn_register *src,
> + int *imm,
> + enum aarch64_insn_variant *variant,
> + enum aarch64_insn_adsb_type *type)
> +{
> + if (aarch64_insn_is_add_imm(insn))
> + *type = ((insn) & AARCH64_INSN_S_BIT) ?
> + AARCH64_INSN_ADSB_ADD_SETFLAGS :
> + AARCH64_INSN_ADSB_ADD;
> + else if (aarch64_insn_is_sub_imm(insn))
> + *type = ((insn) & AARCH64_INSN_S_BIT) ?
> + AARCH64_INSN_ADSB_SUB_SETFLAGS :
> + AARCH64_INSN_ADSB_SUB;
> + else
> + return 0;
Why do we need to return 0 on error? -EINVAL might make more sense.
> +
> + *variant = (insn & AARCH64_INSN_SF_BIT) ? AARCH64_INSN_VARIANT_64BIT :
> + AARCH64_INSN_VARIANT_32BIT;
> +
> + *dst = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RD);
> +
> + *src = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
> +
> + /* TODO: ignore shilft field[23:22] */
I don't understand the TODO.
> + *imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_12, insn);
We use u64 for imm everywhere else.
> + return 1;
> +}
Similarly here, you're trying to implement the converse of
aarch64_insn_gen_add_sub_imm
> +
> +int aarch64_insn_decode_load_store_pair(u32 insn,
> + enum aarch64_insn_register *reg1,
> + enum aarch64_insn_register *reg2,
> + enum aarch64_insn_register *base,
> + int *offset,
> + enum aarch64_insn_variant *variant,
> + enum aarch64_insn_ldst_type *type)
> +{
> + int imm;
> +
> + if (aarch64_insn_is_stp(insn))
> + *type = AARCH64_INSN_LDST_STORE_PAIR;
> + else if (aarch64_insn_is_stp_post(insn))
> + *type = AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX;
> + else if (aarch64_insn_is_stp_pre(insn))
> + *type = AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX;
> + else if (aarch64_insn_is_ldp(insn))
> + *type = AARCH64_INSN_LDST_LOAD_PAIR;
> + else if (aarch64_insn_is_ldp_post(insn))
> + *type = AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX;
> + else if (aarch64_insn_is_ldp_pre(insn))
> + *type = AARCH64_INSN_LDST_LOAD_PAIR_PRE_INDEX;
> + else
> + return 0;
> +
> + *variant = (insn & AARCH64_INSN_S_BIT) ? AARCH64_INSN_VARIANT_64BIT :
> + AARCH64_INSN_VARIANT_32BIT;
> +
> + *reg1 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT);
> +
> + *reg2 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT2);
> +
> + *base = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
> +
> + imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_7, insn);
> + asm("sbfm %0, %0, 0, 6" : "+r" (imm));
What? Can't you write this in C?
Will
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 4/6] arm64: insn: add instruction decoders for ldp/stp and add/sub
2015-12-08 18:15 ` Will Deacon
@ 2015-12-08 23:17 ` Jungseok Lee
2015-12-10 7:10 ` AKASHI Takahiro
1 sibling, 0 replies; 23+ messages in thread
From: Jungseok Lee @ 2015-12-08 23:17 UTC (permalink / raw)
To: linux-arm-kernel
On Dec 9, 2015, at 3:15 AM, Will Deacon wrote:
Hi Will,
> On Wed, Nov 18, 2015 at 03:43:07PM +0900, AKASHI Takahiro wrote:
>> A function prologue analyzer is a requisite for implementing stack tracer
>> and getting better views of stack usages on arm64.
>> To implement a function prologue analyzer, we have to be able to decode,
>> at least, stp, add, sub and mov instructions.
>>
>> This patch adds decoders for those instructions, that are used solely
>> by stack tracer for now, but generic enough for other uses.
>>
>> Reviewed-by: Jungseok Lee <jungseoklee85@gmail.com>
>> Tested-by: Jungseok Lee <jungseoklee85@gmail.com>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>> arch/arm64/include/asm/insn.h | 18 ++++++++
>> arch/arm64/kernel/insn.c | 102 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 120 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index 30e50eb..8d5c538 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -165,6 +165,8 @@ enum aarch64_insn_ldst_type {
>> AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
>> AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
>> AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
>> + AARCH64_INSN_LDST_LOAD_PAIR,
>> + AARCH64_INSN_LDST_STORE_PAIR,
>
> For consistency with the rest of this header, we should be calling these
> AARCH64_INSN_LDST_{LOAD,STORE}_PAIR_REG_OFFSET...
>
>> };
>>
>> enum aarch64_insn_adsb_type {
>> @@ -225,6 +227,8 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
>>
>> __AARCH64_INSN_FUNCS(str_reg, 0x3FE0EC00, 0x38206800)
>> __AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
>> +__AARCH64_INSN_FUNCS(stp, 0x7FC00000, 0x29000000)
>> +__AARCH64_INSN_FUNCS(ldp, 0x7FC00000, 0x29400000)
>
> ... and using stp_reg/ldp_reg here.
>
>> __AARCH64_INSN_FUNCS(stp_post, 0x7FC00000, 0x28800000)
>> __AARCH64_INSN_FUNCS(ldp_post, 0x7FC00000, 0x28C00000)
>> __AARCH64_INSN_FUNCS(stp_pre, 0x7FC00000, 0x29800000)
>> @@ -277,6 +281,7 @@ __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F)
>> __AARCH64_INSN_FUNCS(br, 0xFFFFFC1F, 0xD61F0000)
>> __AARCH64_INSN_FUNCS(blr, 0xFFFFFC1F, 0xD63F0000)
>> __AARCH64_INSN_FUNCS(ret, 0xFFFFFC1F, 0xD65F0000)
>> +__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F00E0)
>
> Is this encoding correct? I ended up with 0xd69f03e0.
You're right. This is updated in [PATCH v6 6/6]. Please refer to [1] for
more details.
[1] http://www.spinics.net/lists/arm-kernel/msg462856.html
Best Regards
Jungseok Lee
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame()
2015-12-02 15:33 ` Jungseok Lee
@ 2015-12-10 6:33 ` AKASHI Takahiro
0 siblings, 0 replies; 23+ messages in thread
From: AKASHI Takahiro @ 2015-12-10 6:33 UTC (permalink / raw)
To: linux-arm-kernel
On 12/03/2015 12:33 AM, Jungseok Lee wrote:
> On Dec 2, 2015, at 10:22 PM, Will Deacon wrote:
>> On Wed, Nov 25, 2015 at 01:39:36PM +0900, AKASHI Takahiro wrote:
>>> On 11/24/2015 10:42 PM, Jungseok Lee wrote:
>>>> On Nov 18, 2015, at 3:43 PM, AKASHI Takahiro wrote:
>>>>> The change here doesn't make any difference in this patch, but is
>>>>> a preparation for later fixing a problem where stacktrace functions,
>>>>> unwind_frame() and walk_stackframe(), may return useless call stacks
>>>>> under function graph tracer.
>>>>
>>>> I'm aligned with the argument. The case cannot be handled correctly
>>>> without ret_stack[] of struct task_struct.
>>>
>>> Thanks. I will add some description about why we need 'tsk' in a commit
>>> message.
>>
>> Ok, so are you planning to repost this series?
>
> I think this function_graph changes could be factored out from this series.
> It would be helpful for maintainers and reviewers to take a look at them.
Yeah, now patch1, 2 and 3 are almost independent from patch 4, 5 and 6.
But I will submit them in one series, at least, for my next version.
Will, if this is inconvenient to you, let me know.
Thanks,
-Takahiro AKASHI
> Best Regards
> Jungseok Lee
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 4/6] arm64: insn: add instruction decoders for ldp/stp and add/sub
2015-12-08 18:15 ` Will Deacon
2015-12-08 23:17 ` Jungseok Lee
@ 2015-12-10 7:10 ` AKASHI Takahiro
1 sibling, 0 replies; 23+ messages in thread
From: AKASHI Takahiro @ 2015-12-10 7:10 UTC (permalink / raw)
To: linux-arm-kernel
Will,
I was back from my vacation.
On 12/09/2015 03:15 AM, Will Deacon wrote:
> On Wed, Nov 18, 2015 at 03:43:07PM +0900, AKASHI Takahiro wrote:
>> A function prologue analyzer is a requisite for implementing stack tracer
>> and getting better views of stack usages on arm64.
>> To implement a function prologue analyzer, we have to be able to decode,
>> at least, stp, add, sub and mov instructions.
>>
>> This patch adds decoders for those instructions, that are used solely
>> by stack tracer for now, but generic enough for other uses.
>>
>> Reviewed-by: Jungseok Lee <jungseoklee85@gmail.com>
>> Tested-by: Jungseok Lee <jungseoklee85@gmail.com>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>> arch/arm64/include/asm/insn.h | 18 ++++++++
>> arch/arm64/kernel/insn.c | 102 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 120 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index 30e50eb..8d5c538 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -165,6 +165,8 @@ enum aarch64_insn_ldst_type {
>> AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX,
>> AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX,
>> AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX,
>> + AARCH64_INSN_LDST_LOAD_PAIR,
>> + AARCH64_INSN_LDST_STORE_PAIR,
>
> For consistency with the rest of this header, we should be calling these
> AARCH64_INSN_LDST_{LOAD,STORE}_PAIR_REG_OFFSET...
OK.
(But taking arguments of registers and offset is also common to _PRE_INDEX and _POST_INDEX cases).
>> };
>>
>> enum aarch64_insn_adsb_type {
>> @@ -225,6 +227,8 @@ static __always_inline u32 aarch64_insn_get_##abbr##_value(void) \
>>
>> __AARCH64_INSN_FUNCS(str_reg, 0x3FE0EC00, 0x38206800)
>> __AARCH64_INSN_FUNCS(ldr_reg, 0x3FE0EC00, 0x38606800)
>> +__AARCH64_INSN_FUNCS(stp, 0x7FC00000, 0x29000000)
>> +__AARCH64_INSN_FUNCS(ldp, 0x7FC00000, 0x29400000)
>
> ... and using stp_reg/ldp_reg here.
Ditto.
>> __AARCH64_INSN_FUNCS(stp_post, 0x7FC00000, 0x28800000)
>> __AARCH64_INSN_FUNCS(ldp_post, 0x7FC00000, 0x28C00000)
>> __AARCH64_INSN_FUNCS(stp_pre, 0x7FC00000, 0x29800000)
>> @@ -277,6 +281,7 @@ __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F)
>> __AARCH64_INSN_FUNCS(br, 0xFFFFFC1F, 0xD61F0000)
>> __AARCH64_INSN_FUNCS(blr, 0xFFFFFC1F, 0xD63F0000)
>> __AARCH64_INSN_FUNCS(ret, 0xFFFFFC1F, 0xD65F0000)
>> +__AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F00E0)
>
> Is this encoding correct? I ended up with 0xd69f03e0.
Fixed it in v6.
>> #undef __AARCH64_INSN_FUNCS
>>
>> @@ -370,6 +375,19 @@ bool aarch32_insn_is_wide(u32 insn);
>> u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
>> u32 aarch32_insn_mcr_extract_opc2(u32 insn);
>> u32 aarch32_insn_mcr_extract_crm(u32 insn);
>> +int aarch64_insn_decode_add_sub_imm(u32 insn,
>> + enum aarch64_insn_register *dst,
>> + enum aarch64_insn_register *src,
>> + int *imm,
>> + enum aarch64_insn_variant *variant,
>> + enum aarch64_insn_adsb_type *type);
>> +int aarch64_insn_decode_load_store_pair(u32 insn,
>> + enum aarch64_insn_register *reg1,
>> + enum aarch64_insn_register *reg2,
>> + enum aarch64_insn_register *base,
>> + int *offset,
>> + enum aarch64_insn_variant *variant,
>> + enum aarch64_insn_ldst_type *type);
>> #endif /* __ASSEMBLY__ */
>>
>> #endif /* __ASM_INSN_H */
>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
>> index c08b9ad..b56a66c 100644
>> --- a/arch/arm64/kernel/insn.c
>> +++ b/arch/arm64/kernel/insn.c
>> @@ -33,6 +33,7 @@
>> #include <asm/insn.h>
>>
>> #define AARCH64_INSN_SF_BIT BIT(31)
>> +#define AARCH64_INSN_S_BIT BIT(29)
>> #define AARCH64_INSN_N_BIT BIT(22)
>>
>> static int aarch64_insn_encoding_class[] = {
>> @@ -1141,3 +1142,104 @@ u32 aarch32_insn_mcr_extract_crm(u32 insn)
>> {
>> return insn & CRM_MASK;
>> }
>> +
>> +enum aarch64_insn_register aarch64_insn_extract_reg_num(u32 insn,
>> + enum aarch64_insn_register_type type)
>
> Maybe call this aarch64_insn_decode_register? You're basically building
> decoders for some of the *_encode_* functions we already have.
OK, will fix it.
But please note that there are already some functions named
aarch32_insn_extract_reg_num() et al.
>> +{
>> + int shift;
>> +
>> + switch (type) {
>> + case AARCH64_INSN_REGTYPE_RT:
>> + case AARCH64_INSN_REGTYPE_RD:
>> + shift = 0;
>> + break;
>> + case AARCH64_INSN_REGTYPE_RN:
>> + shift = 5;
>> + break;
>> + case AARCH64_INSN_REGTYPE_RT2:
>> + case AARCH64_INSN_REGTYPE_RA:
>> + shift = 10;
>> + break;
>> + case AARCH64_INSN_REGTYPE_RM:
>> + shift = 16;
>> + break;
>> + default:
>> + pr_err("%s: unknown register type decoding %d\n", __func__,
>> + type);
>> + return ~0L;
>> + }
>
> This is largely a copy-paste from aarch64_insn_encode_register. Can you
> either work with what we have or factor out the common parts, please?
OK, will re-factor it.
>> + return (insn & (GENMASK(4, 0) << shift)) >> shift;
>> +}
>> +
>> +int aarch64_insn_decode_add_sub_imm(u32 insn,
>
> This one could be aarch64_insn_extract_add_sub_imm, if you like.
> Then extract is the converse of gen and decode is the converse of encode.
I agree, will change the name as you suggest.
But please see the comment above about aarch32_insn_extract_*.
>> + enum aarch64_insn_register *dst,
>> + enum aarch64_insn_register *src,
>> + int *imm,
>> + enum aarch64_insn_variant *variant,
>> + enum aarch64_insn_adsb_type *type)
>> +{
>> + if (aarch64_insn_is_add_imm(insn))
>> + *type = ((insn) & AARCH64_INSN_S_BIT) ?
>> + AARCH64_INSN_ADSB_ADD_SETFLAGS :
>> + AARCH64_INSN_ADSB_ADD;
>> + else if (aarch64_insn_is_sub_imm(insn))
>> + *type = ((insn) & AARCH64_INSN_S_BIT) ?
>> + AARCH64_INSN_ADSB_SUB_SETFLAGS :
>> + AARCH64_INSN_ADSB_SUB;
>> + else
>> + return 0;
>
> Why do we need to return 0 on error? -EINVAL might make more sense.
I think that, in most failure cases of *_gen_*, 0 will be returned.
See aarch64_insn_encode_immediate() and aach64_insn_encode_register().
But returning -EINVAL is also understandable. Will change so.
>> +
>> + *variant = (insn & AARCH64_INSN_SF_BIT) ? AARCH64_INSN_VARIANT_64BIT :
>> + AARCH64_INSN_VARIANT_32BIT;
>> +
>> + *dst = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RD);
>> +
>> + *src = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
>> +
>> + /* TODO: ignore shilft field[23:22] */
>
> I don't understand the TODO.
Haha, we don't have to interpret 'shift' in a function prologue usage,
but yes, will fix it for general cases.
>> + *imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_12, insn);
>
> We use u64 for imm everywhere else.
Not true.
See aarch64_insn_gen_add_sub_imm(), _bitfield() and _movewide().
>> + return 1;
>> +}
return 0;
> Similarly here, you're trying to implement the converse of
> aarch64_insn_gen_add_sub_imm
Will change the name.
>> +
>> +int aarch64_insn_decode_load_store_pair(u32 insn,
>> + enum aarch64_insn_register *reg1,
>> + enum aarch64_insn_register *reg2,
>> + enum aarch64_insn_register *base,
>> + int *offset,
>> + enum aarch64_insn_variant *variant,
>> + enum aarch64_insn_ldst_type *type)
>> +{
>> + int imm;
>> +
>> + if (aarch64_insn_is_stp(insn))
>> + *type = AARCH64_INSN_LDST_STORE_PAIR;
>> + else if (aarch64_insn_is_stp_post(insn))
>> + *type = AARCH64_INSN_LDST_STORE_PAIR_POST_INDEX;
>> + else if (aarch64_insn_is_stp_pre(insn))
>> + *type = AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX;
>> + else if (aarch64_insn_is_ldp(insn))
>> + *type = AARCH64_INSN_LDST_LOAD_PAIR;
>> + else if (aarch64_insn_is_ldp_post(insn))
>> + *type = AARCH64_INSN_LDST_LOAD_PAIR_POST_INDEX;
>> + else if (aarch64_insn_is_ldp_pre(insn))
>> + *type = AARCH64_INSN_LDST_LOAD_PAIR_PRE_INDEX;
>> + else
>> + return 0;
return -EINVAL;
>> + *variant = (insn & AARCH64_INSN_S_BIT) ? AARCH64_INSN_VARIANT_64BIT :
>> + AARCH64_INSN_VARIANT_32BIT;
>> +
>> + *reg1 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT);
>> +
>> + *reg2 = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RT2);
>> +
>> + *base = aarch64_insn_extract_reg_num(insn, AARCH64_INSN_REGTYPE_RN);
>> +
>> + imm = (int)aarch64_insn_decode_immediate(AARCH64_INSN_IMM_7, insn);
>> + asm("sbfm %0, %0, 0, 6" : "+r" (imm));
>
> What? Can't you write this in C?
OK, will use sign_extend64().
Thanks,
-Takahiro AKASHI
> Will
>
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2015-12-10 7:10 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-18 6:43 [PATCH v6 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 1/6] arm64: ftrace: modify a stack frame in a safe way AKASHI Takahiro
2015-11-24 14:22 ` Jungseok Lee
2015-11-18 6:43 ` [PATCH v6 2/6] arm64: pass a task parameter to unwind_frame() AKASHI Takahiro
2015-11-24 13:42 ` Jungseok Lee
2015-11-25 4:39 ` AKASHI Takahiro
2015-12-02 13:22 ` Will Deacon
2015-12-02 15:33 ` Jungseok Lee
2015-12-10 6:33 ` AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer AKASHI Takahiro
2015-11-24 13:37 ` Jungseok Lee
2015-11-25 5:29 ` AKASHI Takahiro
2015-11-25 11:48 ` Jungseok Lee
2015-11-26 3:05 ` AKASHI Takahiro
2015-11-26 14:05 ` Jungseok Lee
2015-11-18 6:43 ` [PATCH v6 4/6] arm64: insn: add instruction decoders for ldp/stp and add/sub AKASHI Takahiro
2015-12-08 18:15 ` Will Deacon
2015-12-08 23:17 ` Jungseok Lee
2015-12-10 7:10 ` AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 5/6] arm64: ftrace: add arch-specific stack tracer AKASHI Takahiro
2015-11-18 6:43 ` [PATCH v6 6/6] arm64: ftrace: add a test of function prologue analyzer AKASHI Takahiro
2015-11-24 13:50 ` Jungseok Lee
2015-11-25 5:33 ` AKASHI Takahiro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.