Patchwork [v2,2/3] ppc32/kprobe: complete kprobe and migrate exception frame

login
register
mail settings
Submitter Tiejun Chen
Date Dec. 15, 2011, 11 a.m.
Message ID <1323946810-4868-3-git-send-email-tiejun.chen@windriver.com>
Download mbox | patch
Permalink /patch/131564/
State Changes Requested
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Tiejun Chen - Dec. 15, 2011, 11 a.m.
We can't emulate stwu since that may corrupt current exception stack.
So we will have to do real store operation in the exception return code.

Firstly we'll allocate a trampoline exception frame below the kprobed
function stack and copy the current exception frame to the trampoline.
Then we can do this real store operation to implement 'stwu', and reroute
the trampoline frame to r1 to complete this exception migration.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/kernel/entry_32.S |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)
Benjamin Herrenschmidt - May 10, 2012, 3:50 a.m.
On Thu, 2011-12-15 at 19:00 +0800, Tiejun Chen wrote:
> We can't emulate stwu since that may corrupt current exception stack.
> So we will have to do real store operation in the exception return code.
> 
> Firstly we'll allocate a trampoline exception frame below the kprobed
> function stack and copy the current exception frame to the trampoline.
> Then we can do this real store operation to implement 'stwu', and reroute
> the trampoline frame to r1 to complete this exception migration.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>  arch/powerpc/kernel/entry_32.S |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index 56212bc..0cdd27d 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -850,6 +850,41 @@ resume_kernel:
>  
>  	/* interrupts are hard-disabled at this point */
>  restore:
> +	lwz	r3,_MSR(r1)	/* Returning to user mode? */
> +	andi.	r0,r3,MSR_PR
> +	bne	1f	

 .../...

Wouldn't it be better to use resume_kernel here ?

IE. We already have restore_user vs. resume_kernel labels, including
the PR test. In the !PREEMPT case, resume_kernel is empty but it's
there, so you can just "populate" it, ie, something like:

restore_user:
	... existing dbcr0 stuff ...
	b restore

resume_kernel: <-- removed the ifdef CONFIG_PREEMPT

	... Do the stack store business here...

#ifdef CONFIG_PREEMPT
	... move the preempt code here...
#endif

restore:
	... existing stuff ...

Also, the added advantage is that the code to calc
the thread info pointer and load the TI_FLAG can be
shared now between your stuff and preempt, ie:

resume_kernel:
	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
	lwz	r0,TI_FLAGS(r9)
	andis.	r0,r0,_TIF_EMULATE_STACK_STORE@h
	bne-	emulate_stack_store
#ifdef CONFIG_PREEMPT
	lwz	r8,TI_PREEMPT(r9) <-- note use of r8 instead of r0,
                                      I -think- r8 can be clobbered
                                      here but pls dbl check
	cmpwi	0,r8,0
	bne	restore
	andi.	r0,r0,_TIF_NEED_RESCHED
	etc...
 	
> +	/* check current_thread_info, _TIF_EMULATE_STACK_STORE */
> +	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
> +	lwz	r0,TI_FLAGS(r9)
> +	andis.	r0,r0,_TIF_EMULATE_STACK_STORE@h
> +	beq+	1f	
> +
> +	addi	r9,r1,INT_FRAME_SIZE	/* Get the kprobed function entry */
> +
> +	lwz	r3,GPR1(r1)
> +	subi	r3,r3,INT_FRAME_SIZE	/* dst: Allocate a trampoline exception frame */
> +	mr	r4,r1			/* src:  current exception frame */
> +	li	r5,INT_FRAME_SIZE	/* size: INT_FRAME_SIZE */
> +	mr	r1,r3			/* Reroute the trampoline frame to r1 */
> +	bl	memcpy			/* Copy from the original to the trampoline */
> +
> +	/* Do real store operation to complete stwu */
> +	lwz	r5,GPR1(r1)
> +	stw	r9,0(r5)

Ok, I think I -finally- understand your trick of using r1 +
INT_FRAME_SIZE as the value to store :-) It makes sense and it's
actually a nice hack :-)

I would recommend that in the C code part of the emulation, you
add some sanity checking to make sure we don't overflow the
kernel stack etc... it should come in generally handy especially
if what's your trying to debug with kprobes is a kernel stack
overflow :-)

> +	/* Clear _TIF_EMULATE_STACK_STORE flag */
> +	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
> +	lis	r11,_TIF_EMULATE_STACK_STORE@h
> +	addi	r9,r9,TI_FLAGS
> +0:	lwarx	r8,0,r9
> +	andc	r8,r8,r11
> +#ifdef CONFIG_IBM405_ERR77
> +	dcbt	0,r9
> +#endif
> +	stwcx.	r8,0,r9
> +	bne-	0b
> +1:
> +
>  #ifdef CONFIG_44x
>  BEGIN_MMU_FTR_SECTION
>  	b	1f

BTW. Are you going to do a ppc64 variant of that patch ?

Cheers,
Ben.
Tiejun Chen - June 3, 2012, 5:01 a.m.
On 05/10/2012 11:50 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2011-12-15 at 19:00 +0800, Tiejun Chen wrote:
>> We can't emulate stwu since that may corrupt current exception stack.
>> So we will have to do real store operation in the exception return code.
>>
>> Firstly we'll allocate a trampoline exception frame below the kprobed
>> function stack and copy the current exception frame to the trampoline.
>> Then we can do this real store operation to implement 'stwu', and reroute
>> the trampoline frame to r1 to complete this exception migration.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>>  arch/powerpc/kernel/entry_32.S |   35 +++++++++++++++++++++++++++++++++++
>>  1 files changed, 35 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
>> index 56212bc..0cdd27d 100644
>> --- a/arch/powerpc/kernel/entry_32.S
>> +++ b/arch/powerpc/kernel/entry_32.S
>> @@ -850,6 +850,41 @@ resume_kernel:
>>  
>>  	/* interrupts are hard-disabled at this point */
>>  restore:
>> +	lwz	r3,_MSR(r1)	/* Returning to user mode? */
>> +	andi.	r0,r3,MSR_PR
>> +	bne	1f	
> 
>  .../...
> 

Sorry for this delay response since I can't take time to do this last week :(

> Wouldn't it be better to use resume_kernel here ?

Agreed :)

> 
> IE. We already have restore_user vs. resume_kernel labels, including
> the PR test. In the !PREEMPT case, resume_kernel is empty but it's
> there, so you can just "populate" it, ie, something like:
> 
> restore_user:
> 	... existing dbcr0 stuff ...
> 	b restore
> 
> resume_kernel: <-- removed the ifdef CONFIG_PREEMPT
> 
> 	... Do the stack store business here...
> 
> #ifdef CONFIG_PREEMPT
> 	... move the preempt code here...
> #endif
> 
> restore:
> 	... existing stuff ...
> 
> Also, the added advantage is that the code to calc
> the thread info pointer and load the TI_FLAG can be
> shared now between your stuff and preempt, ie:
> 
> resume_kernel:
> 	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
> 	lwz	r0,TI_FLAGS(r9)
> 	andis.	r0,r0,_TIF_EMULATE_STACK_STORE@h
> 	bne-	emulate_stack_store
> #ifdef CONFIG_PREEMPT
> 	lwz	r8,TI_PREEMPT(r9) <-- note use of r8 instead of r0,
>                                       I -think- r8 can be clobbered
>                                       here but pls dbl check

Its should be safe.

> 	cmpwi	0,r8,0
> 	bne	restore
> 	andi.	r0,r0,_TIF_NEED_RESCHED
> 	etc...

Please check if next, v3, is fine.

>  	
>> +	/* check current_thread_info, _TIF_EMULATE_STACK_STORE */
>> +	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
>> +	lwz	r0,TI_FLAGS(r9)
>> +	andis.	r0,r0,_TIF_EMULATE_STACK_STORE@h
>> +	beq+	1f	
>> +
>> +	addi	r9,r1,INT_FRAME_SIZE	/* Get the kprobed function entry */
>> +
>> +	lwz	r3,GPR1(r1)
>> +	subi	r3,r3,INT_FRAME_SIZE	/* dst: Allocate a trampoline exception frame */
>> +	mr	r4,r1			/* src:  current exception frame */
>> +	li	r5,INT_FRAME_SIZE	/* size: INT_FRAME_SIZE */
>> +	mr	r1,r3			/* Reroute the trampoline frame to r1 */
>> +	bl	memcpy			/* Copy from the original to the trampoline */
>> +
>> +	/* Do real store operation to complete stwu */
>> +	lwz	r5,GPR1(r1)
>> +	stw	r9,0(r5)
> 
> Ok, I think I -finally- understand your trick of using r1 +
> INT_FRAME_SIZE as the value to store :-) It makes sense and it's
> actually a nice hack :-)
> 
> I would recommend that in the C code part of the emulation, you
> add some sanity checking to make sure we don't overflow the
> kernel stack etc... it should come in generally handy especially
> if what's your trying to debug with kprobes is a kernel stack
> overflow :-)

Added.

> 
>> +	/* Clear _TIF_EMULATE_STACK_STORE flag */
>> +	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
>> +	lis	r11,_TIF_EMULATE_STACK_STORE@h
>> +	addi	r9,r9,TI_FLAGS
>> +0:	lwarx	r8,0,r9
>> +	andc	r8,r8,r11
>> +#ifdef CONFIG_IBM405_ERR77
>> +	dcbt	0,r9
>> +#endif
>> +	stwcx.	r8,0,r9
>> +	bne-	0b
>> +1:
>> +
>>  #ifdef CONFIG_44x
>>  BEGIN_MMU_FTR_SECTION
>>  	b	1f
> 
> BTW. Are you going to do a ppc64 variant of that patch ?

I'd like to go ppc64 ASAP once we did on ppc32 is good enough :)

Tiejun

> 
> Cheers,
> Ben.
> 
> 
> 
>

Patch

diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 56212bc..0cdd27d 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -850,6 +850,41 @@  resume_kernel:
 
 	/* interrupts are hard-disabled at this point */
 restore:
+	lwz	r3,_MSR(r1)	/* Returning to user mode? */
+	andi.	r0,r3,MSR_PR
+	bne	1f	
+	/* check current_thread_info, _TIF_EMULATE_STACK_STORE */
+	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
+	lwz	r0,TI_FLAGS(r9)
+	andis.	r0,r0,_TIF_EMULATE_STACK_STORE@h
+	beq+	1f	
+
+	addi	r9,r1,INT_FRAME_SIZE	/* Get the kprobed function entry */
+
+	lwz	r3,GPR1(r1)
+	subi	r3,r3,INT_FRAME_SIZE	/* dst: Allocate a trampoline exception frame */
+	mr	r4,r1			/* src:  current exception frame */
+	li	r5,INT_FRAME_SIZE	/* size: INT_FRAME_SIZE */
+	mr	r1,r3			/* Reroute the trampoline frame to r1 */
+	bl	memcpy			/* Copy from the original to the trampoline */
+
+	/* Do real store operation to complete stwu */
+	lwz	r5,GPR1(r1)
+	stw	r9,0(r5)
+
+	/* Clear _TIF_EMULATE_STACK_STORE flag */
+	rlwinm	r9,r1,0,0,(31-THREAD_SHIFT)
+	lis	r11,_TIF_EMULATE_STACK_STORE@h
+	addi	r9,r9,TI_FLAGS
+0:	lwarx	r8,0,r9
+	andc	r8,r8,r11
+#ifdef CONFIG_IBM405_ERR77
+	dcbt	0,r9
+#endif
+	stwcx.	r8,0,r9
+	bne-	0b
+1:
+
 #ifdef CONFIG_44x
 BEGIN_MMU_FTR_SECTION
 	b	1f