arm64: mm: Add trace_irqflags annotations to do_debug_exception()

PATCH New James Morse 2 participants 3 messages Mar 21, 2016 – Apr 13, 2016 arm64

Share & tools

James Morse

author

2016-03-21 17:14 UTC #0 lore raw

PATCH 1/1 arm64: mm: Add trace_irqflags annotations to do_debug_exception()

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 <[email protected]>

---
 arch/arm64/mm/fault.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

arch/arm64/mm/fault.c +23 ~~-10~~

index 44e56de23f79..d8b9c1da2630 100644
@@ -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

Signature · 1 line

2.7.0

Will Deacon

2016-03-29 15:06 UTC #1 lore raw

PATCH 1/1 arm64: mm: Add trace_irqflags annotations to do_debug_exception()

Quoted 52 lines · James Morse

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 <[email protected]>

---
 arch/arm64/mm/fault.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

arch/arm64/mm/fault.c +0 -0

index 44e56de23f79..d8b9c1da2630 100644
@@ -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;

arch/arm64/mm/fault.c asmlinkage int __exception do_debug_exception(unsigned long addr, · line 558

Quoted 10 lines

arch/arm64/mm/fault.c +7 -2

+	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

James Morse

author

2016-04-13 12:37 UTC #2 lore raw

PATCH 1/1 arm64: mm: Add trace_irqflags annotations to do_debug_exception()

Quoted 11 lines · Will Deacon

Quoted 10 lines · James Morse

arch/arm64/mm/fault.c +1 -0

index 44e56de23f79..d8b9c1da2630 100644
@@ -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;

Quoted 10 lines

arch/arm64/mm/fault.c asmlinkage int __exception do_debug_exception(unsigned long addr, · line 558
Quoted 8 lines
arch/arm64/mm/fault.c +6 -2
-	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