linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/6] arm64: ftrace: fix incorrect output from stack tracer
@ 2015-10-30  5:25 AKASHI Takahiro
  2015-10-30  5:25 ` [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by " AKASHI Takahiro
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: AKASHI Takahiro @ 2015-10-30  5:25 UTC (permalink / raw)
  To: linux-arm-kernel

This is the fourth patch series for fixing stack tracer on arm64.
The original issue was reported by Jungseok[1], and then I found more
issues[2].
(Steven, Jungseok, sorry for not replying to your comments directly.)

I address here all the issues and implement fixes described in [2] except
for interrupt-triggered problems(II-3) and leaf function(II-5).  Recent
discussions[3] about introducing a dedicated interrupt stack suggests that
we may avoid walking through from an interrupt stack to a process stack.
(So interrupt-stack patch is a prerequisite.)

Basically,
patch1 corresponds to the original issue.
patch2 is a proactive improvement of function_graph tracer. 
patch3 corresponds 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).

Each fix can be applied independently, but if patch4, 5 and 6 are
acceptable, patch1 is not necessary because patch7 replaces a default
stack tracer.

I tested the code with v4.3-rc7 + Jungseok's patch v5[4].

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-July/355920.html 
[3] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368003.html
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/378699.html

AKASHI Takahiro (6):
  arm64: ftrace: adjust callsite addresses examined by stack tracer
  arm64: ftrace: modify a stack frame in a safe way
  arm64: ftrace: fix a stack tracer's output under function graph
    tracer
  ftrace: allow arch-specific stack tracer
  arm64: insn: add instruction decoders for ldp/stp and add/sub
  arm64: ftrace: add arch-specific stack tracer

 arch/arm64/include/asm/ftrace.h     |    7 +-
 arch/arm64/include/asm/insn.h       |   18 +++
 arch/arm64/include/asm/stacktrace.h |    4 +
 arch/arm64/kernel/ftrace.c          |   79 +++++++++++--
 arch/arm64/kernel/insn.c            |  102 +++++++++++++++++
 arch/arm64/kernel/stacktrace.c      |  213 ++++++++++++++++++++++++++++++++++-
 include/linux/ftrace.h              |   17 +++
 kernel/trace/trace_stack.c          |   88 ++++++++-------
 8 files changed, 473 insertions(+), 55 deletions(-)

-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by stack tracer
  2015-10-30  5:25 [PATCH v4 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
@ 2015-10-30  5:25 ` AKASHI Takahiro
  2015-10-30 11:16   ` Will Deacon
  2015-10-30  5:25 ` [PATCH v4 2/6] arm64: ftrace: modify a stack frame in a safe way AKASHI Takahiro
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: AKASHI Takahiro @ 2015-10-30  5:25 UTC (permalink / raw)
  To: linux-arm-kernel

On arm64, no PC values returned by save_stack_trace() will match to LR
values saved in stack frames on a stack after the following commit:
    commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
As a result, the output from stack tracer will be messed up.

This patch introduces an arch-defined macro, FTRACE_STACK_FRAME_OFFSET,
so that check_stack() can handle this case correctly.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/ftrace.h |    5 +++--
 arch/arm64/kernel/stacktrace.c  |    7 ++++---
 include/linux/ftrace.h          |    7 +++++++
 kernel/trace/trace_stack.c      |    8 +++++---
 4 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index c5534fa..2b43e20 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -13,8 +13,9 @@
 
 #include <asm/insn.h>
 
-#define MCOUNT_ADDR		((unsigned long)_mcount)
-#define MCOUNT_INSN_SIZE	AARCH64_INSN_SIZE
+#define MCOUNT_ADDR			((unsigned long)_mcount)
+#define MCOUNT_INSN_SIZE		AARCH64_INSN_SIZE
+#define FTRACE_STACK_FRAME_OFFSET	AARCH64_INSN_SIZE
 
 #ifndef __ASSEMBLY__
 #include <linux/compat.h>
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 407991b..bc0689a 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -20,6 +20,7 @@
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 
+#include <asm/insn.h>
 #include <asm/stacktrace.h>
 
 /*
@@ -49,10 +50,10 @@ int notrace unwind_frame(struct stackframe *frame)
 	frame->sp = fp + 0x10;
 	frame->fp = *(unsigned long *)(fp);
 	/*
-	 * -4 here because we care about the PC at time of bl,
-	 * not where the return will go.
+	 * decrement PC by AARCH64_INSN_SIZE here because we care about
+	 * the PC at time of bl, not where the return will go.
 	 */
-	frame->pc = *(unsigned long *)(fp + 8) - 4;
+	frame->pc = *(unsigned long *)(fp + 8) - AARCH64_INSN_SIZE;
 
 	return 0;
 }
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 6cd8c0e..d77b195 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -263,6 +263,13 @@ static inline void ftrace_kill(void) { }
 #endif /* CONFIG_FUNCTION_TRACER */
 
 #ifdef CONFIG_STACK_TRACER
+/*
+ * the offset value to add to return address from save_stack_trace()
+ */
+#ifndef FTRACE_STACK_FRAME_OFFSET
+#define FTRACE_STACK_FRAME_OFFSET 0
+#endif
+
 extern int stack_tracer_enabled;
 int
 stack_trace_sysctl(struct ctl_table *table, int write,
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 8abf1ba..2e452e8 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -68,7 +68,7 @@ static inline void print_max_stack(void)
 static inline void
 check_stack(unsigned long ip, unsigned long *stack)
 {
-	unsigned long this_size, flags; unsigned long *p, *top, *start;
+	unsigned long this_size, flags; unsigned long *p, *top, *start, addr;
 	static int tracer_frame;
 	int frame_size = ACCESS_ONCE(tracer_frame);
 	int i, x;
@@ -115,7 +115,8 @@ check_stack(unsigned long ip, unsigned long *stack)
 
 	/* Skip over the overhead of the stack tracer itself */
 	for (i = 0; i < max_stack_trace.nr_entries; i++) {
-		if (stack_dump_trace[i] == ip)
+		addr = stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET;
+		if (addr == ip)
 			break;
 	}
 
@@ -143,7 +144,8 @@ check_stack(unsigned long ip, unsigned long *stack)
 		for (; p < top && i < max_stack_trace.nr_entries; p++) {
 			if (stack_dump_trace[i] == ULONG_MAX)
 				break;
-			if (*p == stack_dump_trace[i]) {
+			addr = stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET;
+			if (*p == addr) {
 				stack_dump_trace[x] = stack_dump_trace[i++];
 				this_size = stack_dump_index[x++] =
 					(top - p) * sizeof(unsigned long);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 2/6] arm64: ftrace: modify a stack frame in a safe way
  2015-10-30  5:25 [PATCH v4 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
  2015-10-30  5:25 ` [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by " AKASHI Takahiro
@ 2015-10-30  5:25 ` AKASHI Takahiro
  2015-10-30  5:25 ` [PATCH v4 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer AKASHI Takahiro
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: AKASHI Takahiro @ 2015-10-30  5:25 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] 17+ messages in thread

* [PATCH v4 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer
  2015-10-30  5:25 [PATCH v4 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
  2015-10-30  5:25 ` [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by " AKASHI Takahiro
  2015-10-30  5:25 ` [PATCH v4 2/6] arm64: ftrace: modify a stack frame in a safe way AKASHI Takahiro
@ 2015-10-30  5:25 ` AKASHI Takahiro
  2015-11-01  8:00   ` Jungseok Lee
  2015-10-30  5:25 ` [PATCH v4 4/6] ftrace: allow arch-specific stack tracer AKASHI Takahiro
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: AKASHI Takahiro @ 2015-10-30  5:25 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 stack tracer's output.

This patch replaces such entries with originals values preserved in
current->ret_stack[].

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/ftrace.h |    2 ++
 arch/arm64/kernel/stacktrace.c  |   21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 2b43e20..b7d597c 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -29,6 +29,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/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index bc0689a..631c49d 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>
 
@@ -78,6 +79,9 @@ struct stack_trace_data {
 	struct stack_trace *trace;
 	unsigned int no_sched_functions;
 	unsigned int skip;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	unsigned int ret_stack_index;
+#endif
 };
 
 static int save_trace(struct stackframe *frame, void *d)
@@ -86,6 +90,20 @@ static int save_trace(struct stackframe *frame, void *d)
 	struct stack_trace *trace = data->trace;
 	unsigned long addr = frame->pc;
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) {
+		/*
+		 * 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 = addr =
+			current->ret_stack[data->ret_stack_index--].ret
+							- AARCH64_INSN_SIZE;
+	}
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 	if (data->no_sched_functions && in_sched_functions(addr))
 		return 0;
 	if (data->skip) {
@@ -105,6 +123,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 
 	data.trace = trace;
 	data.skip = trace->skip;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	data.ret_stack_index = current->curr_ret_stack;
+#endif
 
 	if (tsk != current) {
 		data.no_sched_functions = 1;
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 4/6] ftrace: allow arch-specific stack tracer
  2015-10-30  5:25 [PATCH v4 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
                   ` (2 preceding siblings ...)
  2015-10-30  5:25 ` [PATCH v4 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer AKASHI Takahiro
@ 2015-10-30  5:25 ` AKASHI Takahiro
  2015-10-30  5:25 ` [PATCH v4 5/6] arm64: insn: add instruction decoders for ldp/stp and add/sub AKASHI Takahiro
  2015-10-30  5:25 ` [PATCH v4 6/6] arm64: ftrace: add arch-specific stack tracer AKASHI Takahiro
  5 siblings, 0 replies; 17+ messages in thread
From: AKASHI Takahiro @ 2015-10-30  5:25 UTC (permalink / raw)
  To: linux-arm-kernel

A stack frame may be used in a different way depending on cpu architecture.
Thus it is not always appropriate to slurp the stack contents, as current
check_stack() does, in order to calcurate a stack index (height) at a given
function call. At least not on arm64.
In addition, there is a possibility that we will mistakenly detect a stale
stack frame which has not been overwritten.

This patch makes check_stack() a weak function so as to later implement
arch-specific version.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/linux/ftrace.h     |   10 ++++++
 kernel/trace/trace_stack.c |   80 ++++++++++++++++++++++++--------------------
 2 files changed, 53 insertions(+), 37 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index d77b195..c304650 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -270,7 +270,17 @@ static inline void ftrace_kill(void) { }
 #define FTRACE_STACK_FRAME_OFFSET 0
 #endif
 
+#define STACK_TRACE_ENTRIES 500
+
+struct stack_trace;
+
+extern unsigned stack_trace_index[];
+extern struct stack_trace stack_trace_max;
+extern unsigned long stack_trace_max_size;
+extern arch_spinlock_t max_stack_lock;
+
 extern int stack_tracer_enabled;
+void stack_trace_print(void);
 int
 stack_trace_sysctl(struct ctl_table *table, int write,
 		   void __user *buffer, size_t *lenp,
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 2e452e8..40f3368 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -16,24 +16,22 @@
 
 #include "trace.h"
 
-#define STACK_TRACE_ENTRIES 500
-
 static unsigned long stack_dump_trace[STACK_TRACE_ENTRIES+1] =
 	 { [0 ... (STACK_TRACE_ENTRIES)] = ULONG_MAX };
-static unsigned stack_dump_index[STACK_TRACE_ENTRIES];
+unsigned stack_trace_index[STACK_TRACE_ENTRIES];
 
 /*
  * Reserve one entry for the passed in ip. This will allow
  * us to remove most or all of the stack size overhead
  * added by the stack tracer itself.
  */
-static struct stack_trace max_stack_trace = {
+struct stack_trace stack_trace_max = {
 	.max_entries		= STACK_TRACE_ENTRIES - 1,
 	.entries		= &stack_dump_trace[0],
 };
 
-static unsigned long max_stack_size;
-static arch_spinlock_t max_stack_lock =
+unsigned long stack_trace_max_size;
+arch_spinlock_t max_stack_lock =
 	(arch_spinlock_t)__ARCH_SPIN_LOCK_UNLOCKED;
 
 static DEFINE_PER_CPU(int, trace_active);
@@ -42,30 +40,38 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
 int stack_tracer_enabled;
 static int last_stack_tracer_enabled;
 
-static inline void print_max_stack(void)
+void stack_trace_print(void)
 {
 	long i;
 	int size;
 
 	pr_emerg("        Depth    Size   Location    (%d entries)\n"
 			   "        -----    ----   --------\n",
-			   max_stack_trace.nr_entries);
+			   stack_trace_max.nr_entries);
 
-	for (i = 0; i < max_stack_trace.nr_entries; i++) {
+	for (i = 0; i < stack_trace_max.nr_entries; i++) {
 		if (stack_dump_trace[i] == ULONG_MAX)
 			break;
-		if (i+1 == max_stack_trace.nr_entries ||
+		if (i+1 == stack_trace_max.nr_entries ||
 				stack_dump_trace[i+1] == ULONG_MAX)
-			size = stack_dump_index[i];
+			size = stack_trace_index[i];
 		else
-			size = stack_dump_index[i] - stack_dump_index[i+1];
+			size = stack_trace_index[i] - stack_trace_index[i+1];
 
-		pr_emerg("%3ld) %8d   %5d   %pS\n", i, stack_dump_index[i],
+		pr_emerg("%3ld) %8d   %5d   %pS\n", i, stack_trace_index[i],
 				size, (void *)stack_dump_trace[i]);
 	}
 }
 
-static inline void
+/*
+ * When arch-specific code overides this function, the following
+ * data should be filled up, assuming max_stack_lock is held to
+ * prevent concurrent updates.
+ *     stack_trace_index[]
+ *     stack_trace_max
+ *     stack_trace_max_size
+ */
+void __weak
 check_stack(unsigned long ip, unsigned long *stack)
 {
 	unsigned long this_size, flags; unsigned long *p, *top, *start, addr;
@@ -78,7 +84,7 @@ check_stack(unsigned long ip, unsigned long *stack)
 	/* Remove the frame of the tracer */
 	this_size -= frame_size;
 
-	if (this_size <= max_stack_size)
+	if (this_size <= stack_trace_max_size)
 		return;
 
 	/* we do not handle interrupt stacks yet */
@@ -103,18 +109,18 @@ check_stack(unsigned long ip, unsigned long *stack)
 		this_size -= tracer_frame;
 
 	/* a race could have already updated it */
-	if (this_size <= max_stack_size)
+	if (this_size <= stack_trace_max_size)
 		goto out;
 
-	max_stack_size = this_size;
+	stack_trace_max_size = this_size;
 
-	max_stack_trace.nr_entries = 0;
-	max_stack_trace.skip = 3;
+	stack_trace_max.nr_entries = 0;
+	stack_trace_max.skip = 3;
 
-	save_stack_trace(&max_stack_trace);
+	save_stack_trace(&stack_trace_max);
 
 	/* Skip over the overhead of the stack tracer itself */
-	for (i = 0; i < max_stack_trace.nr_entries; i++) {
+	for (i = 0; i < stack_trace_max.nr_entries; i++) {
 		addr = stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET;
 		if (addr == ip)
 			break;
@@ -135,19 +141,19 @@ check_stack(unsigned long ip, unsigned long *stack)
 	 * loop will only happen once. This code only takes place
 	 * on a new max, so it is far from a fast path.
 	 */
-	while (i < max_stack_trace.nr_entries) {
+	while (i < stack_trace_max.nr_entries) {
 		int found = 0;
 
-		stack_dump_index[x] = this_size;
+		stack_trace_index[x] = this_size;
 		p = start;
 
-		for (; p < top && i < max_stack_trace.nr_entries; p++) {
+		for (; p < top && i < stack_trace_max.nr_entries; p++) {
 			if (stack_dump_trace[i] == ULONG_MAX)
 				break;
 			addr = stack_dump_trace[i] + FTRACE_STACK_FRAME_OFFSET;
 			if (*p == addr) {
 				stack_dump_trace[x] = stack_dump_trace[i++];
-				this_size = stack_dump_index[x++] =
+				this_size = stack_trace_index[x++] =
 					(top - p) * sizeof(unsigned long);
 				found = 1;
 				/* Start the search from here */
@@ -162,7 +168,7 @@ check_stack(unsigned long ip, unsigned long *stack)
 				if (unlikely(!tracer_frame)) {
 					tracer_frame = (p - stack) *
 						sizeof(unsigned long);
-					max_stack_size -= tracer_frame;
+					stack_trace_max_size -= tracer_frame;
 				}
 			}
 		}
@@ -171,12 +177,12 @@ check_stack(unsigned long ip, unsigned long *stack)
 			i++;
 	}
 
-	max_stack_trace.nr_entries = x;
+	stack_trace_max.nr_entries = x;
 	for (; x < i; x++)
 		stack_dump_trace[x] = ULONG_MAX;
 
 	if (task_stack_end_corrupted(current)) {
-		print_max_stack();
+		stack_trace_print();
 		BUG();
 	}
 
@@ -275,7 +281,7 @@ __next(struct seq_file *m, loff_t *pos)
 {
 	long n = *pos - 1;
 
-	if (n > max_stack_trace.nr_entries || stack_dump_trace[n] == ULONG_MAX)
+	if (n > stack_trace_max.nr_entries || stack_dump_trace[n] == ULONG_MAX)
 		return NULL;
 
 	m->private = (void *)n;
@@ -345,9 +351,9 @@ static int t_show(struct seq_file *m, void *v)
 		seq_printf(m, "        Depth    Size   Location"
 			   "    (%d entries)\n"
 			   "        -----    ----   --------\n",
-			   max_stack_trace.nr_entries);
+			   stack_trace_max.nr_entries);
 
-		if (!stack_tracer_enabled && !max_stack_size)
+		if (!stack_tracer_enabled && !stack_trace_max_size)
 			print_disabled(m);
 
 		return 0;
@@ -355,17 +361,17 @@ static int t_show(struct seq_file *m, void *v)
 
 	i = *(long *)v;
 
-	if (i >= max_stack_trace.nr_entries ||
+	if (i >= stack_trace_max.nr_entries ||
 	    stack_dump_trace[i] == ULONG_MAX)
 		return 0;
 
-	if (i+1 == max_stack_trace.nr_entries ||
+	if (i+1 == stack_trace_max.nr_entries ||
 	    stack_dump_trace[i+1] == ULONG_MAX)
-		size = stack_dump_index[i];
+		size = stack_trace_index[i];
 	else
-		size = stack_dump_index[i] - stack_dump_index[i+1];
+		size = stack_trace_index[i] - stack_trace_index[i+1];
 
-	seq_printf(m, "%3ld) %8d   %5d   ", i, stack_dump_index[i], size);
+	seq_printf(m, "%3ld) %8d   %5d   ", i, stack_trace_index[i], size);
 
 	trace_lookup_stack(m, i);
 
@@ -455,7 +461,7 @@ static __init int stack_trace_init(void)
 		return 0;
 
 	trace_create_file("stack_max_size", 0644, d_tracer,
-			&max_stack_size, &stack_max_size_fops);
+			&stack_trace_max_size, &stack_max_size_fops);
 
 	trace_create_file("stack_trace", 0444, d_tracer,
 			NULL, &stack_trace_fops);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 5/6] arm64: insn: add instruction decoders for ldp/stp and add/sub
  2015-10-30  5:25 [PATCH v4 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
                   ` (3 preceding siblings ...)
  2015-10-30  5:25 ` [PATCH v4 4/6] ftrace: allow arch-specific stack tracer AKASHI Takahiro
@ 2015-10-30  5:25 ` AKASHI Takahiro
  2015-10-30  5:25 ` [PATCH v4 6/6] arm64: ftrace: add arch-specific stack tracer AKASHI Takahiro
  5 siblings, 0 replies; 17+ messages in thread
From: AKASHI Takahiro @ 2015-10-30  5:25 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.

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] 17+ messages in thread

* [PATCH v4 6/6] arm64: ftrace: add arch-specific stack tracer
  2015-10-30  5:25 [PATCH v4 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
                   ` (4 preceding siblings ...)
  2015-10-30  5:25 ` [PATCH v4 5/6] arm64: insn: add instruction decoders for ldp/stp and add/sub AKASHI Takahiro
@ 2015-10-30  5:25 ` AKASHI Takahiro
  2015-11-01  8:30   ` Jungseok Lee
  2015-11-04  8:42   ` yalin wang
  5 siblings, 2 replies; 17+ messages in thread
From: AKASHI Takahiro @ 2015-10-30  5:25 UTC (permalink / raw)
  To: linux-arm-kernel

Stack tracer on arm64, 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.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 arch/arm64/include/asm/stacktrace.h |    4 +
 arch/arm64/kernel/ftrace.c          |   68 +++++++++++++
 arch/arm64/kernel/stacktrace.c      |  185 ++++++++++++++++++++++++++++++++++-
 3 files changed, 254 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 7318f6d..47b4832 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -25,5 +25,9 @@ struct stackframe {
 extern int unwind_frame(struct stackframe *frame);
 extern void walk_stackframe(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..e8afd42 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,69 @@ 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(&max_stack_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++) {
+		unsigned long addr;
+
+		addr = stack_trace_max.entries[i] + FTRACE_STACK_FRAME_OFFSET;
+		if (addr == 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(&max_stack_lock);
+	local_irq_restore(flags);
+}
+#endif /* CONFIG_STACK_TRACER */
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 631c49d..1b293fe 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -24,6 +24,144 @@
 #include <asm/insn.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_DYNAMIC_FTRACE
+	if (addr == (u32 *)ftrace_call)
+		addr = (u32 *)ftrace_caller;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	else if (addr == (u32 *)ftrace_graph_caller)
+#ifdef CONFIG_DYNAMIC_FTRACE
+		addr = (u32 *)ftrace_caller;
+#else
+		addr = (u32 *)_mcount;
+#endif
+#endif
+#endif
+
+	insn = *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,
+					&reg1, &reg2, &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 = *addr;
+	}
+
+out:
+	return pos;
+}
+#endif
+
 /*
  * AArch64 PCS assigns the frame pointer to x29.
  *
@@ -82,6 +220,9 @@ struct stack_trace_data {
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	unsigned int ret_stack_index;
 #endif
+#ifdef CONFIG_STACK_TRACER
+	unsigned long *sp;
+#endif
 };
 
 static int save_trace(struct stackframe *frame, void *d)
@@ -111,12 +252,33 @@ 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;
@@ -126,6 +288,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	data.ret_stack_index = current->curr_ret_stack;
 #endif
+#ifdef CONFIG_STACK_TRACER
+	data.sp = stack_dump_sp;
+#endif
 
 	if (tsk != current) {
 		data.no_sched_functions = 1;
@@ -136,7 +301,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));
 	}
 
 	walk_stackframe(&frame, save_trace, &data);
@@ -144,9 +310,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] 17+ messages in thread

* [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by stack tracer
  2015-10-30  5:25 ` [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by " AKASHI Takahiro
@ 2015-10-30 11:16   ` Will Deacon
  2015-11-02 18:20     ` Steven Rostedt
  0 siblings, 1 reply; 17+ messages in thread
From: Will Deacon @ 2015-10-30 11:16 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Oct 30, 2015 at 02:25:36PM +0900, AKASHI Takahiro wrote:
> On arm64, no PC values returned by save_stack_trace() will match to LR
> values saved in stack frames on a stack after the following commit:
>     commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
> As a result, the output from stack tracer will be messed up.

FWIW, we've decided to revert that patch for the moment. We're the only
architecture making that kind of adjustment, so we should fix that before
building on top of it.

Will

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer
  2015-10-30  5:25 ` [PATCH v4 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer AKASHI Takahiro
@ 2015-11-01  8:00   ` Jungseok Lee
  2015-11-04  7:49     ` AKASHI Takahiro
  0 siblings, 1 reply; 17+ messages in thread
From: Jungseok Lee @ 2015-11-01  8:00 UTC (permalink / raw)
  To: linux-arm-kernel

On Oct 30, 2015, at 2:25 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 stack tracer's output.
> 
> This patch replaces such entries with originals values preserved in
> current->ret_stack[].
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/include/asm/ftrace.h |    2 ++
> arch/arm64/kernel/stacktrace.c  |   21 +++++++++++++++++++++
> 2 files changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 2b43e20..b7d597c 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -29,6 +29,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/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index bc0689a..631c49d 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>
> 
> @@ -78,6 +79,9 @@ struct stack_trace_data {
> 	struct stack_trace *trace;
> 	unsigned int no_sched_functions;
> 	unsigned int skip;
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	unsigned int ret_stack_index;
> +#endif
> };
> 
> static int save_trace(struct stackframe *frame, void *d)
> @@ -86,6 +90,20 @@ static int save_trace(struct stackframe *frame, void *d)
> 	struct stack_trace *trace = data->trace;
> 	unsigned long addr = frame->pc;
> 
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) {
> +		/*
> +		 * 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 = addr =
> +			current->ret_stack[data->ret_stack_index--].ret
> +							- AARCH64_INSN_SIZE;
> +	}
> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +

This hunk would be affected as the commit, "ARM64: unwind: Fix PC calculation",
e306dfd0, has been reverted.

Best Regards
Jungseok Lee

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 6/6] arm64: ftrace: add arch-specific stack tracer
  2015-10-30  5:25 ` [PATCH v4 6/6] arm64: ftrace: add arch-specific stack tracer AKASHI Takahiro
@ 2015-11-01  8:30   ` Jungseok Lee
  2015-11-04  8:01     ` AKASHI Takahiro
  2015-11-04  8:42   ` yalin wang
  1 sibling, 1 reply; 17+ messages in thread
From: Jungseok Lee @ 2015-11-01  8:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Oct 30, 2015, at 2:25 PM, AKASHI Takahiro wrote:

Hi Akashi,

> Stack tracer on arm64, 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.

Can I get an idea on how to test the function prologue analyzer? It pretty
tough to compare stack trace data with objdump one. Is there an easier way
to observe this enhancement without objdump?

Best Regards
Jungseok Lee

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by stack tracer
  2015-10-30 11:16   ` Will Deacon
@ 2015-11-02 18:20     ` Steven Rostedt
  2015-11-02 18:29       ` Mark Rutland
  0 siblings, 1 reply; 17+ messages in thread
From: Steven Rostedt @ 2015-11-02 18:20 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, 30 Oct 2015 11:16:19 +0000
Will Deacon <will.deacon@arm.com> wrote:

> On Fri, Oct 30, 2015 at 02:25:36PM +0900, AKASHI Takahiro wrote:
> > On arm64, no PC values returned by save_stack_trace() will match to LR
> > values saved in stack frames on a stack after the following commit:
> >     commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
> > As a result, the output from stack tracer will be messed up.
> 
> FWIW, we've decided to revert that patch for the moment. We're the only
> architecture making that kind of adjustment, so we should fix that before
> building on top of it.
> 
>

What is the status of this patch set. I'm currently pulling in last
minute patches for 4.3 and should the ftrace patch in this series be
applied? (I still need to review it too).

-- Steve

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by stack tracer
  2015-11-02 18:20     ` Steven Rostedt
@ 2015-11-02 18:29       ` Mark Rutland
  2015-11-02 18:41         ` Will Deacon
  0 siblings, 1 reply; 17+ messages in thread
From: Mark Rutland @ 2015-11-02 18:29 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 02, 2015 at 01:20:43PM -0500, Steven Rostedt wrote:
> On Fri, 30 Oct 2015 11:16:19 +0000
> Will Deacon <will.deacon@arm.com> wrote:
> 
> > On Fri, Oct 30, 2015 at 02:25:36PM +0900, AKASHI Takahiro wrote:
> > > On arm64, no PC values returned by save_stack_trace() will match to LR
> > > values saved in stack frames on a stack after the following commit:
> > >     commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
> > > As a result, the output from stack tracer will be messed up.
> > 
> > FWIW, we've decided to revert that patch for the moment. We're the only
> > architecture making that kind of adjustment, so we should fix that before
> > building on top of it.
> > 
> >
> 
> What is the status of this patch set. I'm currently pulling in last
> minute patches for 4.3 and should the ftrace patch in this series be
> applied? (I still need to review it too).

The revert Will mentioned is in v4.3 (see commit 9702970c7bd3e2d6), so
this series needs a respin to account for that.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by stack tracer
  2015-11-02 18:29       ` Mark Rutland
@ 2015-11-02 18:41         ` Will Deacon
  2015-11-02 18:43           ` Mark Rutland
  0 siblings, 1 reply; 17+ messages in thread
From: Will Deacon @ 2015-11-02 18:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 02, 2015 at 06:29:28PM +0000, Mark Rutland wrote:
> On Mon, Nov 02, 2015 at 01:20:43PM -0500, Steven Rostedt wrote:
> > On Fri, 30 Oct 2015 11:16:19 +0000
> > Will Deacon <will.deacon@arm.com> wrote:
> > 
> > > On Fri, Oct 30, 2015 at 02:25:36PM +0900, AKASHI Takahiro wrote:
> > > > On arm64, no PC values returned by save_stack_trace() will match to LR
> > > > values saved in stack frames on a stack after the following commit:
> > > >     commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
> > > > As a result, the output from stack tracer will be messed up.
> > > 
> > > FWIW, we've decided to revert that patch for the moment. We're the only
> > > architecture making that kind of adjustment, so we should fix that before
> > > building on top of it.
> > > 
> > >
> > 
> > What is the status of this patch set. I'm currently pulling in last
> > minute patches for 4.3 and should the ftrace patch in this series be
> > applied? (I still need to review it too).
> 
> The revert Will mentioned is in v4.3 (see commit 9702970c7bd3e2d6), so
> this series needs a respin to account for that.

Right, but we're talking about the core ftrace change in patch 4, which
is (afaict) independent of the commit you mention above.

Will

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by stack tracer
  2015-11-02 18:41         ` Will Deacon
@ 2015-11-02 18:43           ` Mark Rutland
  0 siblings, 0 replies; 17+ messages in thread
From: Mark Rutland @ 2015-11-02 18:43 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Nov 02, 2015 at 06:41:24PM +0000, Will Deacon wrote:
> On Mon, Nov 02, 2015 at 06:29:28PM +0000, Mark Rutland wrote:
> > On Mon, Nov 02, 2015 at 01:20:43PM -0500, Steven Rostedt wrote:
> > > On Fri, 30 Oct 2015 11:16:19 +0000
> > > Will Deacon <will.deacon@arm.com> wrote:
> > > 
> > > > On Fri, Oct 30, 2015 at 02:25:36PM +0900, AKASHI Takahiro wrote:
> > > > > On arm64, no PC values returned by save_stack_trace() will match to LR
> > > > > values saved in stack frames on a stack after the following commit:
> > > > >     commit e306dfd06fcb ("ARM64: unwind: Fix PC calculation")
> > > > > As a result, the output from stack tracer will be messed up.
> > > > 
> > > > FWIW, we've decided to revert that patch for the moment. We're the only
> > > > architecture making that kind of adjustment, so we should fix that before
> > > > building on top of it.
> > > > 
> > > >
> > > 
> > > What is the status of this patch set. I'm currently pulling in last
> > > minute patches for 4.3 and should the ftrace patch in this series be
> > > applied? (I still need to review it too).
> > 
> > The revert Will mentioned is in v4.3 (see commit 9702970c7bd3e2d6), so
> > this series needs a respin to account for that.
> 
> Right, but we're talking about the core ftrace change in patch 4, which
> is (afaict) independent of the commit you mention above.

Ah; I don't know on that front, sorry for the noise.

Mark.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer
  2015-11-01  8:00   ` Jungseok Lee
@ 2015-11-04  7:49     ` AKASHI Takahiro
  0 siblings, 0 replies; 17+ messages in thread
From: AKASHI Takahiro @ 2015-11-04  7:49 UTC (permalink / raw)
  To: linux-arm-kernel

On 11/01/2015 05:00 PM, Jungseok Lee wrote:
> On Oct 30, 2015, at 2:25 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 stack tracer's output.
>>
>> This patch replaces such entries with originals values preserved in
>> current->ret_stack[].
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>> arch/arm64/include/asm/ftrace.h |    2 ++
>> arch/arm64/kernel/stacktrace.c  |   21 +++++++++++++++++++++
>> 2 files changed, 23 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
>> index 2b43e20..b7d597c 100644
>> --- a/arch/arm64/include/asm/ftrace.h
>> +++ b/arch/arm64/include/asm/ftrace.h
>> @@ -29,6 +29,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/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index bc0689a..631c49d 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>
>>
>> @@ -78,6 +79,9 @@ struct stack_trace_data {
>> 	struct stack_trace *trace;
>> 	unsigned int no_sched_functions;
>> 	unsigned int skip;
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> +	unsigned int ret_stack_index;
>> +#endif
>> };
>>
>> static int save_trace(struct stackframe *frame, void *d)
>> @@ -86,6 +90,20 @@ static int save_trace(struct stackframe *frame, void *d)
>> 	struct stack_trace *trace = data->trace;
>> 	unsigned long addr = frame->pc;
>>
>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> +	if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) {
>> +		/*
>> +		 * 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 = addr =
>> +			current->ret_stack[data->ret_stack_index--].ret
>> +							- AARCH64_INSN_SIZE;
>> +	}
>> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>> +
>
> This hunk would be affected as the commit, "ARM64: unwind: Fix PC calculation",
> e306dfd0, has been reverted.

Yeah, we don't need apply this patch any more.
The other patches in the series will not be affected.

Thanks,
-Takahiro AKASHI


> Best Regards
> Jungseok Lee
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 6/6] arm64: ftrace: add arch-specific stack tracer
  2015-11-01  8:30   ` Jungseok Lee
@ 2015-11-04  8:01     ` AKASHI Takahiro
  0 siblings, 0 replies; 17+ messages in thread
From: AKASHI Takahiro @ 2015-11-04  8:01 UTC (permalink / raw)
  To: linux-arm-kernel

Jungseok,

On 11/01/2015 05:30 PM, Jungseok Lee wrote:
> On Oct 30, 2015, at 2:25 PM, AKASHI Takahiro wrote:
>
> Hi Akashi,
>
>> Stack tracer on arm64, 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.
>
> Can I get an idea on how to test the function prologue analyzer? It pretty
> tough to compare stack trace data with objdump one. Is there an easier way
> to observe this enhancement without objdump?

It is quite difficult to give an evidence of the correctness of my function
prologue analyzer. I only checked the outputs from stack tracer, one by one
(every function), by comparing it against its disassembled code.

Thanks,
-Takahiro AKASHI

> Best Regards
> Jungseok Lee
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 6/6] arm64: ftrace: add arch-specific stack tracer
  2015-10-30  5:25 ` [PATCH v4 6/6] arm64: ftrace: add arch-specific stack tracer AKASHI Takahiro
  2015-11-01  8:30   ` Jungseok Lee
@ 2015-11-04  8:42   ` yalin wang
  1 sibling, 0 replies; 17+ messages in thread
From: yalin wang @ 2015-11-04  8:42 UTC (permalink / raw)
  To: linux-arm-kernel


> On Oct 30, 2015, at 13:25, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> 
> Stack tracer on arm64, 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.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> arch/arm64/include/asm/stacktrace.h |    4 +
> arch/arm64/kernel/ftrace.c          |   68 +++++++++++++
> arch/arm64/kernel/stacktrace.c      |  185 ++++++++++++++++++++++++++++++++++-
> 3 files changed, 254 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 7318f6d..47b4832 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -25,5 +25,9 @@ struct stackframe {
> extern int unwind_frame(struct stackframe *frame);
> extern void walk_stackframe(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..e8afd42 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,69 @@ 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(&max_stack_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++) {
> +		unsigned long addr;
> +
> +		addr = stack_trace_max.entries[i] + FTRACE_STACK_FRAME_OFFSET;
> +		if (addr == 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(&max_stack_lock);
> +	local_irq_restore(flags);
> +}
> +#endif /* CONFIG_STACK_TRACER */
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 631c49d..1b293fe 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -24,6 +24,144 @@
> #include <asm/insn.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_DYNAMIC_FTRACE
> +	if (addr == (u32 *)ftrace_call)
> +		addr = (u32 *)ftrace_caller;
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	else if (addr == (u32 *)ftrace_graph_caller)
> +#ifdef CONFIG_DYNAMIC_FTRACE
> +		addr = (u32 *)ftrace_caller;
> +#else
> +		addr = (u32 *)_mcount;
> +#endif
> +#endif
> +#endif
> +
> +	insn = *addr;

you should use aarch64_insn_read() or inn = le32_to_cpu(*addr)
here ,  in case the kernel is build as big endian .



> +	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,
> +					&reg1, &reg2, &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 = *addr;
> +	}
> +
> +out:
> +	return pos;
> +}
> +#endif
> +
> /*
>  * AArch64 PCS assigns the frame pointer to x29.
>  *
> @@ -82,6 +220,9 @@ struct stack_trace_data {
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> 	unsigned int ret_stack_index;
> #endif
> +#ifdef CONFIG_STACK_TRACER
> +	unsigned long *sp;
> +#endif
> };
> 
> static int save_trace(struct stackframe *frame, void *d)
> @@ -111,12 +252,33 @@ 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;
> @@ -126,6 +288,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> 	data.ret_stack_index = current->curr_ret_stack;
> #endif
> +#ifdef CONFIG_STACK_TRACER
> +	data.sp = stack_dump_sp;
> +#endif
> 
> 	if (tsk != current) {
> 		data.no_sched_functions = 1;
> @@ -136,7 +301,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));
> 	}
> 
> 	walk_stackframe(&frame, save_trace, &data);
> @@ -144,9 +310,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2015-11-04  8:42 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-30  5:25 [PATCH v4 0/6] arm64: ftrace: fix incorrect output from stack tracer AKASHI Takahiro
2015-10-30  5:25 ` [PATCH v4 1/6] arm64: ftrace: adjust callsite addresses examined by " AKASHI Takahiro
2015-10-30 11:16   ` Will Deacon
2015-11-02 18:20     ` Steven Rostedt
2015-11-02 18:29       ` Mark Rutland
2015-11-02 18:41         ` Will Deacon
2015-11-02 18:43           ` Mark Rutland
2015-10-30  5:25 ` [PATCH v4 2/6] arm64: ftrace: modify a stack frame in a safe way AKASHI Takahiro
2015-10-30  5:25 ` [PATCH v4 3/6] arm64: ftrace: fix a stack tracer's output under function graph tracer AKASHI Takahiro
2015-11-01  8:00   ` Jungseok Lee
2015-11-04  7:49     ` AKASHI Takahiro
2015-10-30  5:25 ` [PATCH v4 4/6] ftrace: allow arch-specific stack tracer AKASHI Takahiro
2015-10-30  5:25 ` [PATCH v4 5/6] arm64: insn: add instruction decoders for ldp/stp and add/sub AKASHI Takahiro
2015-10-30  5:25 ` [PATCH v4 6/6] arm64: ftrace: add arch-specific stack tracer AKASHI Takahiro
2015-11-01  8:30   ` Jungseok Lee
2015-11-04  8:01     ` AKASHI Takahiro
2015-11-04  8:42   ` yalin wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).