Re: [PATCH 1/4] powerpc/kprobes: Disable preemption before invoking probe handler for optprobes

2017-10-27 Thread Naveen N. Rao 
On 2017/10/25 02:18AM, Masami Hiramatsu wrote:
> On Mon, 23 Oct 2017 22:07:38 +0530
> "Naveen N. Rao"  wrote:
> 
> > Per Documentation/kprobes.txt, probe handlers need to be invoked with
> > preemption disabled. Update optimized_callback() to do so. Also move
> > get_kprobe_ctlblk() invocation post preemption disable, since it
> > accesses pre-cpu data.
> > 
> > This was not an issue so far since optprobes wasn't selected if
> > CONFIG_PREEMPT was enabled. Commit a30b85df7d599f ("kprobes: Use
> > synchronize_rcu_tasks() for optprobe with CONFIG_PREEMPT=y") changes
> > this.
> 
> Actually, if you local_irq_save(), it also disables preempt. So unless you
> enables irqs, it should be safe.

I think we still need to disable preemption explicitly (at least on 
powerpc, disabling irqs doesn't increment preempt count).  See commit 
6baea433bc84cd ("powerpc/jprobes: Disable preemption when triggered 
through ftrace") for example.

We need to ensure preempt count is properly balanced for our usage 
across KPROBES_ON_FTRACE, OPTPROBES and POTS (plain old trap system ;-)

> 
> Acked-by: Masami Hiramatsu 

Thanks for the review!
- Naveen

Re: [PATCH 1/4] powerpc/kprobes: Disable preemption before invoking probe handler for optprobes

2017-10-24 Thread Masami Hiramatsu 
On Mon, 23 Oct 2017 22:07:38 +0530
"Naveen N. Rao"  wrote:

> Per Documentation/kprobes.txt, probe handlers need to be invoked with
> preemption disabled. Update optimized_callback() to do so. Also move
> get_kprobe_ctlblk() invocation post preemption disable, since it
> accesses pre-cpu data.
> 
> This was not an issue so far since optprobes wasn't selected if
> CONFIG_PREEMPT was enabled. Commit a30b85df7d599f ("kprobes: Use
> synchronize_rcu_tasks() for optprobe with CONFIG_PREEMPT=y") changes
> this.

Actually, if you local_irq_save(), it also disables preempt. So unless you
enables irqs, it should be safe.

Acked-by: Masami Hiramatsu 

Thank you,


> Signed-off-by: Naveen N. Rao 
> ---
>  arch/powerpc/kernel/optprobes.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index 91e037ab20a1..60ba7f1370a8 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -115,7 +115,6 @@ static unsigned long can_optimize(struct kprobe *p)
>  static void optimized_callback(struct optimized_kprobe *op,
>  struct pt_regs *regs)
>  {
> - struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
>   unsigned long flags;
>  
>   /* This is possible if op is under delayed unoptimizing */
> @@ -124,13 +123,14 @@ static void optimized_callback(struct optimized_kprobe 
> *op,
>  
>   local_irq_save(flags);
>   hard_irq_disable();
> + preempt_disable();
>  
>   if (kprobe_running()) {
>   kprobes_inc_nmissed_count(>kp);
>   } else {
>   __this_cpu_write(current_kprobe, >kp);
>   regs->nip = (unsigned long)op->kp.addr;
> - kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> + get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
>   opt_pre_handler(>kp, regs);
>   __this_cpu_write(current_kprobe, NULL);
>   }
> @@ -140,6 +140,7 @@ static void optimized_callback(struct optimized_kprobe 
> *op,
>* local_irq_restore() will re-enable interrupts,
>* if they were hard disabled.
>*/
> + preempt_enable_no_resched();
>   local_irq_restore(flags);
>  }
>  NOKPROBE_SYMBOL(optimized_callback);
> -- 
> 2.14.2
> 


-- 
Masami Hiramatsu

[PATCH 1/4] powerpc/kprobes: Disable preemption before invoking probe handler for optprobes

2017-10-23 Thread Naveen N. Rao 
Per Documentation/kprobes.txt, probe handlers need to be invoked with
preemption disabled. Update optimized_callback() to do so. Also move
get_kprobe_ctlblk() invocation post preemption disable, since it
accesses pre-cpu data.

This was not an issue so far since optprobes wasn't selected if
CONFIG_PREEMPT was enabled. Commit a30b85df7d599f ("kprobes: Use
synchronize_rcu_tasks() for optprobe with CONFIG_PREEMPT=y") changes
this.

Signed-off-by: Naveen N. Rao 
---
 arch/powerpc/kernel/optprobes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 91e037ab20a1..60ba7f1370a8 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -115,7 +115,6 @@ static unsigned long can_optimize(struct kprobe *p)
 static void optimized_callback(struct optimized_kprobe *op,
   struct pt_regs *regs)
 {
-   struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
unsigned long flags;
 
/* This is possible if op is under delayed unoptimizing */
@@ -124,13 +123,14 @@ static void optimized_callback(struct optimized_kprobe 
*op,
 
local_irq_save(flags);
hard_irq_disable();
+   preempt_disable();
 
if (kprobe_running()) {
kprobes_inc_nmissed_count(>kp);
} else {
__this_cpu_write(current_kprobe, >kp);
regs->nip = (unsigned long)op->kp.addr;
-   kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+   get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
opt_pre_handler(>kp, regs);
__this_cpu_write(current_kprobe, NULL);
}
@@ -140,6 +140,7 @@ static void optimized_callback(struct optimized_kprobe *op,
 * local_irq_restore() will re-enable interrupts,
 * if they were hard disabled.
 */
+   preempt_enable_no_resched();
local_irq_restore(flags);
 }
 NOKPROBE_SYMBOL(optimized_callback);
-- 
2.14.2