[v2 PATCH 1/2] booke/kprobe: make program exception to use one dedicated exception stack

When kprobe these operations such as store-and-update-word for SP(r1),

stwu r1, -A(r1)

The program exception is triggered, and PPC always allocate an exception frame
as shown as the follows:

old r1 ----------
         ...
         nip
         gpr[2] ~ gpr[31]
         gpr[1] <--------- old r1 is stored.
         gpr[0]
       -------- <--------- pr_regs @offset 16 bytes
       padding
       STACK_FRAME_REGS_MARKER
       LR
       back chain
new r1 ----------
Then emulate_step() will emulate this instruction, 'stwu'. Actually its
equivalent to:
1> Update pr_regs->gpr[1] = mem[old r1 + (-A)]
2> stw [old r1], mem[old r1 + (-A)]

Please notice the stack based on new r1 may be covered with mem[old r1
+(-A)] when addr[old r1 + (-A)] < addr[old r1 + sizeof(an exception frame0].
So the above 2# operation will overwirte something to break this exception
frame then unexpected kernel problem will be issued.

So looks we have to implement independed interrupt stack for PPC program
exception when CONFIG_BOOKE is enabled. Here we can use
EXC_LEVEL_EXCEPTION_PROLOG to replace original NORMAL_EXCEPTION_PROLOG
for program exception if CONFIG_BOOKE. Then its always safe for kprobe
with independed exc stack from one pre-allocated and dedicated thread_info.
Actually this is just waht we did for critical/machine check exceptions
on PPC.

Signed-off-by: Tiejun Chen <redacted>
---
 arch/powerpc/include/asm/irq.h   |    3 +++
 arch/powerpc/include/asm/reg.h   |    4 ++++
 arch/powerpc/kernel/head_booke.h |   12 +++++++++++-
 arch/powerpc/kernel/irq.c        |   11 +++++++++++
 arch/powerpc/kernel/setup_32.c   |    4 ++++
 5 files changed, 33 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
index 1bff591..6d12169 100644
--- a/arch/powerpc/include/asm/irq.h
+++ b/arch/powerpc/include/asm/irq.h

@@ -313,6 +313,9 @@ struct pt_regs;
 extern struct thread_info *critirq_ctx[NR_CPUS];
 extern struct thread_info *dbgirq_ctx[NR_CPUS];
 extern struct thread_info *mcheckirq_ctx[NR_CPUS];
+#if defined(CONFIG_KPROBES) && defined(CONFIG_BOOKE)
+extern struct thread_info *pgirq_ctx[NR_CPUS];
+#endif
 extern void exc_lvl_ctx_init(void);
 #else
 #define exc_lvl_ctx_init()

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index c5cae0d..34d6178 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h

@@ -885,6 +885,10 @@
 #endif
 #define SPRN_SPRG_RVCPU		SPRN_SPRG1
 #define SPRN_SPRG_WVCPU		SPRN_SPRG1
+#ifdef	CONFIG_KPROBES
+#define	SPRN_SPRG_RSCRATCH_PG	SPRN_SPRG0
+#define	SPRN_SPRG_WSCRATCH_PG	SPRN_SPRG0
+#endif
 #endif
 
 #ifdef CONFIG_8xx

diff --git a/arch/powerpc/kernel/head_booke.h b/arch/powerpc/kernel/head_booke.h
index a0bf158..cf6cb1e 100644
--- a/arch/powerpc/kernel/head_booke.h
+++ b/arch/powerpc/kernel/head_booke.h

@@ -79,6 +79,10 @@
 /* only on e500mc/e200 */
 #define DBG_STACK_BASE		dbgirq_ctx
 
+#if defined(CONFIG_KPROBES)
+#define PG_STACK_BASE		pgirq_ctx
+#endif
+
 #define EXC_LVL_FRAME_OVERHEAD	(THREAD_SIZE - INT_FRAME_SIZE - EXC_LVL_SIZE)
 
 #ifdef CONFIG_SMP

@@ -158,6 +162,12 @@
 		EXC_LEVEL_EXCEPTION_PROLOG(DBG, SPRN_DSRR0, SPRN_DSRR1)
 #define MCHECK_EXCEPTION_PROLOG \
 		EXC_LEVEL_EXCEPTION_PROLOG(MC, SPRN_MCSRR0, SPRN_MCSRR1)
+#if defined(CONFIG_KPROBES)
+#define	PROGRAM_EXCEPTION_PROLOG \
+		EXC_LEVEL_EXCEPTION_PROLOG(PG, SPRN_SRR0, SPRN_SRR1)
+#else
+#define	PROGRAM_EXCEPTION_PROLOG	NORMAL_EXCEPTION_PROLOG
+#endif
 
 /*
  * Exception vectors.

@@ -370,7 +380,7 @@ label:
 
 #define PROGRAM_EXCEPTION						      \
 	START_EXCEPTION(Program)					      \
-	NORMAL_EXCEPTION_PROLOG;					      \
+	PROGRAM_EXCEPTION_PROLOG;					      \
 	mfspr	r4,SPRN_ESR;		/* Grab the ESR and save it */	      \
 	stw	r4,_ESR(r11);						      \
 	addi	r3,r1,STACK_FRAME_OVERHEAD;				      \

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 5b428e3..ff5b8dd 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c

@@ -397,6 +397,10 @@ struct thread_info   *critirq_ctx[NR_CPUS] __read_mostly;
 struct thread_info    *dbgirq_ctx[NR_CPUS] __read_mostly;
 struct thread_info *mcheckirq_ctx[NR_CPUS] __read_mostly;
 
+#if defined(CONFIG_KPROBES) && defined(CONFIG_BOOKE)
+struct thread_info    *pgirq_ctx[NR_CPUS] __read_mostly;
+#endif
+
 void exc_lvl_ctx_init(void)
 {
 	struct thread_info *tp;

@@ -423,6 +427,13 @@ void exc_lvl_ctx_init(void)
 		tp = mcheckirq_ctx[cpu_nr];
 		tp->cpu = cpu_nr;
 		tp->preempt_count = HARDIRQ_OFFSET;
+
+#if defined(CONFIG_KPROBES)
+		memset((void *)pgirq_ctx[i], 0, THREAD_SIZE);
+		tp = pgirq_ctx[i];
+		tp->cpu = i;
+		tp->preempt_count = 0;
+#endif
 #endif
 	}
 }

diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index 620d792..b872564 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c

@@ -272,6 +272,10 @@ static void __init exc_lvl_early_init(void)
 			__va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
 		mcheckirq_ctx[hw_cpu] = (struct thread_info *)
 			__va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
+#ifdef CONFIG_KPROBES
+		pgirq_ctx[hw_cpu] = (struct thread_info *)
+			__va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
+#endif
 #endif
 	}
 }

-- 
1.5.6

When enable CONFIG_PREEMPT we will trigger the following call trace:

BUG: scheduling while atomic: swapper/1/0x10000000
...

krpobe always goes through the following path:

program_check_exception()
        |
        + notify_die(DIE_BPT, "breakpoint",...)
                |
                + kprobe_handler()
                        |
                        + preempt_disable();
                        + break_handler() <- preempt_enable_no_resched()
                        + emulate_step()
                        + preempt_enable_no_resched()
                        ...
        exit

We should remove unnecessary preempt_enable_no_resched() inside of break_handler()
since looks longjmp_break_handler() always go the above path.

Signed-off-by: Tiejun Chen <redacted>
---
 arch/powerpc/kernel/kprobes.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index bc47352..a8a2a4d 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c

@@ -552,7 +552,6 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 	 * saved regs...
 	 */
 	memcpy(regs, &kcb->jprobe_saved_regs, sizeof(struct pt_regs));
-	preempt_enable_no_resched();
 	return 1;
 }
 

-- 
1.5.6

v1 -> v2: when allocate pgirq_ctx, use 'hw_cpu' to identify cpu ID in exc_lvl_early_init().

BTW, I already validated these patches on fsl mpc8536 by kprobe do_fork() and show_interrupts().

Note this bug I fixed is from another email thread, "[BUG?]3.0-rc4+ftrace+kprobe: 
set kprobe at instruction 'stwu' lead to system crash/freeze", Yong Zhang, [off-list ref]
previously reported.

Tiejun

On Mon, Jul 11, 2011 at 10:39:35AM +0800, Tiejun Chen wrote:

When enable CONFIG_PREEMPT we will trigger the following call trace:

BUG: scheduling while atomic: swapper/1/0x10000000
...

krpobe always goes through the following path:

program_check_exception()
        |
        + notify_die(DIE_BPT, "breakpoint",...)
                |
                + kprobe_handler()
                        |
                        + preempt_disable();
                        + break_handler() <- preempt_enable_no_resched()
                        + emulate_step()
                        + preempt_enable_no_resched()
                        ...
        exit

We should remove unnecessary preempt_enable_no_resched() inside of break_handler()
since looks longjmp_break_handler() always go the above path.

The current code is correct. Reasoning follows...

setjmp_pre_handler() and longjmp_break_handler() are used only for
jprobes. In the case of a jprobe, the code flow would be:

bp hit -> kprobe_handler() -> preempt_disable() -> setjmp_pre_handler()
(not that since this routine returns 1, we skip sstep here) -> jp->entry()
-> jprobe_return() -> bp hit -> kprobe_handler() -> preempt_disable() again
-> longjmp_break_handler() -> preempt_enable() -> sstep -> preempt_enable()
(for the second kprobe_handler() entry).

You could verify this with a preempt_count() printk with a
CONFIG_PREEMPT=y kernel.

Signed-off-by: Tiejun Chen <redacted>

Nack, sorry :-)

Ananth

Ananth N Mavinakayanahalli wrote:

On Mon, Jul 11, 2011 at 10:39:35AM +0800, Tiejun Chen wrote:

quoted

When enable CONFIG_PREEMPT we will trigger the following call trace:

BUG: scheduling while atomic: swapper/1/0x10000000
...

krpobe always goes through the following path:

program_check_exception()
        |
        + notify_die(DIE_BPT, "breakpoint",...)
                |
                + kprobe_handler()
                        |
                        + preempt_disable();
                        + break_handler() <- preempt_enable_no_resched()
                        + emulate_step()
                        + preempt_enable_no_resched()
                        ...
        exit

We should remove unnecessary preempt_enable_no_resched() inside of break_handler()
since looks longjmp_break_handler() always go the above path.

The current code is correct. Reasoning follows...

setjmp_pre_handler() and longjmp_break_handler() are used only for
jprobes. In the case of a jprobe, the code flow would be:

bp hit -> kprobe_handler() -> preempt_disable() -> setjmp_pre_handler()
(not that since this routine returns 1, we skip sstep here) -> jp->entry()
-> jprobe_return() -> bp hit -> kprobe_handler() -> preempt_disable() again
-> longjmp_break_handler() -> preempt_enable() -> sstep -> preempt_enable()
(for the second kprobe_handler() entry).

You could verify this with a preempt_count() printk with a
CONFIG_PREEMPT=y kernel.

quoted

Signed-off-by: Tiejun Chen <redacted>

Nack, sorry :-)

You're right.

When use EXC_LEVEL_EXCEPTION_PROLOG for Critical/Machine check, if the exception
came from kernel mode, we copy thread_info flags, *preempt*, and task pointer
from the process thread_info. So here I steal EXC_LEVEL_EXCEPTION_PROLOG for
Program Exception, preempt count would be corrupted incorrectly.

Thanks
Tiejun

Ananth

tiejun.chen wrote:

Ananth N Mavinakayanahalli wrote:

quoted

On Mon, Jul 11, 2011 at 10:39:35AM +0800, Tiejun Chen wrote:

quoted

When enable CONFIG_PREEMPT we will trigger the following call trace:

BUG: scheduling while atomic: swapper/1/0x10000000
...

krpobe always goes through the following path:

program_check_exception()
        |
        + notify_die(DIE_BPT, "breakpoint",...)
                |
                + kprobe_handler()
                        |
                        + preempt_disable();
                        + break_handler() <- preempt_enable_no_resched()
                        + emulate_step()
                        + preempt_enable_no_resched()
                        ...
        exit

We should remove unnecessary preempt_enable_no_resched() inside of break_handler()
since looks longjmp_break_handler() always go the above path.

The current code is correct. Reasoning follows...

setjmp_pre_handler() and longjmp_break_handler() are used only for
jprobes. In the case of a jprobe, the code flow would be:

bp hit -> kprobe_handler() -> preempt_disable() -> setjmp_pre_handler()
(not that since this routine returns 1, we skip sstep here) -> jp->entry()
-> jprobe_return() -> bp hit -> kprobe_handler() -> preempt_disable() again
-> longjmp_break_handler() -> preempt_enable() -> sstep -> preempt_enable()
(for the second kprobe_handler() entry).

You could verify this with a preempt_count() printk with a
CONFIG_PREEMPT=y kernel.

quoted

Signed-off-by: Tiejun Chen <redacted>

Nack, sorry :-)

You're right.

When use EXC_LEVEL_EXCEPTION_PROLOG for Critical/Machine check, if the exception
came from kernel mode, we copy thread_info flags, *preempt*, and task pointer
from the process thread_info. So here I steal EXC_LEVEL_EXCEPTION_PROLOG for
Program Exception, preempt count would be corrupted incorrectly.

Looks I miss the specific return-from-program-exc to restore those necessary
thread information like we did for debug exception with ret_from_debug_exc when
use EXC_LEVEL_EXCEPTION_PROLOG for debug exception.

Will update this on v3.

Tiejun

Thanks
Tiejun

quoted

Ananth

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev