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

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

[Masami Hiramatsu]

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)

[ impact: Oprofile decodes only stack for backtracing on i386 ]

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Harvey Harrison <harvey.harrison@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jan Blunck <jblunck@suse.de>
Cc: Christoph Hellwig <hch@infradead.org>
---

 arch/x86/include/asm/ptrace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index 5cdd19f..90b76b3 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -187,14 +187,14 @@ 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.  So &regs->sp will be the current sp.
  *
  * This is valid only for kernel mode traps.
  */
 static inline unsigned long kernel_trap_sp(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_32
-	return (unsigned long)regs;
+	return (unsigned long)&regs->sp;
 #else
 	return regs->sp;
 #endif

[tip:x86/urgent] x86, 32-bit: fix kernel_trap_sp()

[tip-bot for Masami Hiramatsu]

Commit-ID:  11d0f25f06fb67e707e6ed4bf9967b859e403639
Gitweb:     http://git.kernel.org/tip/11d0f25f06fb67e707e6ed4bf9967b859e403639
Author:     Masami Hiramatsu <mhiramat@redhat.com>
AuthorDate: Mon, 11 May 2009 17:03:00 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 11 May 2009 23:49:17 +0200

x86, 32-bit: fix kernel_trap_sp()

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)

[ Impact: Oprofile decodes only stack for backtracing on i386 ]

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
Cc: systemtap@sources.redhat.com
Cc: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Harvey Harrison <harvey.harrison@gmail.com>
Cc: Jan Blunck <jblunck@suse.de>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
LKML-Reference: <20090511210300.17332.67549.stgit@localhost.localdomain>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/include/asm/ptrace.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index e304b66..aed0894 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -187,14 +187,14 @@ 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.  So &regs->sp will be the current sp.
  *
  * This is valid only for kernel mode traps.
  */
 static inline unsigned long kernel_trap_sp(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_32
-	return (unsigned long)regs;
+	return (unsigned long)&regs->sp;
 #else
 	return regs->sp;
 #endif

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

[tip:x86/urgent] x86, 32-bit: fix kernel_trap_sp()

[tip-bot for Masami Hiramatsu]

Commit-ID:  8310d65ae37ce9180cd939334218df59f1ad3de6
Gitweb:     http://git.kernel.org/tip/8310d65ae37ce9180cd939334218df59f1ad3de6
Author:     Masami Hiramatsu <mhiramat@redhat.com>
AuthorDate: Mon, 11 May 2009 17:03:00 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 12 May 2009 00:37:40 +0200

x86, 32-bit: fix kernel_trap_sp()

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)

[ Impact: Oprofile decodes only stack for backtracing on i386 ]

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
[ v2: rename the API to kernel_stack_pointer(), move variable inside ]
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: systemtap@sources.redhat.com
Cc: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Harvey Harrison <harvey.harrison@gmail.com>
Cc: Jan Blunck <jblunck@suse.de>
Cc: Christoph Hellwig <hch@infradead.org>
LKML-Reference: <20090511210300.17332.67549.stgit@localhost.localdomain>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


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

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

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 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?

yeah, this is cleaner and probably a tad faster. I've applied it to 
x86/urgent with your acked-by - or would you like to apply this 
straight away?

The use of the backtracing feature on the oprofile side is not very 
common (and the bug is ancient) so i wanted to wait a few days with 
this.

One small detail:

> +	return (unsigned long)(&regs->sp);

the original commit had:

> +	return (unsigned long)&regs->sp;

and this latter is correct as well due to operator priorities. No 
strong preference from my side so i took your version as-is. I kept 
Masami as the author and a comment for your changes.

( Not sure how to express this in a changelog properly - i didnt 
  want two commits but it was authored by two people in essence. So 
  i went for the version that will show up in the commit 
  notification email in a few minutes - also attached below. )

	Ingo

[tip:x86/urgent] x86, 32-bit: fix kernel_trap_sp()

[tip-bot for Masami Hiramatsu]

Commit-ID:  7b6c6c77732ca1d2498eda7eabb64f9648896e96
Gitweb:     http://git.kernel.org/tip/7b6c6c77732ca1d2498eda7eabb64f9648896e96
Author:     Masami Hiramatsu <mhiramat@redhat.com>
AuthorDate: Mon, 11 May 2009 17:03:00 -0400
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 12 May 2009 00:39:52 +0200

x86, 32-bit: fix kernel_trap_sp()

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)

[ Impact: Oprofile decodes only stack for backtracing on i386 ]

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
[ v2: rename the API to kernel_stack_pointer(), move variable inside ]
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: systemtap@sources.redhat.com
Cc: Harvey Harrison <harvey.harrison@gmail.com>
Cc: Jan Blunck <jblunck@suse.de>
Cc: Christoph Hellwig <hch@infradead.org>
LKML-Reference: <20090511210300.17332.67549.stgit@localhost.localdomain>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


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

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

[Linus Torvalds]

On Tue, 12 May 2009, Ingo Molnar wrote:
> 
> yeah, this is cleaner and probably a tad faster.

Well, I don't care about the 'faster' part per se, but I could actually 
see some case where a kernel-only version did some pointer dereference 
that was invalid for the user version, and could oops, so putting it 
inside the code that explicitly tests that it's not user-or-vm seems like 
conceptually the right thing to do.

Of course, in this case it's the other way around - it's the non-kernel 
version that does a dereference, and it just so happens to be safe (but 
return an invalid pointer) when the trap was in kernel mode.  So the 
argument is pretty theoretical, but I think it's cleaner.

> One small detail:
> 
> > +	return (unsigned long)(&regs->sp);
> 
> the original commit had:
> 
> > +	return (unsigned long)&regs->sp;

Ok, that's just because I didn't actually apply the original patch, I 
just rewrote it entirely, and for some reason I put parenthesis around the 
expression. You're right that it doesn't matter, and either is fine.  I 
don't really care, I suspect I add the parenthesis just because I don't 
even want to have to think about the proper operator precedence rules.

So pick whichever version.

			Linus

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

[Robin Holt]

On Mon, May 11, 2009 at 03:24:07PM -0700, Linus Torvalds wrote:
> 
> 
> 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)

Why not have it return an unsigned long *?
>  {
>  #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);

Make this an unsigned long *?

>  		if (depth)
>  			dump_trace(NULL, regs, (unsigned long *)stack, 0,

Then get rid of the cast?

Robin

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

[Masami Hiramatsu]

Robin Holt wrote:
> On Mon, May 11, 2009 at 03:24:07PM -0700, Linus Torvalds wrote:
>>
>> 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)
> 
> Why not have it return an unsigned long *?
>>  {
>>  #ifdef CONFIG_X86_32
>> -	return (unsigned long)regs;
>> +	return (unsigned long)(&regs->sp);
>>  #else
>>  	return regs->sp;

Perhaps, you might forget that this line needs a cast :-).

Anyway, IMHO, it does not come from coding, but meaning.
kernel/user_stack_pointer() just return
"the value of stack pointer register", not a "stack pointer".

Thank you,

>>  #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);
> 
> Make this an unsigned long *?
> 
>>  		if (depth)
>>  			dump_trace(NULL, regs, (unsigned long *)stack, 0,
> 
> Then get rid of the cast?
> 
> Robin

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

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

[Linus Torvalds]

On Tue, 12 May 2009, Robin Holt wrote:
> >   */
> > -static inline unsigned long kernel_trap_sp(struct pt_regs *regs)
> > +static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
> 
> Why not have it return an unsigned long *?

I considered it, but since part of the point of the patch was to make it 
look like the mirror of "user_stack_pointer()", returning "unsigned long" 
looked more appropriate.

But I have no really strong opinions. I wouldn't object to making it 
return "unsigned long *" either.

		Linus