* [RFC PATCH] arm64/mm: hit DEBUG_LOCKS_WARN_ON
@ 2018-10-31 12:13 Anders Roxell
2018-10-31 12:37 ` James Morse
0 siblings, 1 reply; 4+ messages in thread
From: Anders Roxell @ 2018-10-31 12:13 UTC (permalink / raw)
To: will.deacon, jason.wessel, daniel.thompson, james.morse
Cc: kgdb-bugreport, linux-kernel, arnd, Anders Roxell
I'm running an arm64 kernel in a qemu guest and I hit this
DEBUG_LOCKS_WARN_ON and this patch removes the warning. The
interrupts_enabled(reqs) should reflect the irq->enabled state of the
kgdb test process when it hit the breakpoint, and that should be the
same as the current->hardirqs_enabled flag
[ 25.682116] kgdbts:RUN singlestep test 1000 iterations
[ 25.695926] ------------[ cut here ]------------
[ 25.696410] DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)
[ 25.697407] WARNING: CPU: 0 PID: 1 at ../kernel/locking/lockdep.c:3761 check_flags+0x64/0x198
[ 25.698349] Modules linked in:
[ 25.699144] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G T 4.19.0-next-20181030+ #6
[ 25.699739] Hardware name: linux,dummy-virt (DT)
[ 25.700359] pstate: 404003c5 (nZcv DAIF +PAN -UAO)
[ 25.700901] pc : check_flags+0x64/0x198
[ 25.701381] lr : check_flags+0x64/0x198
[ 25.701779] sp : ffffff800814b800
[ 25.702160] x29: ffffff800814b800 x28: ffffffc03ffbc000
[ 25.702721] x27: ffffff80081ee4d0 x26: 0000000000000000
[ 25.703240] x25: 0000000000000000 x24: 0000000000000002
[ 25.703753] x23: 0000000000000000 x22: 0000000000000000
[ 25.704276] x21: ffffff800aa23000 x20: 00000000000003c0
[ 25.704792] x19: ffffff8009c14000 x18: 00000000ffffffff
[ 25.705315] x17: 0000000000000000 x16: 0000000000000000
[ 25.705833] x15: 1740000000000000 x14: 2f6f69747269762f
[ 25.706356] x13: ffffffffffff0000 x12: ffffffffffffffff
[ 25.706877] x11: 000000000000000e x10: 7f7f7f7f7f7f7f7f
[ 25.707431] x9 : ffffff800814b800 x8 : 7261683e2d746e65
[ 25.707954] x7 : 72727563284e4f5f x6 : ffffff800aa2283e
[ 25.708471] x5 : 0000000000000000 x4 : 0000000000000000
[ 25.708990] x3 : ffffff8008318584 x2 : ffffffc03ffbc000
[ 25.709512] x1 : 0344ea595a8c0b00 x0 : 0000000000000000
[ 25.710104] Call trace:
[ 25.710553] check_flags+0x64/0x198
[ 25.711023] lock_acquire+0x60/0x248
[ 25.711477] call_step_hook+0x5c/0x190
[ 25.711937] single_step_handler+0x48/0xa0
[ 25.712399] do_debug_exception+0x104/0x160
[ 25.712846] el1_dbg+0x18/0xa8
[ 25.713252] el1_irq+0xa8/0x1c0
[ 25.713709] kgdbts_break_test+0x0/0x40
[ 25.714171] kgdbts_run_tests+0x2b0/0x430
[ 25.714637] configure_kgdbts+0x7c/0xb0
[ 25.715110] init_kgdbts+0x70/0xac
[ 25.715551] do_one_initcall+0x1fc/0x490
[ 25.716010] do_initcall_level+0x458/0x49c
[ 25.716462] do_initcalls+0x60/0xa4
[ 25.716892] do_basic_setup+0x3c/0x68
[ 25.717343] kernel_init_freeable+0x170/0x2b8
[ 25.717860] kernel_init+0x20/0x130
[ 25.718302] ret_from_fork+0x10/0x18
[ 25.718712] irq event stamp: 266621
[ 25.719209] hardirqs last enabled at (266621): [<ffffff80081e935c>] do_debug_exception+0x13c/0x160
[ 25.719949] hardirqs last disabled at (266620): [<ffffff8008215a58>] kgdb_roundup_cpus+0x30/0x38
[ 25.720675] softirqs last enabled at (266616): [<ffffff80081e9f24>] __do_softirq+0x4e4/0x52c
[ 25.721374] softirqs last disabled at (266609): [<ffffff8008279694>] irq_exit+0xbc/0x128
[ 25.722050] ---[ end trace ecb2b85abf3c30a2 ]---
[ 25.722537] possible reason: unannotated irqs-off.
[ 25.722960] irq event stamp: 266621
Rework code to drop the 'if (interrupts_enabled(regs))' before
trace_hardirqs_*() in function do_debug_exception().
We don't know why current->hardirqs_enabled and interrupts_enabled(regs)
don't match.
Fixes: 6afedcd23cfd ("arm64: mm: Add trace_irqflags annotations to do_debug_exception()")
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
arch/arm64/mm/fault.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 7d9571f4ae3d..9afe5a7ba55b 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -819,11 +819,9 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
int rv;
/*
- * Tell lockdep we disabled irqs in entry.S. Do nothing if they were
- * already disabled to preserve the last enabled/disabled addresses.
+ * Tell lockdep we disabled irqs in entry.S.
*/
- if (interrupts_enabled(regs))
- trace_hardirqs_off();
+ trace_hardirqs_off();
if (user_mode(regs) && instruction_pointer(regs) > TASK_SIZE)
arm64_apply_bp_hardening();
@@ -836,8 +834,7 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
rv = 0;
}
- if (interrupts_enabled(regs))
- trace_hardirqs_on();
+ trace_hardirqs_on();
return rv;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] arm64/mm: hit DEBUG_LOCKS_WARN_ON
2018-10-31 12:13 [RFC PATCH] arm64/mm: hit DEBUG_LOCKS_WARN_ON Anders Roxell
@ 2018-10-31 12:37 ` James Morse
2018-10-31 15:30 ` Anders Roxell
0 siblings, 1 reply; 4+ messages in thread
From: James Morse @ 2018-10-31 12:37 UTC (permalink / raw)
To: Anders Roxell
Cc: will.deacon, jason.wessel, daniel.thompson, kgdb-bugreport,
linux-kernel, arnd, linux-arm-kernel
Hi Anders,
(CC: +linux-arm-kernel@lists.infradead.org)
On 31/10/2018 12:13, Anders Roxell wrote:
> I'm running an arm64 kernel in a qemu guest and I hit this
> DEBUG_LOCKS_WARN_ON and this patch removes the warning. The
> interrupts_enabled(reqs) should reflect the irq->enabled state of the
> kgdb test process
(I know nothing about kgdb, please forgive the stupid questions:)
Where does this 'irq->enabled state of the kgdb test process' come from?
What happens if it takes an interrupt, then you single-step the
interrupt handler? (How can any state in memory get updated...)
> when it hit the breakpoint, and that should be the
> same as the current->hardirqs_enabled flag
>
> [ 25.682116] kgdbts:RUN singlestep test 1000 iterations
> [ 25.695926] ------------[ cut here ]------------
> [ 25.696410] DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)
> [ 25.697407] WARNING: CPU: 0 PID: 1 at ../kernel/locking/lockdep.c:3761 check_flags+0x64/0x198
> [ 25.698349] Modules linked in:
> [ 25.699144] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G T 4.19.0-next-20181030+ #6
> [ 25.699739] Hardware name: linux,dummy-virt (DT)
> [ 25.700359] pstate: 404003c5 (nZcv DAIF +PAN -UAO)
> [ 25.700901] pc : check_flags+0x64/0x198
> [ 25.701381] lr : check_flags+0x64/0x198
> [ 25.710104] Call trace:
> [ 25.710553] check_flags+0x64/0x198
> [ 25.711023] lock_acquire+0x60/0x248
> [ 25.711477] call_step_hook+0x5c/0x190
> [ 25.711937] single_step_handler+0x48/0xa0
> [ 25.712399] do_debug_exception+0x104/0x160
> [ 25.712846] el1_dbg+0x18/0xa8
> [ 25.713252] el1_irq+0xa8/0x1c0
> [ 25.713709] kgdbts_break_test+0x0/0x40
> [ 25.714171] kgdbts_run_tests+0x2b0/0x430
> [ 25.714637] configure_kgdbts+0x7c/0xb0
> [ 25.715110] init_kgdbts+0x70/0xac
> [ 25.715551] do_one_initcall+0x1fc/0x490
> [ 25.716010] do_initcall_level+0x458/0x49c
> [ 25.716462] do_initcalls+0x60/0xa4
> [ 25.716892] do_basic_setup+0x3c/0x68
> [ 25.717343] kernel_init_freeable+0x170/0x2b8
> [ 25.717860] kernel_init+0x20/0x130
> [ 25.718302] ret_from_fork+0x10/0x18
> [ 25.718712] irq event stamp: 266621
> [ 25.719209] hardirqs last enabled at (266621): [<ffffff80081e935c>] do_debug_exception+0x13c/0x160
> [ 25.719949] hardirqs last disabled at (266620): [<ffffff8008215a58>] kgdb_roundup_cpus+0x30/0x38
> [ 25.720675] softirqs last enabled at (266616): [<ffffff80081e9f24>] __do_softirq+0x4e4/0x52c
> [ 25.721374] softirqs last disabled at (266609): [<ffffff8008279694>] irq_exit+0xbc/0x128
> [ 25.722050] ---[ end trace ecb2b85abf3c30a2 ]---
> [ 25.722537] possible reason: unannotated irqs-off.
> [ 25.722960] irq event stamp: 266621
>
> Rework code to drop the 'if (interrupts_enabled(regs))' before
> trace_hardirqs_*() in function do_debug_exception().
>
> We don't know why current->hardirqs_enabled and interrupts_enabled(regs)
> don't match.
You took an IRQ from kgdbts_break_test(). This causes the CPU to mask
interrupts, we tell trace_hardirqs_off() about this in the entry asm.
From arch/arm64/kernel/entry.S:
| el1_irq:
| kernel_entry 1
| enable_da_f
| #ifdef CONFIG_TRACE_IRQFLAGS
| bl trace_hardirqs_off
| #endif
|
| irq_handler
Could you faddr2line 'el1_irq+0xa8/0x1c0'? I bet that is the
'enable_da_f' line which re-enables debug, just _before_ we update call
trace_hardirqs_off.
The problem is debug exceptions can jump in this gap and see an
inconsistent trace_hardirqs_off/regs state.
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 7d9571f4ae3d..9afe5a7ba55b 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -819,11 +819,9 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
> int rv;
>
> /*
> - * Tell lockdep we disabled irqs in entry.S. Do nothing if they were
> - * already disabled to preserve the last enabled/disabled addresses.
> + * Tell lockdep we disabled irqs in entry.S.
> */
> - if (interrupts_enabled(regs))
> - trace_hardirqs_off();
> + trace_hardirqs_off();
Taking a debug-exception from irq-unmasked code will cause IRQs to be
disabled. This code updates trace_hardirqs_off().
But, taking a debug-exception from irq-masked code doesn't change
anything. Making this trace_hardirqs_off() unconditional means you now
overwrite the original location where irqs were masked.
Would making sure debug exceptions can't see current->hardirqs_enabled
and interrupts_enabled(regs) as having different states fix this problem?
If so moving the 'enable_da_f' line in el1_irq below the #ifdef block
should fix it.
Thanks,
James
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] arm64/mm: hit DEBUG_LOCKS_WARN_ON
2018-10-31 12:37 ` James Morse
@ 2018-10-31 15:30 ` Anders Roxell
2018-11-01 11:04 ` Anders Roxell
0 siblings, 1 reply; 4+ messages in thread
From: Anders Roxell @ 2018-10-31 15:30 UTC (permalink / raw)
To: james.morse
Cc: will.deacon, jason.wessel, daniel.thompson, kgdb-bugreport,
Linux Kernel Mailing List, Arnd Bergmann, Linux ARM
On Wed, 31 Oct 2018 at 13:37, James Morse <james.morse@arm.com> wrote:
>
> Hi Anders,
>
Hi James,
> (CC: +linux-arm-kernel@lists.infradead.org)
Thanks, I missed that.
>
> On 31/10/2018 12:13, Anders Roxell wrote:
> > I'm running an arm64 kernel in a qemu guest and I hit this
> > DEBUG_LOCKS_WARN_ON and this patch removes the warning. The
> > interrupts_enabled(reqs) should reflect the irq->enabled state of the
> > kgdb test process
>
> (I know nothing about kgdb, please forgive the stupid questions:)
>
> Where does this 'irq->enabled state of the kgdb test process' come from?
> What happens if it takes an interrupt, then you single-step the
> interrupt handler? (How can any state in memory get updated...)
>
>
> > when it hit the breakpoint, and that should be the
> > same as the current->hardirqs_enabled flag
> >
> > [ 25.682116] kgdbts:RUN singlestep test 1000 iterations
> > [ 25.695926] ------------[ cut here ]------------
> > [ 25.696410] DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)
> > [ 25.697407] WARNING: CPU: 0 PID: 1 at ../kernel/locking/lockdep.c:3761 check_flags+0x64/0x198
> > [ 25.698349] Modules linked in:
> > [ 25.699144] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G T 4.19.0-next-20181030+ #6
> > [ 25.699739] Hardware name: linux,dummy-virt (DT)
> > [ 25.700359] pstate: 404003c5 (nZcv DAIF +PAN -UAO)
> > [ 25.700901] pc : check_flags+0x64/0x198
> > [ 25.701381] lr : check_flags+0x64/0x198
>
> > [ 25.710104] Call trace:
> > [ 25.710553] check_flags+0x64/0x198
> > [ 25.711023] lock_acquire+0x60/0x248
> > [ 25.711477] call_step_hook+0x5c/0x190
> > [ 25.711937] single_step_handler+0x48/0xa0
> > [ 25.712399] do_debug_exception+0x104/0x160
> > [ 25.712846] el1_dbg+0x18/0xa8
> > [ 25.713252] el1_irq+0xa8/0x1c0
> > [ 25.713709] kgdbts_break_test+0x0/0x40
> > [ 25.714171] kgdbts_run_tests+0x2b0/0x430
> > [ 25.714637] configure_kgdbts+0x7c/0xb0
> > [ 25.715110] init_kgdbts+0x70/0xac
> > [ 25.715551] do_one_initcall+0x1fc/0x490
> > [ 25.716010] do_initcall_level+0x458/0x49c
> > [ 25.716462] do_initcalls+0x60/0xa4
> > [ 25.716892] do_basic_setup+0x3c/0x68
> > [ 25.717343] kernel_init_freeable+0x170/0x2b8
> > [ 25.717860] kernel_init+0x20/0x130
> > [ 25.718302] ret_from_fork+0x10/0x18
> > [ 25.718712] irq event stamp: 266621
> > [ 25.719209] hardirqs last enabled at (266621): [<ffffff80081e935c>] do_debug_exception+0x13c/0x160
> > [ 25.719949] hardirqs last disabled at (266620): [<ffffff8008215a58>] kgdb_roundup_cpus+0x30/0x38
> > [ 25.720675] softirqs last enabled at (266616): [<ffffff80081e9f24>] __do_softirq+0x4e4/0x52c
> > [ 25.721374] softirqs last disabled at (266609): [<ffffff8008279694>] irq_exit+0xbc/0x128
> > [ 25.722050] ---[ end trace ecb2b85abf3c30a2 ]---
> > [ 25.722537] possible reason: unannotated irqs-off.
> > [ 25.722960] irq event stamp: 266621
> >
> > Rework code to drop the 'if (interrupts_enabled(regs))' before
> > trace_hardirqs_*() in function do_debug_exception().
> >
> > We don't know why current->hardirqs_enabled and interrupts_enabled(regs)
> > don't match.
>
> You took an IRQ from kgdbts_break_test(). This causes the CPU to mask
> interrupts, we tell trace_hardirqs_off() about this in the entry asm.
>
> From arch/arm64/kernel/entry.S:
> | el1_irq:
> | kernel_entry 1
> | enable_da_f
> | #ifdef CONFIG_TRACE_IRQFLAGS
> | bl trace_hardirqs_off
> | #endif
> |
> | irq_handler
>
>
> Could you faddr2line 'el1_irq+0xa8/0x1c0'? I bet that is the
> 'enable_da_f' line which re-enables debug, just _before_ we update call
> trace_hardirqs_off.
I used objdump and found this:
ffffff800819f600 <el1_irq>:
ffffff800819f600: a90007e0 stp x0, x1, [sp]
...
ffffff800819f6a0: a9105ff6 stp x22, x23, [sp, #256]
ffffff800819f6a4: d5034dff msr daifclr, #0xd
ffffff800819f6a8: 94082970 bl ffffff80083a9c68
<trace_hardirqs_off>
ffffff800819f6ac: d0008641 adrp x1, ffffff8009269000
<cpu_ops+0x108>
ffffff800819f6b0: f940e021 ldr x1, [x1, #448]
>
> The problem is debug exceptions can jump in this gap and see an
> inconsistent trace_hardirqs_off/regs state.
>
>
> > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > index 7d9571f4ae3d..9afe5a7ba55b 100644
> > --- a/arch/arm64/mm/fault.c
> > +++ b/arch/arm64/mm/fault.c
> > @@ -819,11 +819,9 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
> > int rv;
> >
> > /*
> > - * Tell lockdep we disabled irqs in entry.S. Do nothing if they were
> > - * already disabled to preserve the last enabled/disabled addresses.
> > + * Tell lockdep we disabled irqs in entry.S.
> > */
> > - if (interrupts_enabled(regs))
> > - trace_hardirqs_off();
> > + trace_hardirqs_off();
>
> Taking a debug-exception from irq-unmasked code will cause IRQs to be
> disabled. This code updates trace_hardirqs_off().
>
> But, taking a debug-exception from irq-masked code doesn't change
> anything. Making this trace_hardirqs_off() unconditional means you now
> overwrite the original location where irqs were masked.
>
>
> Would making sure debug exceptions can't see current->hardirqs_enabled
> and interrupts_enabled(regs) as having different states fix this problem?
>
> If so moving the 'enable_da_f' line in el1_irq below the #ifdef block
> should fix it.
reverting this RFC patch and did this patch that you suggested (see below) and
I couldn't see the DEBUG_LOCKS_WARN_ON.
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 039144ecbcb2..c509213b2afc 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -614,10 +614,10 @@ ENDPROC(el1_sync)
.align 6
el1_irq:
kernel_entry 1
- enable_da_f
#ifdef CONFIG_TRACE_IRQFLAGS
bl trace_hardirqs_off
#endif
+ enable_da_f
irq_handler
Cheers,
Anders
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] arm64/mm: hit DEBUG_LOCKS_WARN_ON
2018-10-31 15:30 ` Anders Roxell
@ 2018-11-01 11:04 ` Anders Roxell
0 siblings, 0 replies; 4+ messages in thread
From: Anders Roxell @ 2018-11-01 11:04 UTC (permalink / raw)
To: james.morse
Cc: will.deacon, jason.wessel, Daniel Thompson, kgdb-bugreport,
Linux Kernel Mailing List, Arnd Bergmann, Linux ARM
On Wed, 31 Oct 2018 at 16:30, Anders Roxell <anders.roxell@linaro.org> wrote:
>
> On Wed, 31 Oct 2018 at 13:37, James Morse <james.morse@arm.com> wrote:
> >
> > Hi Anders,
> >
>
> Hi James,
>
> > (CC: +linux-arm-kernel@lists.infradead.org)
>
> Thanks, I missed that.
>
> >
> > On 31/10/2018 12:13, Anders Roxell wrote:
> > > I'm running an arm64 kernel in a qemu guest and I hit this
> > > DEBUG_LOCKS_WARN_ON and this patch removes the warning. The
> > > interrupts_enabled(reqs) should reflect the irq->enabled state of the
> > > kgdb test process
> >
> > (I know nothing about kgdb, please forgive the stupid questions:)
> >
> > Where does this 'irq->enabled state of the kgdb test process' come from?
> > What happens if it takes an interrupt, then you single-step the
> > interrupt handler? (How can any state in memory get updated...)
> >
> >
> > > when it hit the breakpoint, and that should be the
> > > same as the current->hardirqs_enabled flag
> > >
> > > [ 25.682116] kgdbts:RUN singlestep test 1000 iterations
> > > [ 25.695926] ------------[ cut here ]------------
> > > [ 25.696410] DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)
> > > [ 25.697407] WARNING: CPU: 0 PID: 1 at ../kernel/locking/lockdep.c:3761 check_flags+0x64/0x198
> > > [ 25.698349] Modules linked in:
> > > [ 25.699144] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G T 4.19.0-next-20181030+ #6
> > > [ 25.699739] Hardware name: linux,dummy-virt (DT)
> > > [ 25.700359] pstate: 404003c5 (nZcv DAIF +PAN -UAO)
> > > [ 25.700901] pc : check_flags+0x64/0x198
> > > [ 25.701381] lr : check_flags+0x64/0x198
> >
> > > [ 25.710104] Call trace:
> > > [ 25.710553] check_flags+0x64/0x198
> > > [ 25.711023] lock_acquire+0x60/0x248
> > > [ 25.711477] call_step_hook+0x5c/0x190
> > > [ 25.711937] single_step_handler+0x48/0xa0
> > > [ 25.712399] do_debug_exception+0x104/0x160
> > > [ 25.712846] el1_dbg+0x18/0xa8
> > > [ 25.713252] el1_irq+0xa8/0x1c0
> > > [ 25.713709] kgdbts_break_test+0x0/0x40
> > > [ 25.714171] kgdbts_run_tests+0x2b0/0x430
> > > [ 25.714637] configure_kgdbts+0x7c/0xb0
> > > [ 25.715110] init_kgdbts+0x70/0xac
> > > [ 25.715551] do_one_initcall+0x1fc/0x490
> > > [ 25.716010] do_initcall_level+0x458/0x49c
> > > [ 25.716462] do_initcalls+0x60/0xa4
> > > [ 25.716892] do_basic_setup+0x3c/0x68
> > > [ 25.717343] kernel_init_freeable+0x170/0x2b8
> > > [ 25.717860] kernel_init+0x20/0x130
> > > [ 25.718302] ret_from_fork+0x10/0x18
> > > [ 25.718712] irq event stamp: 266621
> > > [ 25.719209] hardirqs last enabled at (266621): [<ffffff80081e935c>] do_debug_exception+0x13c/0x160
> > > [ 25.719949] hardirqs last disabled at (266620): [<ffffff8008215a58>] kgdb_roundup_cpus+0x30/0x38
> > > [ 25.720675] softirqs last enabled at (266616): [<ffffff80081e9f24>] __do_softirq+0x4e4/0x52c
> > > [ 25.721374] softirqs last disabled at (266609): [<ffffff8008279694>] irq_exit+0xbc/0x128
> > > [ 25.722050] ---[ end trace ecb2b85abf3c30a2 ]---
> > > [ 25.722537] possible reason: unannotated irqs-off.
> > > [ 25.722960] irq event stamp: 266621
> > >
> > > Rework code to drop the 'if (interrupts_enabled(regs))' before
> > > trace_hardirqs_*() in function do_debug_exception().
> > >
> > > We don't know why current->hardirqs_enabled and interrupts_enabled(regs)
> > > don't match.
> >
> > You took an IRQ from kgdbts_break_test(). This causes the CPU to mask
> > interrupts, we tell trace_hardirqs_off() about this in the entry asm.
> >
> > From arch/arm64/kernel/entry.S:
> > | el1_irq:
> > | kernel_entry 1
> > | enable_da_f
> > | #ifdef CONFIG_TRACE_IRQFLAGS
> > | bl trace_hardirqs_off
> > | #endif
> > |
> > | irq_handler
> >
> >
> > Could you faddr2line 'el1_irq+0xa8/0x1c0'? I bet that is the
> > 'enable_da_f' line which re-enables debug, just _before_ we update call
> > trace_hardirqs_off.
>
> I used objdump and found this:
>
> ffffff800819f600 <el1_irq>:
> ffffff800819f600: a90007e0 stp x0, x1, [sp]
> ...
> ffffff800819f6a0: a9105ff6 stp x22, x23, [sp, #256]
> ffffff800819f6a4: d5034dff msr daifclr, #0xd
> ffffff800819f6a8: 94082970 bl ffffff80083a9c68
> <trace_hardirqs_off>
> ffffff800819f6ac: d0008641 adrp x1, ffffff8009269000
> <cpu_ops+0x108>
> ffffff800819f6b0: f940e021 ldr x1, [x1, #448]
>
>
>
> >
> > The problem is debug exceptions can jump in this gap and see an
> > inconsistent trace_hardirqs_off/regs state.
> >
> >
> > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > > index 7d9571f4ae3d..9afe5a7ba55b 100644
> > > --- a/arch/arm64/mm/fault.c
> > > +++ b/arch/arm64/mm/fault.c
> > > @@ -819,11 +819,9 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
> > > int rv;
> > >
> > > /*
> > > - * Tell lockdep we disabled irqs in entry.S. Do nothing if they were
> > > - * already disabled to preserve the last enabled/disabled addresses.
> > > + * Tell lockdep we disabled irqs in entry.S.
> > > */
> > > - if (interrupts_enabled(regs))
> > > - trace_hardirqs_off();
> > > + trace_hardirqs_off();
> >
> > Taking a debug-exception from irq-unmasked code will cause IRQs to be
> > disabled. This code updates trace_hardirqs_off().
> >
> > But, taking a debug-exception from irq-masked code doesn't change
> > anything. Making this trace_hardirqs_off() unconditional means you now
> > overwrite the original location where irqs were masked.
> >
> >
> > Would making sure debug exceptions can't see current->hardirqs_enabled
> > and interrupts_enabled(regs) as having different states fix this problem?
> >
> > If so moving the 'enable_da_f' line in el1_irq below the #ifdef block
> > should fix it.
>
> reverting this RFC patch and did this patch that you suggested (see below) and
> I couldn't see the DEBUG_LOCKS_WARN_ON.
I'm sorry for my ignorance but, but moving enable_da_f below the
#ifdef block solved
the problem. But I'm not sure that I understand why it solved it. Can
you please elaborate
a bit more ?
Should I move the 'enable_da_f' line in el0_ia below the #ifdef block as well?
Cheers,
Anders
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 039144ecbcb2..c509213b2afc 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -614,10 +614,10 @@ ENDPROC(el1_sync)
> .align 6
> el1_irq:
> kernel_entry 1
> - enable_da_f
> #ifdef CONFIG_TRACE_IRQFLAGS
> bl trace_hardirqs_off
> #endif
> + enable_da_f
>
> irq_handler
>
>
> Cheers,
> Anders
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-11-01 11:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-31 12:13 [RFC PATCH] arm64/mm: hit DEBUG_LOCKS_WARN_ON Anders Roxell
2018-10-31 12:37 ` James Morse
2018-10-31 15:30 ` Anders Roxell
2018-11-01 11:04 ` Anders Roxell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).