[patch 1/4] powerpc: fix for OProfile callgraph for Power 64 bit user apps

[patch 1/4] powerpc: fix for OProfile callgraph for Power 64 bit user apps

[akpm]

From: Carl Love <cel@us.ibm.com>

Fix the 64 bit user code backtrace which currently may hang the system.

Signed-off-by: Carl Love <carll@us.ibm.com>
Cc: Maynard Johnson <maynardj@us.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/powerpc/oprofile/backtrace.c |   33 ++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff -puN arch/powerpc/oprofile/backtrace.c~powerpc-fix-for-oprofile-callgraph-for-power-64-bit-user-apps arch/powerpc/oprofile/backtrace.c
--- a/arch/powerpc/oprofile/backtrace.c~powerpc-fix-for-oprofile-callgraph-for-power-64-bit-user-apps
+++ a/arch/powerpc/oprofile/backtrace.c
@@ -53,19 +53,40 @@ static unsigned int user_getsp32(unsigne
 #ifdef CONFIG_PPC64
 static unsigned long user_getsp64(unsigned long sp, int is_first)
 {
-	unsigned long stack_frame[3];
+	unsigned long stk_frm_lr;
+	unsigned long stk_frm_sp;
+	unsigned long size;
+
+	/* Issue the __copy_from_user_inatomic() third argument currently
+	 * only takes sizes 1, 2, 4 or 8 bytes.  Don't read more then the
+	 * first 48 bytes of the stack frame.  That is all that is
+	 * guaranteed to exist.  Reading more may cause the system to hang.
+	 *
+	 * 64 bit stack frame layout:
+	 * 0-7   bytes is the pointer to previous stack
+	 * 8-15  bytes condition register save area
+	 * 16-23 bytes link register save area
+	 */
+	size = sizeof(unsigned long);
+	if (!access_ok(VERIFY_READ, (void __user *)sp, size))
+		return 0;
 
-	if (!access_ok(VERIFY_READ, (void __user *)sp, sizeof(stack_frame)))
+	if (__copy_from_user_inatomic(&stk_frm_sp, (void __user *)sp,
+					size))
 		return 0;
 
-	if (__copy_from_user_inatomic(stack_frame, (void __user *)sp,
-					sizeof(stack_frame)))
+	/* get the LR from the user stack */
+	if (!access_ok(VERIFY_READ, (void __user *)(sp+16), size))
+		return 0;
+
+	if (__copy_from_user_inatomic(&stk_frm_lr, (void __user *)(sp+16),
+					size))
 		return 0;
 
 	if (!is_first)
-		oprofile_add_trace(STACK_LR64(stack_frame));
+		oprofile_add_trace(stk_frm_lr);
 
-	return STACK_SP(stack_frame);
+	return stk_frm_sp;
 }
 #endif
 
_

Re: [patch 1/4] powerpc: fix for OProfile callgraph for Power 64 bit user apps

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2002 bytes --]

On Wed, 2008-05-14 at 16:12 -0700, akpm@linux-foundation.org wrote:
> From: Carl Love <cel@us.ibm.com>
> 
> Fix the 64 bit user code backtrace which currently may hang the system.
> 
> Signed-off-by: Carl Love <carll@us.ibm.com>
> Cc: Maynard Johnson <maynardj@us.ibm.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Hi Carl,

I'm a bit confused by this change ..

> diff -puN arch/powerpc/oprofile/backtrace.c~powerpc-fix-for-oprofile-callgraph-for-power-64-bit-user-apps arch/powerpc/oprofile/backtrace.c
> --- a/arch/powerpc/oprofile/backtrace.c~powerpc-fix-for-oprofile-callgraph-for-power-64-bit-user-apps
> +++ a/arch/powerpc/oprofile/backtrace.c
> @@ -53,19 +53,40 @@ static unsigned int user_getsp32(unsigne
>  #ifdef CONFIG_PPC64
>  static unsigned long user_getsp64(unsigned long sp, int is_first)
>  {
> -	unsigned long stack_frame[3];
> +	unsigned long stk_frm_lr;
> +	unsigned long stk_frm_sp;
> +	unsigned long size;
> +
> +	/* Issue the __copy_from_user_inatomic() third argument currently
> +	 * only takes sizes 1, 2, 4 or 8 bytes.  Don't read more then the
> +	 * first 48 bytes of the stack frame.  That is all that is
> +	 * guaranteed to exist.  Reading more may cause the system to hang.

__copy_from_user_inatomic() accepts any value for n, it just has a
special case for 1, 2, 4 and 8 - but it should still work for other
values.

The old code copied 24 bytes from sp, and the new code copies 8 bytes
from sp and 8 bytes from sp + 16 - so I don't see where the 48 bytes
comes in to it?

Also the comment is a little hard to parse, I think you mean "Issue:
the ..", but I read "Issue" as a verb in that sentence. And "Don't read
more then" should be "than".

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 1/4] powerpc: fix for OProfile callgraph for Power 64 bit user apps

[Paul Mackerras]

Michael Ellerman writes:

> __copy_from_user_inatomic() accepts any value for n, it just has a
> special case for 1, 2, 4 and 8 - but it should still work for other
> values.

There is a bug in __copy_from_user_inatomic that

http://patchwork.ozlabs.org/linuxppc/patch?id=18418

fixes, and I'm about to send that Linus-wards.

Carl, to what extent does that fix eliminate the need for the changes
in your patch?

Paul.

Re: [patch 1/4] powerpc: fix for OProfile callgraph for Power 64 bit user apps

[Carl Love]

On Thu, 2008-05-15 at 13:58 +1000, Paul Mackerras wrote:
> Michael Ellerman writes:
> 
> > __copy_from_user_inatomic() accepts any value for n, it just has a
> > special case for 1, 2, 4 and 8 - but it should still work for other
> > values.
> 
> There is a bug in __copy_from_user_inatomic that
> 
> http://patchwork.ozlabs.org/linuxppc/patch?id=18418
> 
> fixes, and I'm about to send that Linus-wards.
> 
> Carl, to what extent does that fix eliminate the need for the changes
> in your patch?
> 
> Paul.

Paul:

I will have to apply the above mentioned patch to see if it fixes the
issue.  What I found was when the original code tried to copy three
unsigned long into stack_frame[3], i.e. 24 bytes my system consistently
failed.  The comment about 48 bytes is a note that is all that is
guaranteed to be there according to the API.  I found by restricting the
copy to the values in the case statement, things worked.  

              Carl Love