Re: [patch 1/4] powerpc: fix for OProfile callgraph for Power 64 bit user apps
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[patch 1/4] 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] 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/4] powerpc: fix for OProfile callgraph for Power 64 bit user apps
On Wed, 2008-05-14 at 16:12 -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] Signed-off-by: Andrew Morton [EMAIL PROTECTED] 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 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/4] powerpc: fix for OProfile callgraph for Power 64 bit user apps
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev