Re: [PATCH 4/5] powerpc/jprobes: Disable preemption when triggered through ftrace
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
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
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
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