| Message ID | 156342863822.8565.7624877983728871995.stgit@devnote2 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: kprobes: Fix some bugs in arm64 kprobes | expand |
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; > } >
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; > > } > >
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,
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
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.
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,
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,
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,
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.
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; }
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(-)