diff mbox series

[RFC] arm64/mm: hit DEBUG_LOCKS_WARN_ON

Message ID 20181031121356.30654-1-anders.roxell@linaro.org
State New
Headers show
Series [RFC] arm64/mm: hit DEBUG_LOCKS_WARN_ON | expand

Commit Message

Anders Roxell Oct. 31, 2018, 12:13 p.m. UTC 
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(-)

-- 
2.19.1

Comments

James Morse Oct. 31, 2018, 12:37 p.m. UTC | #1 
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
Anders Roxell Oct. 31, 2018, 3:30 p.m. UTC | #2 
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
Anders Roxell Nov. 1, 2018, 11:04 a.m. UTC | #3 
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
diff mbox series

Patch

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