Skip to contentSkip to conversation

powerpc: Reimplement __get_SP() as a function not a define

PATCH In review Anton Blanchard 2 participants 3 messages Oct 1, 2014 – Oct 8, 2014

Subsystems (2)
PowerPC perf events
lore
Share & tools
lore

Anton Blanchard

author
#0 lore raw

PATCH 1/1 powerpc: Reimplement __get_SP() as a function not a define

Li Zhong points out an issue with our current __get_SP() implementation. If ftrace function tracing is enabled (ie -pg profiling using _mcount) we spill a stack frame on 64bit all the time.

If a function calls __get_SP() and later calls a function that is tail call optimised, we will pop the stack frame and the value returned by __get_SP() is no longer valid. An example from Li can be found in save_stack_trace -> save_context_stack:

c0000000000432c0 <.save_stack_trace>:

c0000000000432c0:       mflr    r0
c0000000000432c4:       std     r0,16(r1)
c0000000000432c8:       stdu    r1,-128(r1) <-- stack frame for _mcount
c0000000000432cc:       std     r3,112(r1)
c0000000000432d0:       bl      <._mcount>
c0000000000432d4:       nop

c0000000000432d8: mr r4,r1 <-- __get_SP()

c0000000000432dc:       ld      r5,632(r13)
c0000000000432e0:       ld      r3,112(r1)
c0000000000432e4:       li      r6,1

c0000000000432e8: addi r1,r1,128 <-- pop stack frame

c0000000000432ec:       ld      r0,16(r1)
c0000000000432f0:       mtlr    r0
c0000000000432f4:       b       <.save_context_stack> <-- tail call optimized

save_context_stack ends up with a stack pointer below the current one, and it is likely to be scribbled over.

Fix this by making __get_SP() a function which returns the callers stack frame. Also replace inline assembly which grabs the stack pointer in save_stack_trace and show_stack with __get_SP().

Reported-by Li Zhong <[email protected]>
Signed-off-by Anton Blanchard <[email protected]>
---
 arch/powerpc/include/asm/reg.h   | 3 +--
 arch/powerpc/kernel/misc.S       | 4 ++++
 arch/powerpc/kernel/process.c    | 2 +-
 arch/powerpc/kernel/stacktrace.c | 2 +-
 4 files changed, 7 insertions(+), 4 deletions(-)
arch/powerpc/include/asm/reg.h +1 -2
index 0c05059..0f973c0 100644
@@ -1264,8 +1264,7 @@ static inline unsigned long mfvtb (void)
 
 #define proc_trap()	asm volatile("trap")
 
-#define __get_SP()	({unsigned long sp; \
-			asm volatile("mr %0,1": "=r" (sp)); sp;})
+extern unsigned long __get_SP(void);
 
 extern unsigned long scom970_read(unsigned int address);
 extern void scom970_write(unsigned int address, unsigned long value);
arch/powerpc/kernel/misc.S +4 -0
index 7ce26d4..120deb7 100644
@@ -114,3 +114,7 @@ _GLOBAL(longjmp)
 	mtlr	r0
 	mr	r3,r4
 	blr
+
+_GLOBAL(__get_SP)
+	PPC_LL	r3,0(r1)
+	blr
arch/powerpc/kernel/process.c +1 -1
index aa1df89..3cc6439 100644
@@ -1545,7 +1545,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
 		tsk = current;
 	if (sp == 0) {
 		if (tsk == current)
-			asm("mr %0,1" : "=r" (sp));
+			sp = __get_SP();
 		else
 			sp = tsk->thread.ksp;
 	}
arch/powerpc/kernel/stacktrace.c +1 -1
index 3d30ef1..7f65bae 100644
@@ -50,7 +50,7 @@ void save_stack_trace(struct stack_trace *trace)
 {
 	unsigned long sp;
 
-	asm("mr %0,1" : "=r" (sp));
+	sp = __get_SP();
 
 	save_context_stack(trace, sp, current, 1);
 }
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 1/1 powerpc: Rename __get_SP() to current_stack_pointer()

Michael points out that __get_SP() is a pretty horrible function name. Let's give it a better name.

Signed-off-by Anton Blanchard <[email protected]>
---
 arch/powerpc/include/asm/perf_event.h | 2 +-
 arch/powerpc/include/asm/reg.h        | 2 +-
 arch/powerpc/kernel/irq.c             | 2 +-
 arch/powerpc/kernel/misc.S            | 2 +-
 arch/powerpc/kernel/process.c         | 2 +-
 arch/powerpc/kernel/stacktrace.c      | 2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)
arch/powerpc/include/asm/perf_event.h +1 -1
index 0bb2372..fc53598 100644
@@ -34,7 +34,7 @@
 	do {							\
 		(regs)->result = 0;				\
 		(regs)->nip = __ip;				\
-		(regs)->gpr[1] = *(unsigned long *)__get_SP();	\
+		(regs)->gpr[1] = *(unsigned long *)current_stack_pointer(); \
 		asm volatile("mfmsr %0" : "=r" ((regs)->msr));	\
 	} while (0)
 #endif
arch/powerpc/include/asm/reg.h +1 -1
index 0f973c0..d8974b5 100644
@@ -1264,7 +1264,7 @@ static inline unsigned long mfvtb (void)
 
 #define proc_trap()	asm volatile("trap")
 
-extern unsigned long __get_SP(void);
+extern unsigned long current_stack_pointer(void);
 
 extern unsigned long scom970_read(unsigned int address);
 extern void scom970_write(unsigned int address, unsigned long value);
arch/powerpc/kernel/irq.c +1 -1
index 8eb857f..c143835 100644
@@ -466,7 +466,7 @@ static inline void check_stack_overflow(void)
 #ifdef CONFIG_DEBUG_STACKOVERFLOW
 	long sp;
 
-	sp = __get_SP() & (THREAD_SIZE-1);
+	sp = current_stack_pointer() & (THREAD_SIZE-1);
 
 	/* check for stack overflow: is there less than 2KB free? */
 	if (unlikely(sp < (sizeof(struct thread_info) + 2048))) {
arch/powerpc/kernel/misc.S +1 -1
index 120deb7..0d43219 100644
@@ -115,6 +115,6 @@ _GLOBAL(longjmp)
 	mr	r3,r4
 	blr
 
-_GLOBAL(__get_SP)
+_GLOBAL(current_stack_pointer)
 	PPC_LL	r3,0(r1)
 	blr
arch/powerpc/kernel/process.c +1 -1
index 3cc6439..923cd2d 100644
@@ -1545,7 +1545,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
 		tsk = current;
 	if (sp == 0) {
 		if (tsk == current)
-			sp = __get_SP();
+			sp = current_stack_pointer();
 		else
 			sp = tsk->thread.ksp;
 	}
arch/powerpc/kernel/stacktrace.c +1 -1
index 7f65bae..ea43a34 100644
@@ -50,7 +50,7 @@ void save_stack_trace(struct stack_trace *trace)
 {
 	unsigned long sp;
 
-	sp = __get_SP();
+	sp = current_stack_pointer();
 
 	save_context_stack(trace, sp, current, 1);
 }
Signature · 1 line
1.9.1
Tested-by on v1 1/1
#2 lore raw
In reply to Anton Blanchard
Quoted 52 lines · Anton Blanchard

Li Zhong points out an issue with our current __get_SP() implementation. If ftrace function tracing is enabled (ie -pg profiling using _mcount) we spill a stack frame on 64bit all the time.

If a function calls __get_SP() and later calls a function that is tail call optimised, we will pop the stack frame and the value returned by __get_SP() is no longer valid. An example from Li can be found in save_stack_trace -> save_context_stack:

c0000000000432c0 <.save_stack_trace>:

c0000000000432c0:       mflr    r0
c0000000000432c4:       std     r0,16(r1)
c0000000000432c8:       stdu    r1,-128(r1) <-- stack frame for _mcount
c0000000000432cc:       std     r3,112(r1)
c0000000000432d0:       bl      <._mcount>
c0000000000432d4:       nop

c0000000000432d8: mr r4,r1 <-- __get_SP()

c0000000000432dc:       ld      r5,632(r13)
c0000000000432e0:       ld      r3,112(r1)
c0000000000432e4:       li      r6,1

c0000000000432e8: addi r1,r1,128 <-- pop stack frame

c0000000000432ec:       ld      r0,16(r1)
c0000000000432f0:       mtlr    r0
c0000000000432f4:       b       <.save_context_stack> <-- tail call optimized

save_context_stack ends up with a stack pointer below the current one, and it is likely to be scribbled over.

Fix this by making __get_SP() a function which returns the callers stack frame. Also replace inline assembly which grabs the stack pointer in save_stack_trace and show_stack with __get_SP().

Reported-by Li Zhong <[email protected]>
Signed-off-by Anton Blanchard <[email protected]>
---
 arch/powerpc/include/asm/reg.h   | 3 +--
 arch/powerpc/kernel/misc.S       | 4 ++++
 arch/powerpc/kernel/process.c    | 2 +-
 arch/powerpc/kernel/stacktrace.c | 2 +-
 4 files changed, 7 insertions(+), 4 deletions(-)
arch/powerpc/include/asm/reg.h +0 -0
index 0c05059..0f973c0 100644

arch/powerpc/include/asm/reg.h static inline unsigned long mfvtb (void) · line 1267

Quoted 6 lines
arch/powerpc/include/asm/reg.h +1 -2
@@ -1264,8 +1264,7 @@ static inline unsigned long mfvtb (void)
 
 #define proc_trap()	asm volatile("trap")
 
-#define __get_SP()	({unsigned long sp; \
-			asm volatile("mr %0,1": "=r" (sp)); sp;})
+extern unsigned long __get_SP(void);

It seems that some module code is using __get_SP, e.g. xfs in the example below:
ERROR: ".__get_SP" [fs/xfs/xfs.ko] undefined!

Maybe we need export this symbol in arch/powerpc/kernel/ppc_ksyms.c?

arch/powerpc/kernel/ppc_ksyms.c +2 -0
index 48d17d6f..eebd4e4 100644
@@ -207,3 +207,5 @@ EXPORT_SYMBOL_GPL(mmu_psize_defs);
 #ifdef CONFIG_EPAPR_PARAVIRT
 EXPORT_SYMBOL(epapr_hypercall_start);
 #endif
+
+EXPORT_SYMBOL(__get_SP);

With the above compiling error fixed, this patch solved the SP issue I saw, so

Tested-by Li Zhong <[email protected]>
Quoted 41 lines
arch/powerpc/include/asm/reg.h +0 -0
 extern unsigned long scom970_read(unsigned int address);
 extern void scom970_write(unsigned int address, unsigned long value);
arch/powerpc/kernel/misc.S +4 -0
index 7ce26d4..120deb7 100644
@@ -114,3 +114,7 @@ _GLOBAL(longjmp)
 	mtlr	r0
 	mr	r3,r4
 	blr
+
+_GLOBAL(__get_SP)
+	PPC_LL	r3,0(r1)
+	blr
arch/powerpc/kernel/process.c +1 -1
index aa1df89..3cc6439 100644
@@ -1545,7 +1545,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
 		tsk = current;
 	if (sp == 0) {
 		if (tsk == current)
-			asm("mr %0,1" : "=r" (sp));
+			sp = __get_SP();
 		else
 			sp = tsk->thread.ksp;
 	}
arch/powerpc/kernel/stacktrace.c +1 -1
index 3d30ef1..7f65bae 100644
@@ -50,7 +50,7 @@ void save_stack_trace(struct stack_trace *trace)
 {
 	unsigned long sp;
 
-	asm("mr %0,1" : "=r" (sp));
+	sp = __get_SP();
 
 	save_context_stack(trace, sp, current, 1);
 }