Re: [PATCH -rc] [BUGFIX] x86: fix kernel_trap_sp()

[Linus Torvalds]

On Mon, 11 May 2009, Masami Hiramatsu wrote:
>
> Use &regs->sp instead of regs for getting the top of stack in kernel mode.
> (on x86-64, regs->sp always points the top of stack)

Ack. 

That said, we have only _one_ use of this "kernel_trap_sp()" in the whole 
kernel, and that use is actually fairly odd too, in that it does it 
_before_ checking that it's in kernel mode.

Admittedly it will then only _use_ the value after it has checked that 
things are in kernel mode, but it all boils down to "ok, that's pretty 
odd".

So how about fixing that, and also fixing the naming of the function. Call 
it "kernel_stack_pointer()" to match its more widely used sibling function
"user_stack_pointer()".

IOW, something like this?

		Linus

---
 arch/x86/include/asm/ptrace.h |    7 ++++---
 arch/x86/oprofile/backtrace.c |    2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index e304b66..624f133 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -187,14 +187,15 @@ static inline int v8086_mode(struct pt_regs *regs)
 
 /*
  * X86_32 CPUs don't save ss and esp if the CPU is already in kernel mode
- * when it traps.  So regs will be the current sp.
+ * when it traps.  The previous stack will be directly underneath the saved
+ * registers, and 'sp/ss' won't even have been saved. Thus the '&regs->sp'.
  *
  * This is valid only for kernel mode traps.
  */
-static inline unsigned long kernel_trap_sp(struct pt_regs *regs)
+static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_32
-	return (unsigned long)regs;
+	return (unsigned long)(&regs->sp);
 #else
 	return regs->sp;
 #endif
diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c
index 04df67f..044897b 100644
--- a/arch/x86/oprofile/backtrace.c
+++ b/arch/x86/oprofile/backtrace.c
@@ -76,9 +76,9 @@ void
 x86_backtrace(struct pt_regs * const regs, unsigned int depth)
 {
 	struct frame_head *head = (struct frame_head *)frame_pointer(regs);
-	unsigned long stack = kernel_trap_sp(regs);
 
 	if (!user_mode_vm(regs)) {
+		unsigned long stack = kernel_stack_pointer(regs);
 		if (depth)
 			dump_trace(NULL, regs, (unsigned long *)stack, 0,
 				   &backtrace_ops, &depth);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/