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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Mon, 2008-06-09 at 16:26 -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>

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 <michael@ellerman.id.au> 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 <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
>  
> _
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev