| Message ID | 1458580446-32474-1-git-send-email-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 21, 2016 at 05:14:06PM +0000, James Morse wrote: > With CONFIG_PROVE_LOCKING, CONFIG_DEBUG_LOCKDEP and CONFIG_TRACE_IRQFLAGS > enabled, lockdep will compare current->hardirqs_enabled with the flags from > local_irq_save(). > > When a debug exception occurs, interrupts are disabled in entry.S, but > lockdep isn't told, resulting in: > DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled) > ------------[ cut here ]------------ > WARNING: at ../kernel/locking/lockdep.c:3523 > Modules linked in: > CPU: 3 PID: 1752 Comm: perf Not tainted 4.5.0-rc4+ #2204 > Hardware name: ARM Juno development board (r1) (DT) > task: ffffffc974868000 ti: ffffffc975f40000 task.ti: ffffffc975f40000 > PC is at check_flags.part.35+0x17c/0x184 > LR is at check_flags.part.35+0x17c/0x184 > pc : [<ffffff80080fc93c>] lr : [<ffffff80080fc93c>] pstate: 600003c5 > [...] > ---[ end trace 74631f9305ef5020 ]--- > Call trace: > [<ffffff80080fc93c>] check_flags.part.35+0x17c/0x184 > [<ffffff80080ffe30>] lock_acquire+0xa8/0xc4 > [<ffffff8008093038>] breakpoint_handler+0x118/0x288 > [<ffffff8008082434>] do_debug_exception+0x3c/0xa8 > [<ffffff80080854b4>] el1_dbg+0x18/0x6c > [<ffffff80081e82f4>] do_filp_open+0x64/0xdc > [<ffffff80081d6e60>] do_sys_open+0x140/0x204 > [<ffffff80081d6f58>] SyS_openat+0x10/0x18 > [<ffffff8008085d30>] el0_svc_naked+0x24/0x28 > possible reason: unannotated irqs-off. > irq event stamp: 65857 > hardirqs last enabled at (65857): [<ffffff80081fb1c0>] lookup_mnt+0xf4/0x1b4 > hardirqs last disabled at (65856): [<ffffff80081fb188>] lookup_mnt+0xbc/0x1b4 > softirqs last enabled at (65790): [<ffffff80080bdca4>] __do_softirq+0x1f8/0x290 > softirqs last disabled at (65757): [<ffffff80080be038>] irq_exit+0x9c/0xd0 > > This patch adds the annotations to do_debug_exception(), while trying not > to call trace_hardirqs_off() if el1_dbg() interrupted a task that already > had irqs disabled. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > arch/arm64/mm/fault.c | 33 +++++++++++++++++++++++---------- > 1 file changed, 23 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 44e56de23f79..d8b9c1da2630 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -548,20 +548,33 @@ asmlinkage int __exception do_debug_exception(unsigned long addr, > { > const struct fault_info *inf = debug_fault_info + DBG_ESR_EVT(esr); > struct siginfo info; > + int rv; > > - if (!inf->fn(addr, esr, regs)) > - return 1; > + /* > + * If we came in from el0_dbg, we disabled irqs. From el1_dbg, > + * we need to test pstate. > + */ > + if (user_mode(regs) || !(regs->pstate & PSR_I_BIT)) > + trace_hardirqs_off(); Can you use interrupts_enabled(regs) for this? Will
On 29/03/16 16:06, Will Deacon wrote: > On Mon, Mar 21, 2016 at 05:14:06PM +0000, James Morse wrote: >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c >> index 44e56de23f79..d8b9c1da2630 100644 >> --- a/arch/arm64/mm/fault.c >> +++ b/arch/arm64/mm/fault.c >> @@ -548,20 +548,33 @@ asmlinkage int __exception do_debug_exception(unsigned long addr, >> { >> const struct fault_info *inf = debug_fault_info + DBG_ESR_EVT(esr); >> struct siginfo info; >> + int rv; >> >> - if (!inf->fn(addr, esr, regs)) >> - return 1; >> + /* >> + * If we came in from el0_dbg, we disabled irqs. From el1_dbg, >> + * we need to test pstate. >> + */ >> + if (user_mode(regs) || !(regs->pstate & PSR_I_BIT)) >> + trace_hardirqs_off(); > > Can you use interrupts_enabled(regs) for this? Yes! That would be better. I can also remove the user_mode() check which was due to paranoia and miss-understanding. Thanks, James
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 44e56de23f79..d8b9c1da2630 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -548,20 +548,33 @@ asmlinkage int __exception do_debug_exception(unsigned long addr, { const struct fault_info *inf = debug_fault_info + DBG_ESR_EVT(esr); struct siginfo info; + int rv; - if (!inf->fn(addr, esr, regs)) - return 1; + /* + * If we came in from el0_dbg, we disabled irqs. From el1_dbg, + * we need to test pstate. + */ + if (user_mode(regs) || !(regs->pstate & PSR_I_BIT)) + trace_hardirqs_off(); - pr_alert("Unhandled debug exception: %s (0x%08x) at 0x%016lx\n", - inf->name, esr, addr); + if (!inf->fn(addr, esr, regs)) { + rv = 1; + } else { + pr_alert("Unhandled debug exception: %s (0x%08x) at 0x%016lx\n", + inf->name, esr, addr); + + info.si_signo = inf->sig; + info.si_errno = 0; + info.si_code = inf->code; + info.si_addr = (void __user *)addr; + arm64_notify_die("", regs, &info, 0); + rv = 0; + } - info.si_signo = inf->sig; - info.si_errno = 0; - info.si_code = inf->code; - info.si_addr = (void __user *)addr; - arm64_notify_die("", regs, &info, 0); + if (user_mode(regs) || !(regs->pstate & PSR_I_BIT)) + trace_hardirqs_on(); - return 0; + return rv; } #ifdef CONFIG_ARM64_PAN
With CONFIG_PROVE_LOCKING, CONFIG_DEBUG_LOCKDEP and CONFIG_TRACE_IRQFLAGS enabled, lockdep will compare current->hardirqs_enabled with the flags from local_irq_save(). When a debug exception occurs, interrupts are disabled in entry.S, but lockdep isn't told, resulting in: DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled) ------------[ cut here ]------------ WARNING: at ../kernel/locking/lockdep.c:3523 Modules linked in: CPU: 3 PID: 1752 Comm: perf Not tainted 4.5.0-rc4+ #2204 Hardware name: ARM Juno development board (r1) (DT) task: ffffffc974868000 ti: ffffffc975f40000 task.ti: ffffffc975f40000 PC is at check_flags.part.35+0x17c/0x184 LR is at check_flags.part.35+0x17c/0x184 pc : [<ffffff80080fc93c>] lr : [<ffffff80080fc93c>] pstate: 600003c5 [...] ---[ end trace 74631f9305ef5020 ]--- Call trace: [<ffffff80080fc93c>] check_flags.part.35+0x17c/0x184 [<ffffff80080ffe30>] lock_acquire+0xa8/0xc4 [<ffffff8008093038>] breakpoint_handler+0x118/0x288 [<ffffff8008082434>] do_debug_exception+0x3c/0xa8 [<ffffff80080854b4>] el1_dbg+0x18/0x6c [<ffffff80081e82f4>] do_filp_open+0x64/0xdc [<ffffff80081d6e60>] do_sys_open+0x140/0x204 [<ffffff80081d6f58>] SyS_openat+0x10/0x18 [<ffffff8008085d30>] el0_svc_naked+0x24/0x28 possible reason: unannotated irqs-off. irq event stamp: 65857 hardirqs last enabled at (65857): [<ffffff80081fb1c0>] lookup_mnt+0xf4/0x1b4 hardirqs last disabled at (65856): [<ffffff80081fb188>] lookup_mnt+0xbc/0x1b4 softirqs last enabled at (65790): [<ffffff80080bdca4>] __do_softirq+0x1f8/0x290 softirqs last disabled at (65757): [<ffffff80080be038>] irq_exit+0x9c/0xd0 This patch adds the annotations to do_debug_exception(), while trying not to call trace_hardirqs_off() if el1_dbg() interrupted a task that already had irqs disabled. Signed-off-by: James Morse <james.morse@arm.com> --- arch/arm64/mm/fault.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)