[3/3] arm64: debug: Remove rcu_read_lock from debug exception
diff mbox series

Message ID 156342863822.8565.7624877983728871995.stgit@devnote2
State New
Headers show
Series
  • arm64: kprobes: Fix some bugs in arm64 kprobes
Related show

Commit Message

Masami Hiramatsu July 18, 2019, 5:43 a.m. UTC
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(-)

Comments

Paul E. McKenney July 18, 2019, 6:22 a.m. UTC | #1
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;
>  }
>
Mark Rutland July 18, 2019, 9:20 a.m. UTC | #2
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;
> >  }
> >
Masami Hiramatsu July 18, 2019, 2:31 p.m. UTC | #3
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,
James Morse July 19, 2019, 8:42 a.m. UTC | #4
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
Mark Rutland July 19, 2019, 9:59 a.m. UTC | #5
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.
Masami Hiramatsu July 20, 2019, 7:32 a.m. UTC | #6
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 July 20, 2019, 7:54 a.m. UTC | #7
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 July 21, 2019, 1:50 a.m. UTC | #8
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,
Mark Rutland July 24, 2019, 10:45 a.m. UTC | #9
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.

Patch
diff mbox series

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;
 }