[PATCH] Fix for OProfile callgraph for Power 64 bit user apps

[PATCH] Fix for OProfile callgraph for Power 64 bit user apps

[Carl Love]

The following patch fixes the 64 bit user code backtrace 
which currently may hang the system.  

Signed-off-by: Carl Love <carll@us.ibm.com>

Index: linux-2.6.25.1/arch/powerpc/oprofile/backtrace.c
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/oprofile/backtrace.c	2008-05-01 16:45:25.000000000 -0500
+++ linux-2.6.25.1/arch/powerpc/oprofile/backtrace.c	2008-05-07 11:29:02.000000000 -0500
@@ -53,19 +53,40 @@
 #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] Fix for OProfile callgraph for Power 64 bit user apps

[Paul Mackerras]

Carl Love writes:

> The following patch fixes the 64 bit user code backtrace 
> which currently may hang the system.  

What exactly is wrong with it?

Having now taken a much closer look, I now don't think Nate Case's
patch addresses this, since it only affects constant size arguments
<= 8 to copy_{to,from}_user_inatomic.

However, I don't see why your patch fixes anything.  It means we do
two access_ok calls and two __copy_from_user_inatomic calls, for 8
bytes, at sp and at sp + 16, rather than doing one access_ok and
__copy_from_user_inatomic for 24 bytes at sp.  Why does that make any
difference (apart from being slower)?

Paul.

Re: [PATCH] Fix for OProfile callgraph for Power 64 bit user apps

[Carl Love]

On Thu, 2008-05-15 at 20:47 +1000, Paul Mackerras wrote:
> Carl Love writes:
> 
> > The following patch fixes the 64 bit user code backtrace 
> > which currently may hang the system.  
> 
> What exactly is wrong with it?
> 
> Having now taken a much closer look, I now don't think Nate Case's
> patch addresses this, since it only affects constant size arguments
> <= 8 to copy_{to,from}_user_inatomic.
> 
> However, I don't see why your patch fixes anything.  It means we do
> two access_ok calls and two __copy_from_user_inatomic calls, for 8
> bytes, at sp and at sp + 16, rather than doing one access_ok and
> __copy_from_user_inatomic for 24 bytes at sp.  Why does that make any
> difference (apart from being slower)?
> 
> Paul

When I tried testing the oprofile call graph on a 64 bit app the system
consistently hung.  I was able to isolate it to the
__copy_from_user_inatomic() call.  When I made the change in my patch to
make sure I was only requesting one of the values (8bytes) listed in the
case statement this fixed the issue.  I do not know nor was I able to
figure out why the __copy_from_user_inatomic() call failed trying to
read 24 bytes.  The system would hang and any attempt to use printk to
see what was going on failed as the output of the print would not go to
the console before the system hangs.  

I backed out my patch, put in Nate's patch.  The call graph test ran
fine.  I then backed out Nate's patch to go back and try to re-validate
that the system still hangs with the original code and it is not
hanging.  Not sure why it now seems to work.  I have done some other
work on the system but I don't see how that would have changed this.
Argh, I hate chasing phantom bugs!  I was working on 2.6.21. I believe
the 2.6.21 kernel had not been changed. Let me load the latest 2.6.25
and start over with a pristine kernel and see if I can reproduce the
hang.  Sorry for all the hassle.

                 Carl Love

Re: [PATCH] Fix for OProfile callgraph for Power 64 bit user apps

[Carl Love]

On Thu, 2008-05-15 at 11:01 -0700, Carl Love wrote:
> On Thu, 2008-05-15 at 20:47 +1000, Paul Mackerras wrote:
> > Carl Love writes:
> > 
> > > The following patch fixes the 64 bit user code backtrace 
> > > which currently may hang the system.  
> > 
> > What exactly is wrong with it?
> > 
> > Having now taken a much closer look, I now don't think Nate Case's
> > patch addresses this, since it only affects constant size arguments
> > <= 8 to copy_{to,from}_user_inatomic.
> > 
> > However, I don't see why your patch fixes anything.  It means we do
> > two access_ok calls and two __copy_from_user_inatomic calls, for 8
> > bytes, at sp and at sp + 16, rather than doing one access_ok and
> > __copy_from_user_inatomic for 24 bytes at sp.  Why does that make any
> > difference (apart from being slower)?
> > 
> > Paul
> 
> When I tried testing the oprofile call graph on a 64 bit app the system
> consistently hung.  I was able to isolate it to the
> __copy_from_user_inatomic() call.  When I made the change in my patch to
> make sure I was only requesting one of the values (8bytes) listed in the
> case statement this fixed the issue.  I do not know nor was I able to
> figure out why the __copy_from_user_inatomic() call failed trying to
> read 24 bytes.  The system would hang and any attempt to use printk to
> see what was going on failed as the output of the print would not go to
> the console before the system hangs.  
> 
> I backed out my patch, put in Nate's patch.  The call graph test ran
> fine.  I then backed out Nate's patch to go back and try to re-validate
> that the system still hangs with the original code and it is not
> hanging.  Not sure why it now seems to work.  I have done some other
> work on the system but I don't see how that would have changed this.
> Argh, I hate chasing phantom bugs!  I was working on 2.6.21. I believe
> the 2.6.21 kernel had not been changed. Let me load the latest 2.6.25
> and start over with a pristine kernel and see if I can reproduce the
> hang.  Sorry for all the hassle.
> 
>                  Carl Love
> 

I installed the latest 2.6.25 kernel and tested OProfile call graph on
the 64 bit user application.  I did not see any hangs for the tests that
I ran.  I tried things multiple times.  So, I guess we should drop the
OProfile callgraph patch.  Clearly if there still is a problem it is not
in how the OProfile call graph code is written but is probably in the
underlying calls, i.e. __copy_from_user_inatomic().  I will continue to
test the functionality and see if I can find a example where the system
will hang so we can investigate the underlying cause.

Thank you for your time on this. 

               Carl Love