[PATCH 0/3] arm64: kprobes: Fix some bugs in arm64 kprobes

[PATCH 0/3] arm64: kprobes: Fix some bugs in arm64 kprobes

[Masami Hiramatsu]

Hi,

Here are the patches which fixes kprobe bugs on arm64.

Naresh reported that recently ftracetest crashes kernel, and I found
there are 3 different bugs around the crash.

- Kprobes on arm64 doesn't recover pstate.D mask even if probed
  context masks pstate.D. This causes a real kernel crash if a
  kprobe is nested.
- Some symbols which are called from blacklisted function, are not
  blacklisted.
- Debug exception handlers on arm64 is using rcu_read_lock(). This
  doesn't crashes kernel, but kicks suspicious RCU usage warning if
  we put kprobes on the function which is called in idle context.

This series includes fixes for above bugs.

Thank you,

---

Masami Hiramatsu (3):
      arm64: kprobes: Recover pstate.D in single-step exception handler
      arm64: unwind: Prohibit probing on return_address()
      arm64: debug: Remove rcu_read_lock from debug exception


 arch/arm64/kernel/debug-monitors.c |   14 ++++++++------
 arch/arm64/kernel/probes/kprobes.c |    9 ++++++---
 arch/arm64/kernel/return_address.c |    4 +++-
 arch/arm64/kernel/stacktrace.c     |    3 +++
 4 files changed, 20 insertions(+), 10 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH 1/3] arm64: kprobes: Recover pstate.D in single-step exception handler

[Masami Hiramatsu]

On arm64, if a nested kprobes hit, it can crash the kernel with below
error message.

[  152.118921] Unexpected kernel single-step exception at EL1

This is because commit 7419333fa15e ("arm64: kprobe: Always clear
pstate.D in breakpoint exception handler") clears pstate.D always in
the nested kprobes. That is correct *unless* any nested kprobes
(single-stepping) runs inside other kprobes (including kprobes in
 user handler).

When the 1st kprobe hits, do_debug_exception() will be called. At this
point, debug exception (= pstate.D) must be masked (=1). When the 2nd
 (nested) kprobe is hit before single-step of the first kprobe, it
modifies debug exception clear (pstate.D = 0).
Then, when the 1st kprobe setting up single-step, it saves current
DAIF, mask DAIF, enable single-step, and restore DAIF.
However, since "D" flag in DAIF is cleared by the 2nd kprobe, the
single-step exception happens soon after restoring DAIF.

To solve this issue, this refers saved pstate register to check the
previous pstate.D and recover it if needed.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Fixes: commit 7419333fa15e ("arm64: kprobe: Always clear pstate.D in breakpoint exception handler")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/probes/kprobes.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index bd5dfffca272..6e1dc0bb4c82 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -201,12 +201,14 @@ spsr_set_debug_flag(struct pt_regs *regs, int mask)
  * interrupt occurrence in the period of exception return and  start of
  * out-of-line single-step, that result in wrongly single stepping
  * into the interrupt handler.
+ * This also controls debug flag, so that we can refer the saved pstate.
  */
 static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb,
 						struct pt_regs *regs)
 {
 	kcb->saved_irqflag = regs->pstate;
 	regs->pstate |= PSR_I_BIT;
+	spsr_set_debug_flag(regs, 0);
 }
 
 static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
@@ -216,6 +218,10 @@ static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
 		regs->pstate |= PSR_I_BIT;
 	else
 		regs->pstate &= ~PSR_I_BIT;
+
+	/* Recover pstate.D mask if needed */
+	if (kcb->saved_irqflag & PSR_D_BIT)
+		spsr_set_debug_flag(regs, 1);
 }
 
 static void __kprobes
@@ -245,15 +251,12 @@ static void __kprobes setup_singlestep(struct kprobe *p,
 		kcb->kprobe_status = KPROBE_HIT_SS;
 	}
 
-
 	if (p->ainsn.api.insn) {
 		/* prepare for single stepping */
 		slot = (unsigned long)p->ainsn.api.insn;
 
 		set_ss_context(kcb, slot);	/* mark pending ss */
 
-		spsr_set_debug_flag(regs, 0);
-
 		/* IRQs and single stepping do not mix well. */
 		kprobes_save_local_irqflag(kcb, regs);
 		kernel_enable_single_step(regs);

Re: [PATCH 1/3] arm64: kprobes: Recover pstate.D in single-step exception handler

[James Morse]

Hi!

On 18/07/2019 06:43, Masami Hiramatsu wrote:
> On arm64, if a nested kprobes hit, it can crash the kernel with below
> error message.
> 
> [  152.118921] Unexpected kernel single-step exception at EL1
> 
> This is because commit 7419333fa15e ("arm64: kprobe: Always clear
> pstate.D in breakpoint exception handler") clears pstate.D always in
> the nested kprobes. That is correct *unless* any nested kprobes
> (single-stepping) runs inside other kprobes (including kprobes in
>  user handler).

kprobes probing kprobes!? ... why do we support this?

We treat 'debug' as our highest exception level, it can interrupt pNMI and RAS-errors.
Letting it loop doesn't sound like a good idea.


> When the 1st kprobe hits, do_debug_exception() will be called. At this
> point, debug exception (= pstate.D) must be masked (=1).

> When the 2nd (nested) kprobe is hit before single-step of the first kprobe,

How does this happen?
I guess the kprobe-helper-function gets called in debug context, but surely you can't
kprobe a kprobe-helper-function? What stops this going in a loop?


> it modifies debug exception clear (pstate.D = 0).

After taking the first BRK, DAIF=0xf, everything is masked. When you take the second BRK
this shouldn't change.

Those spsr_set_debug_flag() calls are modifying the spsr in the regs structure, they only
become PSTATE when we eret for single-step.


> Then, when the 1st kprobe setting up single-step, it saves current
> DAIF, mask DAIF, enable single-step, and restore DAIF.

> However, since "D" flag in DAIF is cleared by the 2nd kprobe, the
> single-step exception happens soon after restoring DAIF.

PSTATE.D bit clearing should only be effective for the duration of the single-step.


> To solve this issue, this refers saved pstate register to check the
> previous pstate.D and recover it if needed.

(This sounds like undoing something that shouldn't have happened in the first place)


> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index bd5dfffca272..6e1dc0bb4c82 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -201,12 +201,14 @@ spsr_set_debug_flag(struct pt_regs *regs, int mask)
>   * interrupt occurrence in the period of exception return and  start of
>   * out-of-line single-step, that result in wrongly single stepping
>   * into the interrupt handler.
> + * This also controls debug flag, so that we can refer the saved pstate.
>   */
>  static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb,
>  						struct pt_regs *regs)
>  {
>  	kcb->saved_irqflag = regs->pstate;
>  	regs->pstate |= PSR_I_BIT;
> +	spsr_set_debug_flag(regs, 0);

(Nit: this is the only caller of spsr_set_debug_flag(), as we're modifing regs->pstate
directly here, can we lose the helper and just manipulate regs->pstate? )

>  }
>  
>  static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
> @@ -245,15 +251,12 @@ static void __kprobes setup_singlestep(struct kprobe *p,
>  		kcb->kprobe_status = KPROBE_HIT_SS;
>  	}
>
> -
>  	if (p->ainsn.api.insn) {
>  		/* prepare for single stepping */
>  		slot = (unsigned long)p->ainsn.api.insn;
>
>  		set_ss_context(kcb, slot);	/* mark pending ss */
>
> -		spsr_set_debug_flag(regs, 0);
> -
>  		/* IRQs and single stepping do not mix well. */
>  		kprobes_save_local_irqflag(kcb, regs);
>  		kernel_enable_single_step(regs);

These two hunks look like cleanup, could we do this separately from a fix for stable?



> @@ -216,6 +218,10 @@ static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
>  		regs->pstate |= PSR_I_BIT;
>  	else
>  		regs->pstate &= ~PSR_I_BIT;
> +
> +	/* Recover pstate.D mask if needed */
> +	if (kcb->saved_irqflag & PSR_D_BIT)
> +		spsr_set_debug_flag(regs, 1);
>  }

Ugh. .. I get it ..

I think the simplest summary of the problem is:
Kprobes unmasks debug exceptions for single-step, then leaves them unmasked when the
probed function is restarted.

I'd like to know more about this nested case, but I don't think its the simplest example
of this problem.
The commit message is describing both the interrupted and running PSTATE as PSTATE. I
think it would be clearer if you called the interrupted one SPSR (saved pstate register).
That's the value in the regs structure.


Please don't re-manipulate the flags, its overly verbose and we've already got this wrong
once! We should just blindly restore the DAIF setting we had before as its simpler.

Could we change kprobes_save_local_irqflag() to save the DAIF bits of pstate:
| kcb->saved_irqflag = regs->pstate & DAIF_MASK;
(DAIF_MASK is all four PSR bits)

So that we can then fix this in kprobes_restore_local_irqflag() with:
| regs->pstate &= ~DAIF_MASK;
| regs->pstate |= kcb->saved_irqflag

(the value splicing is needed because regs->pstate also holds the 'condition code' flags,
which could be modified by the single-step instruction, then depended on afterwards.)


Thanks,

James

Re: [PATCH 1/3] arm64: kprobes: Recover pstate.D in single-step exception handler

[Masami Hiramatsu]

On Fri, 19 Jul 2019 11:07:33 +0100
James Morse <james.morse@arm.com> wrote:

> Hi!
> 
> On 18/07/2019 06:43, Masami Hiramatsu wrote:
> > On arm64, if a nested kprobes hit, it can crash the kernel with below
> > error message.
> > 
> > [  152.118921] Unexpected kernel single-step exception at EL1
> > 
> > This is because commit 7419333fa15e ("arm64: kprobe: Always clear
> > pstate.D in breakpoint exception handler") clears pstate.D always in
> > the nested kprobes. That is correct *unless* any nested kprobes
> > (single-stepping) runs inside other kprobes (including kprobes in
> >  user handler).
> 
> kprobes probing kprobes!? ... why do we support this?

That is NOT "supported", this is just for avoiding kernel crash as much
as possible.

The nested kprobe can unexpectedly happen, usually in kprobe user handlers.
Also, the critical functions outside of kprobe user handler, are blacklisted
(e.g. [PATCH 2/3]).

> We treat 'debug' as our highest exception level, it can interrupt pNMI and RAS-errors.
> Letting it loop doesn't sound like a good idea.

Agreed. that must be avoided.

> > When the 1st kprobe hits, do_debug_exception() will be called. At this
> > point, debug exception (= pstate.D) must be masked (=1).
> 
> > When the 2nd (nested) kprobe is hit before single-step of the first kprobe,
> 
> How does this happen?

As you may know, kprobes provides a handler interface to user. User must
carefully program the handler which doesn't cause this recursion, but
if he failed, he can put another probe on the function which is called
(indirectly) from the user handler.

> I guess the kprobe-helper-function gets called in debug context, but surely you can't
> kprobe a kprobe-helper-function? What stops this going in a loop?

The kprobe blacklist. __kprobes and NOKPROBE_SYMBOL() will blacklist it.

> > it modifies debug exception clear (pstate.D = 0).
> 
> After taking the first BRK, DAIF=0xf, everything is masked. When you take the second BRK
> this shouldn't change.

Yes, at the beginning of the 2nd BRK hit.
However in the 2nd BRK, it clears the D flag for single-step, and never recover it.

> 
> Those spsr_set_debug_flag() calls are modifying the spsr in the regs structure, they only
> become PSTATE when we eret for single-step.
> 
> 
> > Then, when the 1st kprobe setting up single-step, it saves current
> > DAIF, mask DAIF, enable single-step, and restore DAIF.
> 
> > However, since "D" flag in DAIF is cleared by the 2nd kprobe, the
> > single-step exception happens soon after restoring DAIF.
> 
> PSTATE.D bit clearing should only be effective for the duration of the single-step.
> 
> 
> > To solve this issue, this refers saved pstate register to check the
> > previous pstate.D and recover it if needed.
> 
> (This sounds like undoing something that shouldn't have happened in the first place)
> 
> 
> > diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> > index bd5dfffca272..6e1dc0bb4c82 100644
> > --- a/arch/arm64/kernel/probes/kprobes.c
> > +++ b/arch/arm64/kernel/probes/kprobes.c
> > @@ -201,12 +201,14 @@ spsr_set_debug_flag(struct pt_regs *regs, int mask)
> >   * interrupt occurrence in the period of exception return and  start of
> >   * out-of-line single-step, that result in wrongly single stepping
> >   * into the interrupt handler.
> > + * This also controls debug flag, so that we can refer the saved pstate.
> >   */
> >  static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb,
> >  						struct pt_regs *regs)
> >  {
> >  	kcb->saved_irqflag = regs->pstate;
> >  	regs->pstate |= PSR_I_BIT;
> > +	spsr_set_debug_flag(regs, 0);
> 
> (Nit: this is the only caller of spsr_set_debug_flag(), as we're modifing regs->pstate
> directly here, can we lose the helper and just manipulate regs->pstate? )
> 
> >  }
> >  
> >  static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
> > @@ -245,15 +251,12 @@ static void __kprobes setup_singlestep(struct kprobe *p,
> >  		kcb->kprobe_status = KPROBE_HIT_SS;
> >  	}
> >
> > -
> >  	if (p->ainsn.api.insn) {
> >  		/* prepare for single stepping */
> >  		slot = (unsigned long)p->ainsn.api.insn;
> >
> >  		set_ss_context(kcb, slot);	/* mark pending ss */
> >
> > -		spsr_set_debug_flag(regs, 0);
> > -
> >  		/* IRQs and single stepping do not mix well. */
> >  		kprobes_save_local_irqflag(kcb, regs);
> >  		kernel_enable_single_step(regs);
> 
> These two hunks look like cleanup, could we do this separately from a fix for stable?
> 
> 
> 
> > @@ -216,6 +218,10 @@ static void __kprobes kprobes_restore_local_irqflag(struct kprobe_ctlblk *kcb,
> >  		regs->pstate |= PSR_I_BIT;
> >  	else
> >  		regs->pstate &= ~PSR_I_BIT;
> > +
> > +	/* Recover pstate.D mask if needed */
> > +	if (kcb->saved_irqflag & PSR_D_BIT)
> > +		spsr_set_debug_flag(regs, 1);
> >  }
> 
> Ugh. .. I get it ..
> 
> I think the simplest summary of the problem is:
> Kprobes unmasks debug exceptions for single-step, then leaves them unmasked when the
> probed function is restarted.
> 
> I'd like to know more about this nested case, but I don't think its the simplest example
> of this problem.
> The commit message is describing both the interrupted and running PSTATE as PSTATE. I
> think it would be clearer if you called the interrupted one SPSR (saved pstate register).
> That's the value in the regs structure.

OK. I'm not sure why the original code decided only recovers irq flag.

> 
> Please don't re-manipulate the flags, its overly verbose and we've already got this wrong
> once! We should just blindly restore the DAIF setting we had before as its simpler.
> 
> Could we change kprobes_save_local_irqflag() to save the DAIF bits of pstate:
> | kcb->saved_irqflag = regs->pstate & DAIF_MASK;
> (DAIF_MASK is all four PSR bits)
> 
> So that we can then fix this in kprobes_restore_local_irqflag() with:
> | regs->pstate &= ~DAIF_MASK;
> | regs->pstate |= kcb->saved_irqflag

OK, that is much simpler and easy to understand.

> 
> (the value splicing is needed because regs->pstate also holds the 'condition code' flags,
> which could be modified by the single-step instruction, then depended on afterwards.)

Yes, that must be hold.

Thank you,

> 
> 
> Thanks,
> 
> James


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH 2/3] arm64: unwind: Prohibit probing on return_address()

[Masami Hiramatsu]

Prohibit probing on return_address() and subroutines which
is called from return_address(), since the it is invoked from
trace_hardirqs_off() which is also kprobe blacklisted.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/return_address.c |    4 +++-
 arch/arm64/kernel/stacktrace.c     |    3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
index b21cba90f82d..7f8a143268b0 100644
--- a/arch/arm64/kernel/return_address.c
+++ b/arch/arm64/kernel/return_address.c
@@ -8,6 +8,7 @@
 
 #include <linux/export.h>
 #include <linux/ftrace.h>
+#include <linux/kprobes.h>
 
 #include <asm/stack_pointer.h>
 #include <asm/stacktrace.h>
@@ -17,7 +18,7 @@ struct return_address_data {
 	void *addr;
 };
 
-static int save_return_addr(struct stackframe *frame, void *d)
+static nokprobe_inline int save_return_addr(struct stackframe *frame, void *d)
 {
 	struct return_address_data *data = d;
 
@@ -52,3 +53,4 @@ void *return_address(unsigned int level)
 		return NULL;
 }
 EXPORT_SYMBOL_GPL(return_address);
+NOKPROBE_SYMBOL(return_address);
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 62d395151abe..cd7dab54d17b 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -7,6 +7,7 @@
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/ftrace.h>
+#include <linux/kprobes.h>
 #include <linux/sched.h>
 #include <linux/sched/debug.h>
 #include <linux/sched/task_stack.h>
@@ -73,6 +74,7 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 
 	return 0;
 }
+NOKPROBE_SYMBOL(unwind_frame);
 
 void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
 		     int (*fn)(struct stackframe *, void *), void *data)
@@ -87,6 +89,7 @@ void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
 			break;
 	}
 }
+NOKPROBE_SYMBOL(walk_stackframe);
 
 #ifdef CONFIG_STACKTRACE
 struct stack_trace_data {

[PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[Masami Hiramatsu]

Remove rcu_read_lock()/rcu_read_unlock() from debug exception
handlers since the software breakpoint can be hit on idle task.

Actually, we don't need it because those handlers run in exception
context where the interrupts are disabled. This means those are never
preempted.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Cc: Paul E. McKenney <paulmck@linux.ibm.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/debug-monitors.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index f8719bd30850..48222a4760c2 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -207,16 +207,16 @@ static int call_step_hook(struct pt_regs *regs, unsigned int esr)
 
 	list = user_mode(regs) ? &user_step_hook : &kernel_step_hook;
 
-	rcu_read_lock();
-
+	/*
+	 * Since single-step exception disables interrupt, this function is
+	 * entirely not preemptible, and we can use rcu list safely here.
+	 */
 	list_for_each_entry_rcu(hook, list, node)	{
 		retval = hook->fn(regs, esr);
 		if (retval == DBG_HOOK_HANDLED)
 			break;
 	}
 
-	rcu_read_unlock();
-
 	return retval;
 }
 NOKPROBE_SYMBOL(call_step_hook);
@@ -305,14 +305,16 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
 
 	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
 
-	rcu_read_lock();
+	/*
+	 * Since brk exception disables interrupt, this function is
+	 * entirely not preemptible, and we can use rcu list safely here.
+	 */
 	list_for_each_entry_rcu(hook, list, node) {
 		unsigned int comment = esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
 
 		if ((comment & ~hook->mask) == hook->imm)
 			fn = hook->fn;
 	}
-	rcu_read_unlock();
 
 	return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
 }

Re: [PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[Paul E. McKenney]

On Thu, Jul 18, 2019 at 02:43:58PM +0900, Masami Hiramatsu wrote:
> Remove rcu_read_lock()/rcu_read_unlock() from debug exception
> handlers since the software breakpoint can be hit on idle task.

The exception entry and exit use irq_enter() and irq_exit(), in this
case, correct?  Otherwise RCU will be ignoring this CPU.

							Thanx, Paul

> Actually, we don't need it because those handlers run in exception
> context where the interrupts are disabled. This means those are never
> preempted.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Cc: Paul E. McKenney <paulmck@linux.ibm.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/arm64/kernel/debug-monitors.c |   14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index f8719bd30850..48222a4760c2 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -207,16 +207,16 @@ static int call_step_hook(struct pt_regs *regs, unsigned int esr)
>  
>  	list = user_mode(regs) ? &user_step_hook : &kernel_step_hook;
>  
> -	rcu_read_lock();
> -
> +	/*
> +	 * Since single-step exception disables interrupt, this function is
> +	 * entirely not preemptible, and we can use rcu list safely here.
> +	 */
>  	list_for_each_entry_rcu(hook, list, node)	{
>  		retval = hook->fn(regs, esr);
>  		if (retval == DBG_HOOK_HANDLED)
>  			break;
>  	}
>  
> -	rcu_read_unlock();
> -
>  	return retval;
>  }
>  NOKPROBE_SYMBOL(call_step_hook);
> @@ -305,14 +305,16 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
>  
>  	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
>  
> -	rcu_read_lock();
> +	/*
> +	 * Since brk exception disables interrupt, this function is
> +	 * entirely not preemptible, and we can use rcu list safely here.
> +	 */
>  	list_for_each_entry_rcu(hook, list, node) {
>  		unsigned int comment = esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
>  
>  		if ((comment & ~hook->mask) == hook->imm)
>  			fn = hook->fn;
>  	}
> -	rcu_read_unlock();
>  
>  	return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
>  }
> 

Re: [PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[Mark Rutland]

On Wed, Jul 17, 2019 at 11:22:15PM -0700, Paul E. McKenney wrote:
> On Thu, Jul 18, 2019 at 02:43:58PM +0900, Masami Hiramatsu wrote:
> > Remove rcu_read_lock()/rcu_read_unlock() from debug exception
> > handlers since the software breakpoint can be hit on idle task.

Why precisely do we need to elide these? Are we seeing warnings today?

> The exception entry and exit use irq_enter() and irq_exit(), in this
> case, correct?  Otherwise RCU will be ignoring this CPU.

This is missing today, which sounds like the underlying bug.

Thanks,
Mark.

> 
> 							Thanx, Paul
> 
> > Actually, we don't need it because those handlers run in exception
> > context where the interrupts are disabled. This means those are never
> > preempted.
> > 
> > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> > Cc: Paul E. McKenney <paulmck@linux.ibm.com>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  arch/arm64/kernel/debug-monitors.c |   14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> > index f8719bd30850..48222a4760c2 100644
> > --- a/arch/arm64/kernel/debug-monitors.c
> > +++ b/arch/arm64/kernel/debug-monitors.c
> > @@ -207,16 +207,16 @@ static int call_step_hook(struct pt_regs *regs, unsigned int esr)
> >  
> >  	list = user_mode(regs) ? &user_step_hook : &kernel_step_hook;
> >  
> > -	rcu_read_lock();
> > -
> > +	/*
> > +	 * Since single-step exception disables interrupt, this function is
> > +	 * entirely not preemptible, and we can use rcu list safely here.
> > +	 */
> >  	list_for_each_entry_rcu(hook, list, node)	{
> >  		retval = hook->fn(regs, esr);
> >  		if (retval == DBG_HOOK_HANDLED)
> >  			break;
> >  	}
> >  
> > -	rcu_read_unlock();
> > -
> >  	return retval;
> >  }
> >  NOKPROBE_SYMBOL(call_step_hook);
> > @@ -305,14 +305,16 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
> >  
> >  	list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
> >  
> > -	rcu_read_lock();
> > +	/*
> > +	 * Since brk exception disables interrupt, this function is
> > +	 * entirely not preemptible, and we can use rcu list safely here.
> > +	 */
> >  	list_for_each_entry_rcu(hook, list, node) {
> >  		unsigned int comment = esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
> >  
> >  		if ((comment & ~hook->mask) == hook->imm)
> >  			fn = hook->fn;
> >  	}
> > -	rcu_read_unlock();
> >  
> >  	return fn ? fn(regs, esr) : DBG_HOOK_ERROR;
> >  }
> > 

Re: [PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[Masami Hiramatsu]

On Thu, 18 Jul 2019 10:20:23 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Wed, Jul 17, 2019 at 11:22:15PM -0700, Paul E. McKenney wrote:
> > On Thu, Jul 18, 2019 at 02:43:58PM +0900, Masami Hiramatsu wrote:
> > > Remove rcu_read_lock()/rcu_read_unlock() from debug exception
> > > handlers since the software breakpoint can be hit on idle task.
> 
> Why precisely do we need to elide these? Are we seeing warnings today?

Yes, unfortunately, or fortunately. Naresh reported that warns when
ftracetest ran. I confirmed that happens if I probe on default_idle_call too.

/sys/kernel/debug/tracing # echo p default_idle_call >> kprobe_events 
/sys/kernel/debug/tracing # echo 1 > events/kprobes/enable 
/sys/kernel/debug/tracing # [  135.122237] 
[  135.125035] =============================
[  135.125310] WARNING: suspicious RCU usage
[  135.125581] 5.2.0-08445-g9187c508bdc7 #20 Not tainted
[  135.125904] -----------------------------
[  135.126205] include/linux/rcupdate.h:594 rcu_read_lock() used illegally while idle!
[  135.126839] 
[  135.126839] other info that might help us debug this:
[  135.126839] 
[  135.127410] 
[  135.127410] RCU used illegally from idle CPU!
[  135.127410] rcu_scheduler_active = 2, debug_locks = 1
[  135.128114] RCU used illegally from extended quiescent state!
[  135.128555] 1 lock held by swapper/0/0:
[  135.128944]  #0: (____ptrval____) (rcu_read_lock){....}, at: call_break_hook+0x0/0x178
[  135.130499] 
[  135.130499] stack backtrace:
[  135.131192] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.2.0-08445-g9187c508bdc7 #20
[  135.131841] Hardware name: linux,dummy-virt (DT)
[  135.132224] Call trace:
[  135.132491]  dump_backtrace+0x0/0x140
[  135.132806]  show_stack+0x24/0x30
[  135.133133]  dump_stack+0xc4/0x10c
[  135.133726]  lockdep_rcu_suspicious+0xf8/0x108
[  135.134171]  call_break_hook+0x170/0x178
[  135.134486]  brk_handler+0x28/0x68
[  135.134792]  do_debug_exception+0x90/0x150
[  135.135051]  el1_dbg+0x18/0x8c
[  135.135260]  default_idle_call+0x0/0x44
[  135.135516]  cpu_startup_entry+0x2c/0x30
[  135.135815]  rest_init+0x1b0/0x280
[  135.136044]  arch_call_rest_init+0x14/0x1c
[  135.136305]  start_kernel+0x4d4/0x500
[  135.136597] 


> 
> > The exception entry and exit use irq_enter() and irq_exit(), in this
> > case, correct?  Otherwise RCU will be ignoring this CPU.
> 
> This is missing today, which sounds like the underlying bug.

Agreed. I'm not so familier with how debug exception is handled on arm64,
would it be a kind of NMI or IRQ?

Anyway, it seems that normal irqs are also not calling irq_enter/exit
except for arch/arm64/kernel/smp.c. We need to fix that too for avoiding
unexpected RCU issues.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[James Morse]

Hi,

On 7/18/19 3:31 PM, Masami Hiramatsu wrote:
> On Thu, 18 Jul 2019 10:20:23 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
>> On Wed, Jul 17, 2019 at 11:22:15PM -0700, Paul E. McKenney wrote:
>>> On Thu, Jul 18, 2019 at 02:43:58PM +0900, Masami Hiramatsu wrote:
>>>> Remove rcu_read_lock()/rcu_read_unlock() from debug exception
>>>> handlers since the software breakpoint can be hit on idle task.
>>
>> Why precisely do we need to elide these? Are we seeing warnings today?
> 
> Yes, unfortunately, or fortunately. Naresh reported that warns when
> ftracetest ran. I confirmed that happens if I probe on default_idle_call too.
> 
> /sys/kernel/debug/tracing # echo p default_idle_call >> kprobe_events
> /sys/kernel/debug/tracing # echo 1 > events/kprobes/enable
> /sys/kernel/debug/tracing # [  135.122237]
> [  135.125035] =============================
> [  135.125310] WARNING: suspicious RCU usage

> [  135.132224] Call trace:
> [  135.132491]  dump_backtrace+0x0/0x140
> [  135.132806]  show_stack+0x24/0x30
> [  135.133133]  dump_stack+0xc4/0x10c
> [  135.133726]  lockdep_rcu_suspicious+0xf8/0x108
> [  135.134171]  call_break_hook+0x170/0x178
> [  135.134486]  brk_handler+0x28/0x68
> [  135.134792]  do_debug_exception+0x90/0x150
> [  135.135051]  el1_dbg+0x18/0x8c
> [  135.135260]  default_idle_call+0x0/0x44
> [  135.135516]  cpu_startup_entry+0x2c/0x30
> [  135.135815]  rest_init+0x1b0/0x280
> [  135.136044]  arch_call_rest_init+0x14/0x1c
> [  135.136305]  start_kernel+0x4d4/0x500

>>> The exception entry and exit use irq_enter() and irq_exit(), in this
>>> case, correct?  Otherwise RCU will be ignoring this CPU.
>>
>> This is missing today, which sounds like the underlying bug.
> 
> Agreed. I'm not so familier with how debug exception is handled on arm64,
> would it be a kind of NMI or IRQ?

Debug exceptions can interrupt both SError (think: machine check) and 
pseudo-NMI, which both in turn interrupt interrupt-masked code. So they 
are a kind of NMI. But, be careful not to call 'nmi_enter()' twice, see 
do_serror() for how we work around this...


> Anyway, it seems that normal irqs are also not calling irq_enter/exit
> except for arch/arm64/kernel/smp.c
drivers/irqchip/irq-gic.c:gic_handle_irq() either calls 
handle_domain_irq() or handle_IPI(). The enter/exit calls live in those 
functions.


Thanks,

James

Re: [PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[Masami Hiramatsu]

Hi James,

On Fri, 19 Jul 2019 09:42:05 +0100
James Morse <james.morse@arm.com> wrote:

> Hi,
> 
> On 7/18/19 3:31 PM, Masami Hiramatsu wrote:
> > On Thu, 18 Jul 2019 10:20:23 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> >> On Wed, Jul 17, 2019 at 11:22:15PM -0700, Paul E. McKenney wrote:
> >>> On Thu, Jul 18, 2019 at 02:43:58PM +0900, Masami Hiramatsu wrote:
> >>>> Remove rcu_read_lock()/rcu_read_unlock() from debug exception
> >>>> handlers since the software breakpoint can be hit on idle task.
> >>
> >> Why precisely do we need to elide these? Are we seeing warnings today?
> > 
> > Yes, unfortunately, or fortunately. Naresh reported that warns when
> > ftracetest ran. I confirmed that happens if I probe on default_idle_call too.
> > 
> > /sys/kernel/debug/tracing # echo p default_idle_call >> kprobe_events
> > /sys/kernel/debug/tracing # echo 1 > events/kprobes/enable
> > /sys/kernel/debug/tracing # [  135.122237]
> > [  135.125035] =============================
> > [  135.125310] WARNING: suspicious RCU usage
> 
> > [  135.132224] Call trace:
> > [  135.132491]  dump_backtrace+0x0/0x140
> > [  135.132806]  show_stack+0x24/0x30
> > [  135.133133]  dump_stack+0xc4/0x10c
> > [  135.133726]  lockdep_rcu_suspicious+0xf8/0x108
> > [  135.134171]  call_break_hook+0x170/0x178
> > [  135.134486]  brk_handler+0x28/0x68
> > [  135.134792]  do_debug_exception+0x90/0x150
> > [  135.135051]  el1_dbg+0x18/0x8c
> > [  135.135260]  default_idle_call+0x0/0x44
> > [  135.135516]  cpu_startup_entry+0x2c/0x30
> > [  135.135815]  rest_init+0x1b0/0x280
> > [  135.136044]  arch_call_rest_init+0x14/0x1c
> > [  135.136305]  start_kernel+0x4d4/0x500
> 
> >>> The exception entry and exit use irq_enter() and irq_exit(), in this
> >>> case, correct?  Otherwise RCU will be ignoring this CPU.
> >>
> >> This is missing today, which sounds like the underlying bug.
> > 
> > Agreed. I'm not so familier with how debug exception is handled on arm64,
> > would it be a kind of NMI or IRQ?
> 
> Debug exceptions can interrupt both SError (think: machine check) and 
> pseudo-NMI, which both in turn interrupt interrupt-masked code. So they 
> are a kind of NMI. But, be careful not to call 'nmi_enter()' twice, see 
> do_serror() for how we work around this...

OK. I think we can use rcu_nmi_enter/exit() as same as x86.

> > Anyway, it seems that normal irqs are also not calling irq_enter/exit
> > except for arch/arm64/kernel/smp.c
> drivers/irqchip/irq-gic.c:gic_handle_irq() either calls 
> handle_domain_irq() or handle_IPI(). The enter/exit calls live in those 
> functions.

Ah, I see.
Would you think we need to put rcu_nmi_enter/exit() as similar to x86
on do_mem_abort() and do_sp_pc_abort() too?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[Masami Hiramatsu]

On Sat, 20 Jul 2019 16:32:32 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi James,
> 
> On Fri, 19 Jul 2019 09:42:05 +0100
> James Morse <james.morse@arm.com> wrote:
> 
> > Hi,
> > 
> > On 7/18/19 3:31 PM, Masami Hiramatsu wrote:
> > > On Thu, 18 Jul 2019 10:20:23 +0100
> > > Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > >> On Wed, Jul 17, 2019 at 11:22:15PM -0700, Paul E. McKenney wrote:
> > >>> On Thu, Jul 18, 2019 at 02:43:58PM +0900, Masami Hiramatsu wrote:
> > >>>> Remove rcu_read_lock()/rcu_read_unlock() from debug exception
> > >>>> handlers since the software breakpoint can be hit on idle task.
> > >>
> > >> Why precisely do we need to elide these? Are we seeing warnings today?
> > > 
> > > Yes, unfortunately, or fortunately. Naresh reported that warns when
> > > ftracetest ran. I confirmed that happens if I probe on default_idle_call too.
> > > 
> > > /sys/kernel/debug/tracing # echo p default_idle_call >> kprobe_events
> > > /sys/kernel/debug/tracing # echo 1 > events/kprobes/enable
> > > /sys/kernel/debug/tracing # [  135.122237]
> > > [  135.125035] =============================
> > > [  135.125310] WARNING: suspicious RCU usage
> > 
> > > [  135.132224] Call trace:
> > > [  135.132491]  dump_backtrace+0x0/0x140
> > > [  135.132806]  show_stack+0x24/0x30
> > > [  135.133133]  dump_stack+0xc4/0x10c
> > > [  135.133726]  lockdep_rcu_suspicious+0xf8/0x108
> > > [  135.134171]  call_break_hook+0x170/0x178
> > > [  135.134486]  brk_handler+0x28/0x68
> > > [  135.134792]  do_debug_exception+0x90/0x150
> > > [  135.135051]  el1_dbg+0x18/0x8c
> > > [  135.135260]  default_idle_call+0x0/0x44
> > > [  135.135516]  cpu_startup_entry+0x2c/0x30
> > > [  135.135815]  rest_init+0x1b0/0x280
> > > [  135.136044]  arch_call_rest_init+0x14/0x1c
> > > [  135.136305]  start_kernel+0x4d4/0x500
> > 
> > >>> The exception entry and exit use irq_enter() and irq_exit(), in this
> > >>> case, correct?  Otherwise RCU will be ignoring this CPU.
> > >>
> > >> This is missing today, which sounds like the underlying bug.
> > > 
> > > Agreed. I'm not so familier with how debug exception is handled on arm64,
> > > would it be a kind of NMI or IRQ?
> > 
> > Debug exceptions can interrupt both SError (think: machine check) and 
> > pseudo-NMI, which both in turn interrupt interrupt-masked code. So they 
> > are a kind of NMI. But, be careful not to call 'nmi_enter()' twice, see 
> > do_serror() for how we work around this...
> 
> OK. I think we can use rcu_nmi_enter/exit() as same as x86.

Adding this solves rcu_read_lock() warning issues too.
So I will just replace [PATCH 3/3] with that.

> > > Anyway, it seems that normal irqs are also not calling irq_enter/exit
> > > except for arch/arm64/kernel/smp.c
> > drivers/irqchip/irq-gic.c:gic_handle_irq() either calls 
> > handle_domain_irq() or handle_IPI(). The enter/exit calls live in those 
> > functions.
> 
> Ah, I see.
> Would you think we need to put rcu_nmi_enter/exit() as similar to x86
> on do_mem_abort() and do_sp_pc_abort() too?

Hmm, it seems that adding rcu_nmi_enter/exit to both function causes
a failure of init process. At this moment I don't do that.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[Mark Rutland]

On Thu, Jul 18, 2019 at 11:31:33PM +0900, Masami Hiramatsu wrote:
> On Thu, 18 Jul 2019 10:20:23 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Wed, Jul 17, 2019 at 11:22:15PM -0700, Paul E. McKenney wrote:
> > > On Thu, Jul 18, 2019 at 02:43:58PM +0900, Masami Hiramatsu wrote:
> > > > Remove rcu_read_lock()/rcu_read_unlock() from debug exception
> > > > handlers since the software breakpoint can be hit on idle task.
> > 
> > Why precisely do we need to elide these? Are we seeing warnings today?
> 
> Yes, unfortunately, or fortunately. Naresh reported that warns when
> ftracetest ran. I confirmed that happens if I probe on default_idle_call too.
> 
> /sys/kernel/debug/tracing # echo p default_idle_call >> kprobe_events 
> /sys/kernel/debug/tracing # echo 1 > events/kprobes/enable 
> /sys/kernel/debug/tracing # [  135.122237] 
> [  135.125035] =============================
> [  135.125310] WARNING: suspicious RCU usage
> [  135.125581] 5.2.0-08445-g9187c508bdc7 #20 Not tainted
> [  135.125904] -----------------------------
> [  135.126205] include/linux/rcupdate.h:594 rcu_read_lock() used illegally while idle!
> [  135.126839] 
> [  135.126839] other info that might help us debug this:
> [  135.126839] 
> [  135.127410] 
> [  135.127410] RCU used illegally from idle CPU!
> [  135.127410] rcu_scheduler_active = 2, debug_locks = 1
> [  135.128114] RCU used illegally from extended quiescent state!
> [  135.128555] 1 lock held by swapper/0/0:
> [  135.128944]  #0: (____ptrval____) (rcu_read_lock){....}, at: call_break_hook+0x0/0x178
> [  135.130499] 
> [  135.130499] stack backtrace:
> [  135.131192] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.2.0-08445-g9187c508bdc7 #20
> [  135.131841] Hardware name: linux,dummy-virt (DT)
> [  135.132224] Call trace:
> [  135.132491]  dump_backtrace+0x0/0x140
> [  135.132806]  show_stack+0x24/0x30
> [  135.133133]  dump_stack+0xc4/0x10c
> [  135.133726]  lockdep_rcu_suspicious+0xf8/0x108
> [  135.134171]  call_break_hook+0x170/0x178
> [  135.134486]  brk_handler+0x28/0x68
> [  135.134792]  do_debug_exception+0x90/0x150
> [  135.135051]  el1_dbg+0x18/0x8c
> [  135.135260]  default_idle_call+0x0/0x44
> [  135.135516]  cpu_startup_entry+0x2c/0x30
> [  135.135815]  rest_init+0x1b0/0x280
> [  135.136044]  arch_call_rest_init+0x14/0x1c
> [  135.136305]  start_kernel+0x4d4/0x500
> [  135.136597] 
> 
> > > The exception entry and exit use irq_enter() and irq_exit(), in this
> > > case, correct?  Otherwise RCU will be ignoring this CPU.
> > 
> > This is missing today, which sounds like the underlying bug.
> 
> Agreed. I'm not so familier with how debug exception is handled on arm64,
> would it be a kind of NMI or IRQ?

They're more like faults, in that they're synchronous exceptions.

Given that, I think using irq_enter() / irq_exit() would be surprising
here, but perhaps they're misnamed.

What do other architectures do here? Having a kprobe on the critical
path to idle doesn't sound specific to arm64, but perhaps it is (and we
should rule it out).

Thanks,
Mark.

Re: [PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[Masami Hiramatsu]

Hi Mark,

On Fri, 19 Jul 2019 10:59:59 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Jul 18, 2019 at 11:31:33PM +0900, Masami Hiramatsu wrote:
> > On Thu, 18 Jul 2019 10:20:23 +0100
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > > On Wed, Jul 17, 2019 at 11:22:15PM -0700, Paul E. McKenney wrote:
> > > > On Thu, Jul 18, 2019 at 02:43:58PM +0900, Masami Hiramatsu wrote:
> > > > > Remove rcu_read_lock()/rcu_read_unlock() from debug exception
> > > > > handlers since the software breakpoint can be hit on idle task.
> > > 
> > > Why precisely do we need to elide these? Are we seeing warnings today?
> > 
> > Yes, unfortunately, or fortunately. Naresh reported that warns when
> > ftracetest ran. I confirmed that happens if I probe on default_idle_call too.
> > 
> > /sys/kernel/debug/tracing # echo p default_idle_call >> kprobe_events 
> > /sys/kernel/debug/tracing # echo 1 > events/kprobes/enable 
> > /sys/kernel/debug/tracing # [  135.122237] 
> > [  135.125035] =============================
> > [  135.125310] WARNING: suspicious RCU usage
> > [  135.125581] 5.2.0-08445-g9187c508bdc7 #20 Not tainted
> > [  135.125904] -----------------------------
> > [  135.126205] include/linux/rcupdate.h:594 rcu_read_lock() used illegally while idle!
> > [  135.126839] 
> > [  135.126839] other info that might help us debug this:
> > [  135.126839] 
> > [  135.127410] 
> > [  135.127410] RCU used illegally from idle CPU!
> > [  135.127410] rcu_scheduler_active = 2, debug_locks = 1
> > [  135.128114] RCU used illegally from extended quiescent state!
> > [  135.128555] 1 lock held by swapper/0/0:
> > [  135.128944]  #0: (____ptrval____) (rcu_read_lock){....}, at: call_break_hook+0x0/0x178
> > [  135.130499] 
> > [  135.130499] stack backtrace:
> > [  135.131192] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.2.0-08445-g9187c508bdc7 #20
> > [  135.131841] Hardware name: linux,dummy-virt (DT)
> > [  135.132224] Call trace:
> > [  135.132491]  dump_backtrace+0x0/0x140
> > [  135.132806]  show_stack+0x24/0x30
> > [  135.133133]  dump_stack+0xc4/0x10c
> > [  135.133726]  lockdep_rcu_suspicious+0xf8/0x108
> > [  135.134171]  call_break_hook+0x170/0x178
> > [  135.134486]  brk_handler+0x28/0x68
> > [  135.134792]  do_debug_exception+0x90/0x150
> > [  135.135051]  el1_dbg+0x18/0x8c
> > [  135.135260]  default_idle_call+0x0/0x44
> > [  135.135516]  cpu_startup_entry+0x2c/0x30
> > [  135.135815]  rest_init+0x1b0/0x280
> > [  135.136044]  arch_call_rest_init+0x14/0x1c
> > [  135.136305]  start_kernel+0x4d4/0x500
> > [  135.136597] 
> > 
> > > > The exception entry and exit use irq_enter() and irq_exit(), in this
> > > > case, correct?  Otherwise RCU will be ignoring this CPU.
> > > 
> > > This is missing today, which sounds like the underlying bug.
> > 
> > Agreed. I'm not so familier with how debug exception is handled on arm64,
> > would it be a kind of NMI or IRQ?
> 
> They're more like faults, in that they're synchronous exceptions.
> 
> Given that, I think using irq_enter() / irq_exit() would be surprising
> here, but perhaps they're misnamed.
> 
> What do other architectures do here? Having a kprobe on the critical
> path to idle doesn't sound specific to arm64, but perhaps it is (and we
> should rule it out).

On x86, it uses rcu_nmi_enter/exit() for kernel mode. For user mode,
we don't need to care since it must not be an idle task.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 3/3] arm64: debug: Remove rcu_read_lock from debug exception

[Mark Rutland]

On Sat, Jul 20, 2019 at 04:54:58PM +0900, Masami Hiramatsu wrote:
> Hi Mark,
> 
> On Fri, 19 Jul 2019 10:59:59 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Thu, Jul 18, 2019 at 11:31:33PM +0900, Masami Hiramatsu wrote:
> > > On Thu, 18 Jul 2019 10:20:23 +0100
> > > Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > > > On Wed, Jul 17, 2019 at 11:22:15PM -0700, Paul E. McKenney wrote:
> > > > > On Thu, Jul 18, 2019 at 02:43:58PM +0900, Masami Hiramatsu wrote:
> > > > > > Remove rcu_read_lock()/rcu_read_unlock() from debug exception
> > > > > > handlers since the software breakpoint can be hit on idle task.
> > > > 
> > > > Why precisely do we need to elide these? Are we seeing warnings today?
> > > 
> > > Yes, unfortunately, or fortunately. Naresh reported that warns when
> > > ftracetest ran. I confirmed that happens if I probe on default_idle_call too.
> > > 
> > > /sys/kernel/debug/tracing # echo p default_idle_call >> kprobe_events 
> > > /sys/kernel/debug/tracing # echo 1 > events/kprobes/enable 
> > > /sys/kernel/debug/tracing # [  135.122237] 
> > > [  135.125035] =============================
> > > [  135.125310] WARNING: suspicious RCU usage
> > > [  135.125581] 5.2.0-08445-g9187c508bdc7 #20 Not tainted
> > > [  135.125904] -----------------------------
> > > [  135.126205] include/linux/rcupdate.h:594 rcu_read_lock() used illegally while idle!
> > > [  135.126839] 
> > > [  135.126839] other info that might help us debug this:
> > > [  135.126839] 
> > > [  135.127410] 
> > > [  135.127410] RCU used illegally from idle CPU!
> > > [  135.127410] rcu_scheduler_active = 2, debug_locks = 1
> > > [  135.128114] RCU used illegally from extended quiescent state!
> > > [  135.128555] 1 lock held by swapper/0/0:
> > > [  135.128944]  #0: (____ptrval____) (rcu_read_lock){....}, at: call_break_hook+0x0/0x178
> > > [  135.130499] 
> > > [  135.130499] stack backtrace:
> > > [  135.131192] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.2.0-08445-g9187c508bdc7 #20
> > > [  135.131841] Hardware name: linux,dummy-virt (DT)
> > > [  135.132224] Call trace:
> > > [  135.132491]  dump_backtrace+0x0/0x140
> > > [  135.132806]  show_stack+0x24/0x30
> > > [  135.133133]  dump_stack+0xc4/0x10c
> > > [  135.133726]  lockdep_rcu_suspicious+0xf8/0x108
> > > [  135.134171]  call_break_hook+0x170/0x178
> > > [  135.134486]  brk_handler+0x28/0x68
> > > [  135.134792]  do_debug_exception+0x90/0x150
> > > [  135.135051]  el1_dbg+0x18/0x8c
> > > [  135.135260]  default_idle_call+0x0/0x44
> > > [  135.135516]  cpu_startup_entry+0x2c/0x30
> > > [  135.135815]  rest_init+0x1b0/0x280
> > > [  135.136044]  arch_call_rest_init+0x14/0x1c
> > > [  135.136305]  start_kernel+0x4d4/0x500
> > > [  135.136597] 
> > > 
> > > > > The exception entry and exit use irq_enter() and irq_exit(), in this
> > > > > case, correct?  Otherwise RCU will be ignoring this CPU.
> > > > 
> > > > This is missing today, which sounds like the underlying bug.
> > > 
> > > Agreed. I'm not so familier with how debug exception is handled on arm64,
> > > would it be a kind of NMI or IRQ?
> > 
> > They're more like faults, in that they're synchronous exceptions.
> > 
> > Given that, I think using irq_enter() / irq_exit() would be surprising
> > here, but perhaps they're misnamed.
> > 
> > What do other architectures do here? Having a kprobe on the critical
> > path to idle doesn't sound specific to arm64, but perhaps it is (and we
> > should rule it out).
> 
> On x86, it uses rcu_nmi_enter/exit() for kernel mode. For user mode,
> we don't need to care since it must not be an idle task.

Ok. IIUC, doing the same for arm64 would make sense.

Thanks,
Mark.