[PATCH v2 0/2] arm64: Fix the call trace when running kgdb test suite

[PATCH v2 0/2] arm64: Fix the call trace when running kgdb test suite

[Kevin Hao]

Hi guys,

This fixes a call trace when running the kgdb test suite:
    # echo kgdbts=V1 > /sys/module/kgdbts/parameters/kgdbts

v2:
  - Add a patch to fix a typo in el1_dbg()
  - Use the method as suggested by Mark.

v1:
  https://lore.kernel.org/r/20200401052107.36076-1-haokexin@gmail.com


Kevin Hao (2):
  arm64: entry: Fix the typo in the comment of el1_dbg()
  arm64: debug: Always update the IRQ tracing in debug_exception_enter()

 arch/arm64/kernel/entry-common.c | 2 +-
 arch/arm64/mm/fault.c            | 8 ++------
 2 files changed, 3 insertions(+), 7 deletions(-)

-- 
2.26.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] arm64: entry: Fix the typo in the comment of el1_dbg()

[Kevin Hao]

The function name should be local_daif_mask().

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 arch/arm64/kernel/entry-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index c839b5bf1904..420cd8e1534b 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -57,7 +57,7 @@ static void notrace el1_dbg(struct pt_regs *regs, unsigned long esr)
 	/*
 	 * The CPU masked interrupts, and we are leaving them masked during
 	 * do_debug_exception(). Update PMR as if we had called
-	 * local_mask_daif().
+	 * local_daif_mask().
 	 */
 	if (system_uses_irq_prio_masking())
 		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
-- 
2.26.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] arm64: debug: Always update the IRQ tracing in debug_exception_enter()

[Kevin Hao]

When running the kgdb test suite, we get the following call trace:
  # echo kgdbts=V1 > /sys/module/kgdbts/parameters/kgdbts

  DEBUG_LOCKS_WARN_ON(current->hardirqs_enabled)
  WARNING: CPU: 10 PID: 697 at kernel/locking/lockdep.c:4793 check_flags.part.0+0x15c/0x180
  Modules linked in:
  CPU: 10 PID: 697 Comm: sh Not tainted 5.6.0-next-20200331-yoctodev-standard+ #341
  Hardware name: Marvell OcteonTX CN96XX board (DT)
  pstate: 604003c9 (nZCv DAIF +PAN -UAO)
  pc : check_flags.part.0+0x15c/0x180
  lr : check_flags.part.0+0x15c/0x180
  sp : ffff800017d0f830
  x29: ffff800017d0f830 x28: ffff000b936c0000
  x27: 00000000c28f5c29 x26: 00000000ffffffff
  x25: 00000000000003c0 x24: ffff800011af8dd0
  x23: 0000000000000000 x22: ffff8000119afdc0
  x21: ffff80001198bbe8 x20: ffff800011400018
  x19: ffff800012655000 x18: 0000000000000001
  x17: 0000000000000000 x16: 0000000000000000
  x15: ffff000b936c0470 x14: ffffffffffffffff
  x13: 0000000000000000 x12: ffff800012655000
  x11: ffff800017d0f830 x10: ffff800017d0f830
  x9 : 00000000000003c0 x8 : 6e655f7371726964
  x7 : 7261683e2d746e65 x6 : ffff8000126552fe
  x5 : 0000000000000000 x4 : 0000000000000000
  x3 : 00000000ffffffff x2 : 0000000000000000
  x1 : bbf8ef1cf7dda200 x0 : 0000000000000000
  Call trace:
   check_flags.part.0+0x15c/0x180
   lock_is_held_type+0xf0/0x120
   rcu_read_lock_sched_held+0x74/0x98
   trace_rcu_dyntick+0x1b8/0x1e0
   rcu_nmi_enter+0x7c/0xb8
   debug_exception_enter+0x68/0xe8
   do_debug_exception+0x60/0x150
   el1_sync_handler+0xd8/0xf8
   el1_sync+0x7c/0x100
   el1_irq+0x78/0x180
   kgdbts_break_test+0x0/0x40
   param_set_kgdbts_var+0x68/0xe8
   param_attr_store+0xb8/0x120
   module_attr_store+0x2c/0x48
   sysfs_kf_write+0x54/0x80
   kernfs_fop_write+0x154/0x248
   __vfs_write+0x24/0x50
   vfs_write+0xec/0x1d8
   ksys_write+0x74/0x100
   __arm64_sys_write+0x24/0x30
   do_el0_svc+0x8c/0x1e8
   el0_sync_handler+0x11c/0x198
   el0_sync+0x158/0x180
  irq event stamp: 76505
  hardirqs last  enabled at (76505): [<ffff80001009f9ec>] debug_exception_exit+0x54/0x68
  hardirqs last disabled at (76504): [<ffff80001009fb04>] debug_exception_enter+0xac/0xe8
  softirqs last  enabled at (76498): [<ffff8000100817b4>] __do_softirq+0x5a4/0x5ec
  softirqs last disabled at (76439): [<ffff8000100b6ff4>] irq_exit+0x13c/0x150

The reason is that an IRQ is emitted when doing the single step debug,
but in the IRQ handler we would enable the debug exception before
updating the IRQ tracing flags. This will cause the debug exception
running in an context which the IRQ state and IRQ tracing flags are
mismatched. And the debug exception handler only update the IRQ tracing
flag to off when it thinks that the debug emit in a IRQ enabled context.
Then we would get the above call trace if any code like check_flags()
is called in the debug exception handler's path. We can't fix this issue
by just shuffling the enable_da_f in el1_irq. As indicated by
Mark Rutland, we also can run into the same issue when we set a
breakpoint in the middle of local_irq_disable(). So we should invoke the
trace_hardirqs_off() unconditionally in debug_exception_enter().
Also the reason why we conditionally invoke trace_hardirqs_off() seems
invalid since the redundant invoking of trace_hardirqs_off() would not
overwrite the last IRQ disabled address tracked by IRQ tracing.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 arch/arm64/mm/fault.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index c9cedc0432d2..b3c6a2e9232e 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -789,12 +789,8 @@ void __init hook_debug_fault_code(int nr,
  */
 static void debug_exception_enter(struct pt_regs *regs)
 {
-	/*
-	 * Tell lockdep we disabled irqs in entry.S. Do nothing if they were
-	 * already disabled to preserve the last enabled/disabled addresses.
-	 */
-	if (interrupts_enabled(regs))
-		trace_hardirqs_off();
+	/* Tell lockdep we disabled irqs in entry.S. */
+	trace_hardirqs_off();
 
 	if (user_mode(regs)) {
 		RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
-- 
2.26.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] arm64: entry: Fix the typo in the comment of el1_dbg()

[Mark Rutland]

On Fri, Apr 17, 2020 at 06:32:11PM +0800, Kevin Hao wrote:
> The function name should be local_daif_mask().
> 
> Signed-off-by: Kevin Hao <haokexin@gmail.com>

Acked-by: Mark Rutlamd <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/kernel/entry-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index c839b5bf1904..420cd8e1534b 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -57,7 +57,7 @@ static void notrace el1_dbg(struct pt_regs *regs, unsigned long esr)
>  	/*
>  	 * The CPU masked interrupts, and we are leaving them masked during
>  	 * do_debug_exception(). Update PMR as if we had called
> -	 * local_mask_daif().
> +	 * local_daif_mask().
>  	 */
>  	if (system_uses_irq_prio_masking())
>  		gic_write_pmr(GIC_PRIO_IRQON | GIC_PRIO_PSR_I_SET);
> -- 
> 2.26.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] arm64: Fix the call trace when running kgdb test suite

[Kevin Hao]

[-- Attachment #1.1: Type: text/plain, Size: 752 bytes --]

On Fri, Apr 17, 2020 at 06:32:10PM +0800, Kevin Hao wrote:
> Hi guys,
> 
> This fixes a call trace when running the kgdb test suite:
>     # echo kgdbts=V1 > /sys/module/kgdbts/parameters/kgdbts

Ping.

Thanks,
Kevin

> 
> v2:
>   - Add a patch to fix a typo in el1_dbg()
>   - Use the method as suggested by Mark.
> 
> v1:
>   https://lore.kernel.org/r/20200401052107.36076-1-haokexin@gmail.com
> 
> 
> Kevin Hao (2):
>   arm64: entry: Fix the typo in the comment of el1_dbg()
>   arm64: debug: Always update the IRQ tracing in debug_exception_enter()
> 
>  arch/arm64/kernel/entry-common.c | 2 +-
>  arch/arm64/mm/fault.c            | 8 ++------
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> -- 
> 2.26.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] arm64: Fix the call trace when running kgdb test suite

[Will Deacon]

On Fri, 17 Apr 2020 18:32:10 +0800, Kevin Hao wrote:
> This fixes a call trace when running the kgdb test suite:
>     # echo kgdbts=V1 > /sys/module/kgdbts/parameters/kgdbts
> 
> v2:
>   - Add a patch to fix a typo in el1_dbg()
>   - Use the method as suggested by Mark.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: entry: Fix the typo in the comment of el1_dbg()
      https://git.kernel.org/arm64/c/b8c1c9fe6a04

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] arm64: Fix the call trace when running kgdb test suite

[Kevin Hao]

[-- Attachment #1.1: Type: text/plain, Size: 866 bytes --]

On Wed, Jul 08, 2020 at 11:02:34PM +0100, Will Deacon wrote:
> On Fri, 17 Apr 2020 18:32:10 +0800, Kevin Hao wrote:
> > This fixes a call trace when running the kgdb test suite:
> >     # echo kgdbts=V1 > /sys/module/kgdbts/parameters/kgdbts
> > 
> > v2:
> >   - Add a patch to fix a typo in el1_dbg()
> >   - Use the method as suggested by Mark.
> > 
> > [...]
> 
> Applied to arm64 (for-next/fixes), thanks!
> 
> [1/1] arm64: entry: Fix the typo in the comment of el1_dbg()
>       https://git.kernel.org/arm64/c/b8c1c9fe6a04

How about patch 2 ("arm64: debug: Always update the IRQ tracing in debug_exception_enter())"?
It has been dangled in the mail list for almost 3 months without any explicitly objections.

Thanks,
Kevin

> 
> Cheers,
> -- 
> Will
> 
> https://fixes.arm64.dev
> https://next.arm64.dev
> https://will.arm64.dev

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/2] arm64: Fix the call trace when running kgdb test suite

[Will Deacon]

On Thu, Jul 09, 2020 at 08:55:04PM +0800, Kevin Hao wrote:
> On Wed, Jul 08, 2020 at 11:02:34PM +0100, Will Deacon wrote:
> > On Fri, 17 Apr 2020 18:32:10 +0800, Kevin Hao wrote:
> > > This fixes a call trace when running the kgdb test suite:
> > >     # echo kgdbts=V1 > /sys/module/kgdbts/parameters/kgdbts
> > > 
> > > v2:
> > >   - Add a patch to fix a typo in el1_dbg()
> > >   - Use the method as suggested by Mark.
> > > 
> > > [...]
> > 
> > Applied to arm64 (for-next/fixes), thanks!
> > 
> > [1/1] arm64: entry: Fix the typo in the comment of el1_dbg()
> >       https://git.kernel.org/arm64/c/b8c1c9fe6a04
> 
> How about patch 2 ("arm64: debug: Always update the IRQ tracing in debug_exception_enter())"?
> It has been dangled in the mail list for almost 3 months without any explicitly objections.

No, sorry, I really think it's the wrong direction of travel. Its adding
bodges to try to handle the fact that we disabled interrupts during the
step, and we really shouldn't be doing that (See my reply to Doug).

As I said, I would prefer some feedback on my proposal for dealing with
this properly [1] rather than more futile attempts to build on top of the
mess that we have at the moment.

Will

[1] https://lore.kernel.org/r/20200626095551.GA9312@willie-the-truck

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel