Re: [PATCH 4/5] powerpc/jprobes: Disable preemption when triggered through ftrace

2017-09-14 Thread Masami Hiramatsu 
On Thu, 14 Sep 2017 15:55:39 +0530
"Naveen N. Rao"  wrote:

> On 2017/09/13 05:05PM, Masami Hiramatsu wrote:
> > On Thu, 14 Sep 2017 02:50:35 +0530
> > "Naveen N. Rao"  wrote:
> > 
> > > KPROBES_SANITY_TEST throws the below splat when CONFIG_PREEMPT is
> > > enabled:
> > > 
> > > [3.140410] Kprobe smoke test: started
> > > [3.149680] DEBUG_LOCKS_WARN_ON(val > preempt_count())
> > > [3.149684] [ cut here ]
> > > [3.149695] WARNING: CPU: 19 PID: 1 at kernel/sched/core.c:3094 
> > > preempt_count_sub+0xcc/0x140
> > > [3.149699] Modules linked in:
> > > [3.149705] CPU: 19 PID: 1 Comm: swapper/0 Not tainted 4.13.0-rc7-nnr+ 
> > > #97
> > > [3.149709] task: c000fea8 task.stack: c000feb0
> > > [3.149713] NIP:  c011d3dc LR: c011d3d8 CTR: 
> > > c0a090d0
> > > [3.149718] REGS: c000feb03400 TRAP: 0700   Not tainted  
> > > (4.13.0-rc7-nnr+)
> > > [3.149722] MSR:  80021033   CR: 28000282  
> > > XER: 
> > > [3.149732] CFAR: c015aa18 SOFTE: 0
> > > 
> > > [3.149786] NIP [c011d3dc] preempt_count_sub+0xcc/0x140
> > > [3.149790] LR [c011d3d8] preempt_count_sub+0xc8/0x140
> > > [3.149794] Call Trace:
> > > [3.149798] [c000feb03680] [c011d3d8] 
> > > preempt_count_sub+0xc8/0x140 (unreliable)
> > > [3.149804] [c000feb036e0] [c0046198] 
> > > kprobe_handler+0x228/0x4b0
> > > [3.149810] [c000feb03750] [c00269c8] 
> > > program_check_exception+0x58/0x3b0
> > > [3.149816] [c000feb037c0] [c000903c] 
> > > program_check_common+0x16c/0x170
> > > [3.149822] --- interrupt: 0 at kprobe_target+0x8/0x20
> > >LR = init_test_probes+0x248/0x7d0
> > > [3.149829] [c000feb03ab0] [c0e4f048] kp+0x0/0x80 
> > > (unreliable)
> > > [3.149835] [c000feb03b10] [c004ea60] 
> > > livepatch_handler+0x38/0x74
> > > [3.149841] [c000feb03ba0] [c0d0de54] 
> > > init_kprobes+0x1d8/0x208
> > > [3.149846] [c000feb03c40] [c000daa8] 
> > > do_one_initcall+0x68/0x1d0
> > > [3.149852] [c000feb03d00] [c0ce44f0] 
> > > kernel_init_freeable+0x298/0x374
> > > [3.149857] [c000feb03dc0] [c000dd84] 
> > > kernel_init+0x24/0x160
> > > [3.149863] [c000feb03e30] [c000bfec] 
> > > ret_from_kernel_thread+0x5c/0x70
> > > [3.149867] Instruction dump:
> > > [3.149871] 419effdc 3d22001b 39299240 8129 2f89 409effc8 
> > > 3c82ffcb 3c62ffcb
> > > [3.149879] 3884bc68 3863bc18 4803d5fd 6000 <0fe0> 4ba8 
> > > 6000 6000
> > > [3.149890] ---[ end trace 432dd46b4ce3d29f ]---
> > > [3.166003] Kprobe smoke test: passed successfully
> > > 
> > > The issue is that we aren't disabling preemption in
> > > kprobe_ftrace_handler(). Disable it.
> > 
> > Oops, right! Similar patch may need for x86 too.
> 
> Indeed, I will send a patch for that.
> 
> On a related note, I've been looking into why we have !PREEMPT for 
> CONFIG_OPTPROBES. It looks like the primary reason is x86 having to deal 
> with replacing multiple instructions. However, that isn't true with arm 
> and powerpc. So, does it make sense to move 'depends on !PREEMPT' to the 
> x86 code? Are there other scenarios where it might cause issues for 
> arm/powerpc?

Indeed! Whuat I expected was to replace several instructions in RISC processors
for jumping far site (like load & jump), but you chose different approach.
So there is no reason to prehibit that.

Thanks!


-- 
Masami Hiramatsu

Re: [PATCH 4/5] powerpc/jprobes: Disable preemption when triggered through ftrace

2017-09-14 Thread Naveen N. Rao 
On 2017/09/13 05:05PM, Masami Hiramatsu wrote:
> On Thu, 14 Sep 2017 02:50:35 +0530
> "Naveen N. Rao"  wrote:
> 
> > KPROBES_SANITY_TEST throws the below splat when CONFIG_PREEMPT is
> > enabled:
> > 
> > [3.140410] Kprobe smoke test: started
> > [3.149680] DEBUG_LOCKS_WARN_ON(val > preempt_count())
> > [3.149684] [ cut here ]
> > [3.149695] WARNING: CPU: 19 PID: 1 at kernel/sched/core.c:3094 
> > preempt_count_sub+0xcc/0x140
> > [3.149699] Modules linked in:
> > [3.149705] CPU: 19 PID: 1 Comm: swapper/0 Not tainted 4.13.0-rc7-nnr+ 
> > #97
> > [3.149709] task: c000fea8 task.stack: c000feb0
> > [3.149713] NIP:  c011d3dc LR: c011d3d8 CTR: 
> > c0a090d0
> > [3.149718] REGS: c000feb03400 TRAP: 0700   Not tainted  
> > (4.13.0-rc7-nnr+)
> > [3.149722] MSR:  80021033   CR: 28000282  
> > XER: 
> > [3.149732] CFAR: c015aa18 SOFTE: 0
> > 
> > [3.149786] NIP [c011d3dc] preempt_count_sub+0xcc/0x140
> > [3.149790] LR [c011d3d8] preempt_count_sub+0xc8/0x140
> > [3.149794] Call Trace:
> > [3.149798] [c000feb03680] [c011d3d8] 
> > preempt_count_sub+0xc8/0x140 (unreliable)
> > [3.149804] [c000feb036e0] [c0046198] 
> > kprobe_handler+0x228/0x4b0
> > [3.149810] [c000feb03750] [c00269c8] 
> > program_check_exception+0x58/0x3b0
> > [3.149816] [c000feb037c0] [c000903c] 
> > program_check_common+0x16c/0x170
> > [3.149822] --- interrupt: 0 at kprobe_target+0x8/0x20
> >LR = init_test_probes+0x248/0x7d0
> > [3.149829] [c000feb03ab0] [c0e4f048] kp+0x0/0x80 
> > (unreliable)
> > [3.149835] [c000feb03b10] [c004ea60] 
> > livepatch_handler+0x38/0x74
> > [3.149841] [c000feb03ba0] [c0d0de54] 
> > init_kprobes+0x1d8/0x208
> > [3.149846] [c000feb03c40] [c000daa8] 
> > do_one_initcall+0x68/0x1d0
> > [3.149852] [c000feb03d00] [c0ce44f0] 
> > kernel_init_freeable+0x298/0x374
> > [3.149857] [c000feb03dc0] [c000dd84] kernel_init+0x24/0x160
> > [3.149863] [c000feb03e30] [c000bfec] 
> > ret_from_kernel_thread+0x5c/0x70
> > [3.149867] Instruction dump:
> > [3.149871] 419effdc 3d22001b 39299240 8129 2f89 409effc8 
> > 3c82ffcb 3c62ffcb
> > [3.149879] 3884bc68 3863bc18 4803d5fd 6000 <0fe0> 4ba8 
> > 6000 6000
> > [3.149890] ---[ end trace 432dd46b4ce3d29f ]---
> > [3.166003] Kprobe smoke test: passed successfully
> > 
> > The issue is that we aren't disabling preemption in
> > kprobe_ftrace_handler(). Disable it.
> 
> Oops, right! Similar patch may need for x86 too.

Indeed, I will send a patch for that.

On a related note, I've been looking into why we have !PREEMPT for 
CONFIG_OPTPROBES. It looks like the primary reason is x86 having to deal 
with replacing multiple instructions. However, that isn't true with arm 
and powerpc. So, does it make sense to move 'depends on !PREEMPT' to the 
x86 code? Are there other scenarios where it might cause issues for 
arm/powerpc?

Thanks!
- Naveen

Re: [PATCH 4/5] powerpc/jprobes: Disable preemption when triggered through ftrace

2017-09-13 Thread Masami Hiramatsu 
On Thu, 14 Sep 2017 02:50:35 +0530
"Naveen N. Rao"  wrote:

> KPROBES_SANITY_TEST throws the below splat when CONFIG_PREEMPT is
> enabled:
> 
> [3.140410] Kprobe smoke test: started
> [3.149680] DEBUG_LOCKS_WARN_ON(val > preempt_count())
> [3.149684] [ cut here ]
> [3.149695] WARNING: CPU: 19 PID: 1 at kernel/sched/core.c:3094 
> preempt_count_sub+0xcc/0x140
> [3.149699] Modules linked in:
> [3.149705] CPU: 19 PID: 1 Comm: swapper/0 Not tainted 4.13.0-rc7-nnr+ #97
> [3.149709] task: c000fea8 task.stack: c000feb0
> [3.149713] NIP:  c011d3dc LR: c011d3d8 CTR: 
> c0a090d0
> [3.149718] REGS: c000feb03400 TRAP: 0700   Not tainted  
> (4.13.0-rc7-nnr+)
> [3.149722] MSR:  80021033   CR: 28000282  XER: 
> 
> [3.149732] CFAR: c015aa18 SOFTE: 0
> 
> [3.149786] NIP [c011d3dc] preempt_count_sub+0xcc/0x140
> [3.149790] LR [c011d3d8] preempt_count_sub+0xc8/0x140
> [3.149794] Call Trace:
> [3.149798] [c000feb03680] [c011d3d8] 
> preempt_count_sub+0xc8/0x140 (unreliable)
> [3.149804] [c000feb036e0] [c0046198] 
> kprobe_handler+0x228/0x4b0
> [3.149810] [c000feb03750] [c00269c8] 
> program_check_exception+0x58/0x3b0
> [3.149816] [c000feb037c0] [c000903c] 
> program_check_common+0x16c/0x170
> [3.149822] --- interrupt: 0 at kprobe_target+0x8/0x20
>LR = init_test_probes+0x248/0x7d0
> [3.149829] [c000feb03ab0] [c0e4f048] kp+0x0/0x80 (unreliable)
> [3.149835] [c000feb03b10] [c004ea60] 
> livepatch_handler+0x38/0x74
> [3.149841] [c000feb03ba0] [c0d0de54] init_kprobes+0x1d8/0x208
> [3.149846] [c000feb03c40] [c000daa8] 
> do_one_initcall+0x68/0x1d0
> [3.149852] [c000feb03d00] [c0ce44f0] 
> kernel_init_freeable+0x298/0x374
> [3.149857] [c000feb03dc0] [c000dd84] kernel_init+0x24/0x160
> [3.149863] [c000feb03e30] [c000bfec] 
> ret_from_kernel_thread+0x5c/0x70
> [3.149867] Instruction dump:
> [3.149871] 419effdc 3d22001b 39299240 8129 2f89 409effc8 3c82ffcb 
> 3c62ffcb
> [3.149879] 3884bc68 3863bc18 4803d5fd 6000 <0fe0> 4ba8 
> 6000 6000
> [3.149890] ---[ end trace 432dd46b4ce3d29f ]---
> [3.166003] Kprobe smoke test: passed successfully
> 
> The issue is that we aren't disabling preemption in
> kprobe_ftrace_handler(). Disable it.

Oops, right! Similar patch may need for x86 too.

Acked-by: Masami Hiramatsu 

Thanks!

> Fixes: ead514d5fb30a0 ("powerpc/kprobes: Add support for KPROBES_ON_FTRACE")
> Signed-off-by: Naveen N. Rao 
> ---
>  arch/powerpc/kernel/kprobes-ftrace.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/kprobes-ftrace.c 
> b/arch/powerpc/kernel/kprobes-ftrace.c
> index 6c089d9757c9..2d81404f818c 100644
> --- a/arch/powerpc/kernel/kprobes-ftrace.c
> +++ b/arch/powerpc/kernel/kprobes-ftrace.c
> @@ -65,6 +65,7 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long 
> parent_nip,
>   /* Disable irq for emulating a breakpoint and avoiding preempt */
>   local_irq_save(flags);
>   hard_irq_disable();
> + preempt_disable();
>  
>   p = get_kprobe((kprobe_opcode_t *)nip);
>   if (unlikely(!p) || kprobe_disabled(p))
> @@ -86,12 +87,18 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned 
> long parent_nip,
>   kcb->kprobe_status = KPROBE_HIT_ACTIVE;
>   if (!p->pre_handler || !p->pre_handler(p, regs))
>   __skip_singlestep(p, regs, kcb, orig_nip);
> - /*
> -  * If pre_handler returns !0, it sets regs->nip and
> -  * resets current kprobe.
> -  */
> + else {
> + /*
> +  * If pre_handler returns !0, it sets regs->nip and
> +  * resets current kprobe. In this case, we still need
> +  * to restore irq, but not preemption.
> +  */
> + local_irq_restore(flags);
> + return;
> + }
>   }
>  end:
> + preempt_enable_no_resched();
>   local_irq_restore(flags);
>  }
>  NOKPROBE_SYMBOL(kprobe_ftrace_handler);
> -- 
> 2.14.1
> 


-- 
Masami Hiramatsu

[PATCH 4/5] powerpc/jprobes: Disable preemption when triggered through ftrace

2017-09-13 Thread Naveen N. Rao 
KPROBES_SANITY_TEST throws the below splat when CONFIG_PREEMPT is
enabled:

[3.140410] Kprobe smoke test: started
[3.149680] DEBUG_LOCKS_WARN_ON(val > preempt_count())
[3.149684] [ cut here ]
[3.149695] WARNING: CPU: 19 PID: 1 at kernel/sched/core.c:3094 
preempt_count_sub+0xcc/0x140
[3.149699] Modules linked in:
[3.149705] CPU: 19 PID: 1 Comm: swapper/0 Not tainted 4.13.0-rc7-nnr+ #97
[3.149709] task: c000fea8 task.stack: c000feb0
[3.149713] NIP:  c011d3dc LR: c011d3d8 CTR: c0a090d0
[3.149718] REGS: c000feb03400 TRAP: 0700   Not tainted  
(4.13.0-rc7-nnr+)
[3.149722] MSR:  80021033   CR: 28000282  XER: 

[3.149732] CFAR: c015aa18 SOFTE: 0

[3.149786] NIP [c011d3dc] preempt_count_sub+0xcc/0x140
[3.149790] LR [c011d3d8] preempt_count_sub+0xc8/0x140
[3.149794] Call Trace:
[3.149798] [c000feb03680] [c011d3d8] 
preempt_count_sub+0xc8/0x140 (unreliable)
[3.149804] [c000feb036e0] [c0046198] kprobe_handler+0x228/0x4b0
[3.149810] [c000feb03750] [c00269c8] 
program_check_exception+0x58/0x3b0
[3.149816] [c000feb037c0] [c000903c] 
program_check_common+0x16c/0x170
[3.149822] --- interrupt: 0 at kprobe_target+0x8/0x20
   LR = init_test_probes+0x248/0x7d0
[3.149829] [c000feb03ab0] [c0e4f048] kp+0x0/0x80 (unreliable)
[3.149835] [c000feb03b10] [c004ea60] livepatch_handler+0x38/0x74
[3.149841] [c000feb03ba0] [c0d0de54] init_kprobes+0x1d8/0x208
[3.149846] [c000feb03c40] [c000daa8] do_one_initcall+0x68/0x1d0
[3.149852] [c000feb03d00] [c0ce44f0] 
kernel_init_freeable+0x298/0x374
[3.149857] [c000feb03dc0] [c000dd84] kernel_init+0x24/0x160
[3.149863] [c000feb03e30] [c000bfec] 
ret_from_kernel_thread+0x5c/0x70
[3.149867] Instruction dump:
[3.149871] 419effdc 3d22001b 39299240 8129 2f89 409effc8 3c82ffcb 
3c62ffcb
[3.149879] 3884bc68 3863bc18 4803d5fd 6000 <0fe0> 4ba8 6000 
6000
[3.149890] ---[ end trace 432dd46b4ce3d29f ]---
[3.166003] Kprobe smoke test: passed successfully

The issue is that we aren't disabling preemption in
kprobe_ftrace_handler(). Disable it.

Fixes: ead514d5fb30a0 ("powerpc/kprobes: Add support for KPROBES_ON_FTRACE")
Signed-off-by: Naveen N. Rao 
---
 arch/powerpc/kernel/kprobes-ftrace.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/kprobes-ftrace.c 
b/arch/powerpc/kernel/kprobes-ftrace.c
index 6c089d9757c9..2d81404f818c 100644
--- a/arch/powerpc/kernel/kprobes-ftrace.c
+++ b/arch/powerpc/kernel/kprobes-ftrace.c
@@ -65,6 +65,7 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long 
parent_nip,
/* Disable irq for emulating a breakpoint and avoiding preempt */
local_irq_save(flags);
hard_irq_disable();
+   preempt_disable();
 
p = get_kprobe((kprobe_opcode_t *)nip);
if (unlikely(!p) || kprobe_disabled(p))
@@ -86,12 +87,18 @@ void kprobe_ftrace_handler(unsigned long nip, unsigned long 
parent_nip,
kcb->kprobe_status = KPROBE_HIT_ACTIVE;
if (!p->pre_handler || !p->pre_handler(p, regs))
__skip_singlestep(p, regs, kcb, orig_nip);
-   /*
-* If pre_handler returns !0, it sets regs->nip and
-* resets current kprobe.
-*/
+   else {
+   /*
+* If pre_handler returns !0, it sets regs->nip and
+* resets current kprobe. In this case, we still need
+* to restore irq, but not preemption.
+*/
+   local_irq_restore(flags);
+   return;
+   }
}
 end:
+   preempt_enable_no_resched();
local_irq_restore(flags);
 }
 NOKPROBE_SYMBOL(kprobe_ftrace_handler);
-- 
2.14.1