[PATCH v2 0/3] ppc32/kprobe: Fix a bug for kprobe stwu r1

[PATCH v2 0/3] ppc32/kprobe: Fix a bug for kprobe stwu r1

[Tiejun Chen]

Changes from V1:

* use memcpy simply to withdraw copy_exc_stack
* add !(regs->msr & MSR_PR)) and
	WARN_ON(test_thread_flag(TIF_EMULATE_STACK_STORE));
  to make sure we're in goot path.
* move this migration process inside 'restore'
* clear TIF flag atomically 

Tiejun Chen (3):
      powerpc/kprobe: introduce a new thread flag
      ppc32/kprobe: complete kprobe and migrate exception frame
      ppc32/kprobe: don't emulate store when kprobe stwu r1

 arch/powerpc/include/asm/thread_info.h |    3 ++
 arch/powerpc/kernel/entry_32.S         |   35 ++++++++++++++++++++++++++++++++
 arch/powerpc/lib/sstep.c               |   25 +++++++++++++++++++++-
 3 files changed, 61 insertions(+), 2 deletions(-)

Tiejun

[PATCH v2 1/3] powerpc/kprobe: introduce a new thread flag

[Tiejun Chen]

We need to add a new thread flag, TIF_EMULATE_STACK_STORE,
for emulating stack store operation while exiting exception.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/include/asm/thread_info.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index 836f231..f414fee 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -112,6 +112,8 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_FREEZE		14	/* Freezing for suspend */
 #define TIF_SYSCALL_TRACEPOINT	15	/* syscall tracepoint instrumentation */
 #define TIF_RUNLATCH		16	/* Is the runlatch enabled? */
+#define TIF_EMULATE_STACK_STORE	17	/* Is an instruction emulation
+					   for stack store? */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -130,6 +132,7 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_FREEZE		(1<<TIF_FREEZE)
 #define _TIF_SYSCALL_TRACEPOINT	(1<<TIF_SYSCALL_TRACEPOINT)
 #define _TIF_RUNLATCH		(1<<TIF_RUNLATCH)
+#define _TIF_EMULATE_STACK_STORE	(1<<TIF_EMULATE_STACK_STORE)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
 				 _TIF_SECCOMP | _TIF_SYSCALL_TRACEPOINT)
 
-- 
1.5.6

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

[Tiejun Chen]

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	
+	/* 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
-- 
1.5.6

Re: [PATCH v2 2/3] ppc32/kprobe: complete kprobe and migrate exception frame

[Benjamin Herrenschmidt]

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.

Re: [PATCH v2 2/3] ppc32/kprobe: complete kprobe and migrate exception frame

[tiejun.chen]

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 v2 3/3] ppc32/kprobe: don't emulate store when kprobe stwu r1

[Tiejun Chen]

We don't do the real store operation for kprobing 'stwu Rx,(y)R1'
since this may corrupt the exception frame, now we will do this
operation safely in exception return code after migrate current
exception frame below the kprobed function stack.

So we only update gpr[1] here and trigger a thread flag to mask
this.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/lib/sstep.c |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 9a52349..5145d10 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -566,7 +566,7 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 	unsigned long int ea;
 	unsigned int cr, mb, me, sh;
 	int err;
-	unsigned long old_ra;
+	unsigned long old_ra, val3;
 	long ival;
 
 	opcode = instr >> 26;
@@ -1486,10 +1486,31 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr)
 		goto ldst_done;
 
 	case 36:	/* stw */
-	case 37:	/* stwu */
 		val = regs->gpr[rd];
 		err = write_mem(val, dform_ea(instr, regs), 4, regs);
 		goto ldst_done;
+	case 37:	/* stwu */
+		val = regs->gpr[rd];
+		val3 = dform_ea(instr, regs);
+		/*
+		 * For PPC32 we always use stwu to change stack point with r1. So
+		 * this emulated store may corrupt the exception frame, now we
+		 * have to provide the exception frame trampoline, which is pushed
+		 * below the kprobed function stack. So we only update gpr[1] but
+		 * don't emulate the real store operation. We will do real store
+		 * operation safely in exception return code by checking this flag.
+		 */
+		if ((ra == 1) && !(regs->msr & MSR_PR)) {
+			/*
+			 * Check if we already set since that means we'll
+			 * lose the previous value.
+			 */
+			WARN_ON(test_thread_flag(TIF_EMULATE_STACK_STORE));
+			set_thread_flag(TIF_EMULATE_STACK_STORE);
+			err = 0;
+		} else
+			err = write_mem(val, val3, 4, regs);
+		goto ldst_done;
 
 	case 38:	/* stb */
 	case 39:	/* stbu */
-- 
1.5.6

Re: [PATCH v2 0/3] ppc32/kprobe: Fix a bug for kprobe stwu r1

[tiejun.chen]

Tiejun Chen wrote:
> Changes from V1:
> 
> * use memcpy simply to withdraw copy_exc_stack
> * add !(regs->msr & MSR_PR)) and
> 	WARN_ON(test_thread_flag(TIF_EMULATE_STACK_STORE));
>   to make sure we're in goot path.
> * move this migration process inside 'restore'
> * clear TIF flag atomically 

Ben,

Is this series OK?

Thanks
Tiejun

> 
> Tiejun Chen (3):
>       powerpc/kprobe: introduce a new thread flag
>       ppc32/kprobe: complete kprobe and migrate exception frame
>       ppc32/kprobe: don't emulate store when kprobe stwu r1
> 
>  arch/powerpc/include/asm/thread_info.h |    3 ++
>  arch/powerpc/kernel/entry_32.S         |   35 ++++++++++++++++++++++++++++++++
>  arch/powerpc/lib/sstep.c               |   25 +++++++++++++++++++++-
>  3 files changed, 61 insertions(+), 2 deletions(-)
> 
> Tiejun

Re: [PATCH v2 0/3] ppc32/kprobe: Fix a bug for kprobe stwu r1

[Benjamin Herrenschmidt]

On Tue, 2012-01-10 at 17:15 +0800, tiejun.chen wrote:
> Tiejun Chen wrote:
> > Changes from V1:
> > 
> > * use memcpy simply to withdraw copy_exc_stack
> > * add !(regs->msr & MSR_PR)) and
> > 	WARN_ON(test_thread_flag(TIF_EMULATE_STACK_STORE));
> >   to make sure we're in goot path.
> > * move this migration process inside 'restore'
> > * clear TIF flag atomically 
> 
> Ben,
> 
> Is this series OK?

Not completely sorry. I was about to send you some comments a couple of
weeks ago then had to leave urgently and forgot about it, then started
rewriting your code and didn't finish :-)

I'll give you some more feedback asap, sorry about that. It's getting
there but there's a couple of things I'd like to see done a bit
differently.

Cheers,
Ben.

> Thanks
> Tiejun
> 
> > 
> > Tiejun Chen (3):
> >       powerpc/kprobe: introduce a new thread flag
> >       ppc32/kprobe: complete kprobe and migrate exception frame
> >       ppc32/kprobe: don't emulate store when kprobe stwu r1
> > 
> >  arch/powerpc/include/asm/thread_info.h |    3 ++
> >  arch/powerpc/kernel/entry_32.S         |   35 ++++++++++++++++++++++++++++++++
> >  arch/powerpc/lib/sstep.c               |   25 +++++++++++++++++++++-
> >  3 files changed, 61 insertions(+), 2 deletions(-)
> > 
> > Tiejun