Re: [patch 1/5] powerpc: fix for OProfile callgraph for Power 64 bit user apps
On Tue, 2008-06-10 at 10:30 +1000, Michael Ellerman wrote: On Tue, 2008-06-10 at 09:41 +1000, Benjamin Herrenschmidt wrote: On Mon, 2008-06-09 at 16:26 -0700, [EMAIL PROTECTED] wrote: From: Carl Love [EMAIL PROTECTED] Fix the 64 bit user code backtrace which currently may hang the system. Signed-off-by: Carl Love [EMAIL PROTECTED] Cc: Maynard Johnson [EMAIL PROTECTED] That looks weird. I doubt it's the right fix for the problem. Paul, I remember this was discussed earlier, did we come up with a proper fix already ? We decided there probably wasn't a bug there at all: http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056449.html Haven't heard from Carl if he reproduced the hang and traced it to something else. cheers After a discussion of this patch, the general consensus was that the issue is with the underlying copy from user call. After developing and posting the patch, I had tried to go back and reproduce it again and was not able to. My system had changed slightly and I could get it to fail. I still have this on my to do list to get back to working on the Powerpc callgraph support as there are some other fundamental issues with the code. Specifically the call back traces are not always correct for leaf nodes. This was actually the issue I was working on when I found the issue with the 64 bit applications hanging the system. The patch should be dropped. Carl Love ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[patch 1/5] powerpc: fix for OProfile callgraph for Power 64 bit user apps
From: Carl Love [EMAIL PROTECTED]
Fix the 64 bit user code backtrace which currently may hang the system.
Signed-off-by: Carl Love [EMAIL PROTECTED]
Cc: Maynard Johnson [EMAIL PROTECTED]
On Thu, 15 May 2008 10:20:44 +1000
Michael Ellerman [EMAIL PROTECTED] wrote:
__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?
\ufeffAlso 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.
Signed-off-by: Andrew Morton [EMAIL PROTECTED]
---
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
_
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [patch 1/5] powerpc: fix for OProfile callgraph for Power 64 bit user apps
On Mon, 2008-06-09 at 16:26 -0700, [EMAIL PROTECTED] wrote:
From: Carl Love [EMAIL PROTECTED]
Fix the 64 bit user code backtrace which currently may hang the system.
Signed-off-by: Carl Love [EMAIL PROTECTED]
Cc: Maynard Johnson [EMAIL PROTECTED]
That looks weird. I doubt it's the right fix for the problem. Paul,
I remember this was discussed earlier, did we come up with a proper fix
already ?
Cheers,
Ben.
On Thu, 15 May 2008 10:20:44 +1000
Michael Ellerman [EMAIL PROTECTED] wrote:
__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?
\ufeffAlso 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.
Signed-off-by: Andrew Morton [EMAIL PROTECTED]
---
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
_
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [patch 1/5] powerpc: fix for OProfile callgraph for Power 64 bit user apps
On Tue, 2008-06-10 at 09:41 +1000, Benjamin Herrenschmidt wrote: On Mon, 2008-06-09 at 16:26 -0700, [EMAIL PROTECTED] wrote: From: Carl Love [EMAIL PROTECTED] Fix the 64 bit user code backtrace which currently may hang the system. Signed-off-by: Carl Love [EMAIL PROTECTED] Cc: Maynard Johnson [EMAIL PROTECTED] That looks weird. I doubt it's the right fix for the problem. Paul, I remember this was discussed earlier, did we come up with a proper fix already ? We decided there probably wasn't a bug there at all: http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056449.html Haven't heard from Carl if he reproduced the hang and traced it to something else. 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 signature.asc Description: This is a digitally signed message part ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [patch 1/5] powerpc: fix for OProfile callgraph for Power 64 bit user apps
[EMAIL PROTECTED] writes: From: Carl Love [EMAIL PROTECTED] Fix the 64 bit user code backtrace which currently may hang the system. NAK - Carl withdrew this patch ages ago. Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
