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

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 <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/optprobes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

On Mon, 23 Oct 2017 22:07:38 +0530
"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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 <mhiramat@kernel.org>

Thank you,


> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  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(&op->kp);
>  	} else {
>  		__this_cpu_write(current_kprobe, &op->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(&op->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
>

On 2017/10/25 02:18AM, Masami Hiramatsu wrote:
> On Mon, 23 Oct 2017 22:07:38 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> 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 <mhiramat@kernel.org>

Thanks for the review!
- Naveen

On Mon, 2017-10-23 at 16:37:38 UTC, "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.
> 
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/8a2d71a3f2737e2448aa68de2b6052

cheers