Fix for OProfile callgraph for Power 64 bit user apps

PATCH In review Carl Love 2 participants 4 messages May 7, 2008 – May 16, 2008 PowerPC

lore

Share & tools

lore

Carl Love

author

2008-05-07 18:14 UTC #0 lore raw

PATCH 1/1 Fix for OProfile callgraph for Power 64 bit user apps

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

Signed-off-by Carl Love <[email protected]>

arch/powerpc/oprofile/backtrace.c +27 -6

Index: linux-2.6.25.1/arch/powerpc/oprofile/backtrace.c
===================================================================
@@ -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

Paul Mackerras

2008-05-15 10:47 UTC #1 lore raw

Quoted 2 lines · Carl Love

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.

Carl Love

author

2008-05-15 18:03 UTC #2 lore raw

Quoted 11 lines · Paul Mackerras

Quoted 2 lines · Carl Love
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.

Quoted 7 lines

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

author

2008-05-16 16:34 UTC #3 lore raw

Quoted 40 lines · Carl Love

Quoted 11 lines · Paul Mackerras
Quoted 2 lines · Carl Love
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.
Quoted 7 lines
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.

Quoted 2 lines

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