diff mbox

[v4,1/6] arm64: ftrace: adjust callsite addresses examined by stack tracer

Message ID 1446182741-31019-2-git-send-email-takahiro.akashi@linaro.org
State New
Headers show

Commit Message

AKASHI Takahiro Oct. 30, 2015, 5:25 a.m. UTC 
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(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Will Deacon Oct. 30, 2015, 11:16 a.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mark Rutland Nov. 2, 2015, 6:29 p.m. UTC | #2 
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Will Deacon Nov. 2, 2015, 6:41 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mark Rutland Nov. 2, 2015, 6:43 p.m. UTC | #4 
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

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);