[PATCH 0/2] arm64: kgdb/kdb: Fix pending single-step debugging issues

[PATCH 0/2] arm64: kgdb/kdb: Fix pending single-step debugging issues

[Sumit Garg]

This patch-set reworks pending fixes from Wei's series [1] to make
single-step debugging via kgdb/kdb on arm64 work as expected. There was
a prior discussion on ML [2] regarding if we should keep the interrupts
enabled during single-stepping but it turns out that in case of kgdb, it
is risky to enable interrupts as sometimes a resume after single
stepping an interrupt handler leads to following unbalanced locking
issue:

[  300.328300] WARNING: bad unlock balance detected!
[  300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted
[  300.329058] -------------------------------------
[  300.329298] sh/173 is trying to release lock (dbg_slave_lock) at:
[  300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820
[  300.330029] but there are no more locks to release!
[  300.330265] 
[  300.330265] other info that might help us debug this:
[  300.330668] 4 locks held by sh/173:
[  300.330891]  #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, at: vfs_write+0x98/0x204
[  300.331735]  #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, at: kgdb_cpu_enter+0x5b4/0x820
[  300.332259]  #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, at: kgdb_cpu_enter+0xe0/0x820
[  300.332717]  #3: ffffd57c973bc2a8 (dbg_master_lock){....}-{2:2}, at: kgdb_cpu_enter+0x1ec/0x820

So, I choose to keep interrupts disabled specifically for kgdb. This
series has been rebased to Linux 5.18-rc1 and I have dropped Doug's
review and test tags as there is significant rework involved.

[1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/
[2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/

Sumit Garg (2):
  arm64: kgdb: Fix incorrect single stepping into the irq handler
  arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step

 arch/arm64/include/asm/debug-monitors.h |  1 +
 arch/arm64/kernel/debug-monitors.c      |  5 ++++
 arch/arm64/kernel/kgdb.c                | 35 +++++++++++++++++++++++--
 3 files changed, 39 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/2] arm64: kgdb: Fix incorrect single stepping into the irq handler

[Sumit Garg]

PSTATE.I and PSTATE.D are very important for single step working.

Without disabling interrupt on local CPU, there is a chance of
interrupt occurrence in the period of exception return and start of
kgdb/kdb single-step, that result in wrongly single stepping into the
interrupt handler. And if D bit is set then, it results into undefined
exception and when it's handler enables dbg then single step exception
is generated, not as expected.

Currently when we execute single step in kdb/kgdb, we may see it jumps
to the irq_handler (where PSTATE.D is cleared) instead of the next
instruction. And a resume after single stepping into interrupt handler
sometimes leads to unbalanced locking:

[  300.328300] WARNING: bad unlock balance detected!
[  300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted
[  300.329058] -------------------------------------
[  300.329298] sh/173 is trying to release lock (dbg_slave_lock) at:
[  300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820
[  300.330029] but there are no more locks to release!
[  300.330265]
[  300.330265] other info that might help us debug this:
[  300.330668] 4 locks held by sh/173:
[  300.330891]  #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, at: vfs_write+0x98/0x204
[  300.331735]  #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, at: kgdb_cpu_enter+0x5b4/0x820
[  300.332259]  #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, at: kgdb_cpu_enter+0xe0/0x820
[  300.332717]  #3: ffffd57c973bc2a8 (dbg_master_lock){....}-{2:2}, at: kgdb_cpu_enter+0x1ec/0x820

Add the save and restore work for single-step while enabling and
disabling single stepping to maintain the PSTATE.I and PSTATE.D carefully.

Fixes: 44679a4f142b ("arm64: KGDB: Add step debugging support")
Co-developed-by: Wei Li <liwei391@huawei.com>
Signed-off-by: Wei Li <liwei391@huawei.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 arch/arm64/kernel/kgdb.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 2aede780fb80..653ad0d19f2f 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -15,6 +15,7 @@
 #include <linux/kprobes.h>
 #include <linux/sched/task_stack.h>
 
+#include <asm/daifflags.h>
 #include <asm/debug-monitors.h>
 #include <asm/insn.h>
 #include <asm/patching.h>
@@ -171,6 +172,30 @@ static void kgdb_arch_update_addr(struct pt_regs *regs,
 	compiled_break = 0;
 }
 
+/*
+ * Interrupts need to be disabled before single-step mode is set, and not
+ * re-enabled until after single-step mode ends. Without disabling interrupt
+ * on local CPU, there is a chance of interrupt occurrence in the period of
+ * exception return and start of kgdb/kdb single-step, that result in wrongly
+ * single stepping into the interrupt handler. Also, resume from single
+ * stepping the interrupt handler is risky as it sometimes leads to unbalanced
+ * locking.
+ */
+static DEFINE_PER_CPU(unsigned long, kgdb_ss_flags);
+
+static void kgdb_save_local_irqflag(struct pt_regs *regs)
+{
+	__this_cpu_write(kgdb_ss_flags, (regs->pstate & DAIF_MASK));
+	regs->pstate |= PSR_I_BIT;
+	regs->pstate &= ~PSR_D_BIT;
+}
+
+static void kgdb_restore_local_irqflag(struct pt_regs *regs)
+{
+	regs->pstate &= ~DAIF_MASK;
+	regs->pstate |= __this_cpu_read(kgdb_ss_flags);
+}
+
 int kgdb_arch_handle_exception(int exception_vector, int signo,
 			       int err_code, char *remcom_in_buffer,
 			       char *remcom_out_buffer,
@@ -201,8 +226,10 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
 		/*
 		 * Received continue command, disable single step
 		 */
-		if (kernel_active_single_step())
+		if (kernel_active_single_step()) {
+			kgdb_restore_local_irqflag(linux_regs);
 			kernel_disable_single_step();
+		}
 
 		err = 0;
 		break;
@@ -222,8 +249,10 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
 		/*
 		 * Enable single step handling
 		 */
-		if (!kernel_active_single_step())
+		if (!kernel_active_single_step()) {
+			kgdb_save_local_irqflag(linux_regs);
 			kernel_enable_single_step(linux_regs);
+		}
 		err = 0;
 		break;
 	default:
-- 
2.25.1

[PATCH 2/2] arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step

[Sumit Garg]

After fixing wrongly single-stepping into the irq handler, when we execute
single-step in kdb/kgdb, we can see only the first step can work.

Refer to the ARM Architecture Reference Manual (ARM DDI 0487E.a) D2.12,
i think PSTATE.SS=1 should be set each step for transferring the PE to the
'Active-not-pending' state. The problem here is PSTATE.SS=1 is not set
since the second single-step.

After the first single-step, the PE transferes to the 'Inactive' state,
with PSTATE.SS=0 and MDSCR.SS=1, thus PSTATE.SS won't be set to 1 due to
kernel_active_single_step()=true. Then the PE transferes to the
'Active-pending' state when ERET and returns to the debugger by step
exception.

Before this patch:
==================
Entering kdb (current=0xffff3376039f0000, pid 1) on processor 0 due to Keyboard Entry
[0]kdb>

[0]kdb>
[0]kdb> bp write_sysrq_trigger
Instruction(i) BP #0 at 0xffffa45c13d09290 (write_sysrq_trigger)
    is enabled   addr at ffffa45c13d09290, hardtype=0 installed=0

[0]kdb> go
$ echo h > /proc/sysrq-trigger

Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to Breakpoint @ 0xffffad651a309290
[1]kdb> ss

Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294
[1]kdb> ss

Entering kdb (current=0xffff4f7e453f8000, pid 175) on processor 1 due to SS trap @ 0xffffad651a309294
[1]kdb>

After this patch:
=================
Entering kdb (current=0xffff6851c39f0000, pid 1) on processor 0 due to Keyboard Entry
[0]kdb> bp write_sysrq_trigger
Instruction(i) BP #0 at 0xffffc02d2dd09290 (write_sysrq_trigger)
    is enabled   addr at ffffc02d2dd09290, hardtype=0 installed=0

[0]kdb> go
$ echo h > /proc/sysrq-trigger

Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to Breakpoint @ 0xffffc02d2dd09290
[1]kdb> ss

Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09294
[1]kdb> ss

Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd09298
[1]kdb> ss

Entering kdb (current=0xffff6851c53c1840, pid 174) on processor 1 due to SS trap @ 0xffffc02d2dd0929c
[1]kdb>

Fixes: 44679a4f142b ("arm64: KGDB: Add step debugging support")
Co-developed-by: Wei Li <liwei391@huawei.com>
Signed-off-by: Wei Li <liwei391@huawei.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 arch/arm64/include/asm/debug-monitors.h | 1 +
 arch/arm64/kernel/debug-monitors.c      | 5 +++++
 arch/arm64/kernel/kgdb.c                | 2 ++
 3 files changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 00c291067e57..9e1e864d6440 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -104,6 +104,7 @@ void user_regs_reset_single_step(struct user_pt_regs *regs,
 void kernel_enable_single_step(struct pt_regs *regs);
 void kernel_disable_single_step(void);
 int kernel_active_single_step(void);
+void kernel_regs_reset_single_step(struct pt_regs *regs);
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 int reinstall_suspended_bps(struct pt_regs *regs);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 4f3661eeb7ec..ea3f410aa385 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -438,6 +438,11 @@ int kernel_active_single_step(void)
 }
 NOKPROBE_SYMBOL(kernel_active_single_step);
 
+void kernel_regs_reset_single_step(struct pt_regs *regs)
+{
+	set_regs_spsr_ss(regs);
+}
+
 /* ptrace API */
 void user_enable_single_step(struct task_struct *task)
 {
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 653ad0d19f2f..783484a3a831 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -252,6 +252,8 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
 		if (!kernel_active_single_step()) {
 			kgdb_save_local_irqflag(linux_regs);
 			kernel_enable_single_step(linux_regs);
+		} else {
+			kernel_regs_reset_single_step(linux_regs);
 		}
 		err = 0;
 		break;
-- 
2.25.1

Re: [PATCH 0/2] arm64: kgdb/kdb: Fix pending single-step debugging issues

[Doug Anderson]

Hi,

On Mon, Apr 11, 2022 at 2:38 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> This patch-set reworks pending fixes from Wei's series [1] to make
> single-step debugging via kgdb/kdb on arm64 work as expected. There was
> a prior discussion on ML [2] regarding if we should keep the interrupts
> enabled during single-stepping but it turns out that in case of kgdb, it
> is risky to enable interrupts as sometimes a resume after single
> stepping an interrupt handler leads to following unbalanced locking
> issue:
>
> [  300.328300] WARNING: bad unlock balance detected!
> [  300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted
> [  300.329058] -------------------------------------
> [  300.329298] sh/173 is trying to release lock (dbg_slave_lock) at:
> [  300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820
> [  300.330029] but there are no more locks to release!
> [  300.330265]
> [  300.330265] other info that might help us debug this:
> [  300.330668] 4 locks held by sh/173:
> [  300.330891]  #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, at: vfs_write+0x98/0x204
> [  300.331735]  #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, at: kgdb_cpu_enter+0x5b4/0x820
> [  300.332259]  #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, at: kgdb_cpu_enter+0xe0/0x820
> [  300.332717]  #3: ffffd57c973bc2a8 (dbg_master_lock){....}-{2:2}, at: kgdb_cpu_enter+0x1ec/0x820
>
> So, I choose to keep interrupts disabled specifically for kgdb. This
> series has been rebased to Linux 5.18-rc1 and I have dropped Doug's
> review and test tags as there is significant rework involved.

Hmmmm. I guess it's really up to Will here, but re-reading his
previous email made it pretty clear that he wasn't willing to land a
solution that he wasn't willing to land a solution that left
interrupts disabled during step. He also pointed out some things that
would actually be broken, like single-stepping over a call to
irqs_disabled() or single stepping over something that caused an
exception where the exception handler needed interrupts enabled.

I thought he had a proposal at:

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

...that was supposed to make all the problems go away and it was just
that nobody had time to implement his proposal?


> [1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/
> [2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/
>
> Sumit Garg (2):
>   arm64: kgdb: Fix incorrect single stepping into the irq handler
>   arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step
>
>  arch/arm64/include/asm/debug-monitors.h |  1 +
>  arch/arm64/kernel/debug-monitors.c      |  5 ++++
>  arch/arm64/kernel/kgdb.c                | 35 +++++++++++++++++++++++--
>  3 files changed, 39 insertions(+), 2 deletions(-)
>
> --
> 2.25.1
>

Re: [PATCH 0/2] arm64: kgdb/kdb: Fix pending single-step debugging issues

[Sumit Garg]

Hi Doug,

Thanks for looking into this patch-set.

On Tue, 12 Apr 2022 at 05:39, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Apr 11, 2022 at 2:38 AM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > This patch-set reworks pending fixes from Wei's series [1] to make
> > single-step debugging via kgdb/kdb on arm64 work as expected. There was
> > a prior discussion on ML [2] regarding if we should keep the interrupts
> > enabled during single-stepping but it turns out that in case of kgdb, it
> > is risky to enable interrupts as sometimes a resume after single
> > stepping an interrupt handler leads to following unbalanced locking
> > issue:
> >
> > [  300.328300] WARNING: bad unlock balance detected!
> > [  300.328608] 5.18.0-rc1-00016-g3e732ebf7316-dirty #6 Not tainted
> > [  300.329058] -------------------------------------
> > [  300.329298] sh/173 is trying to release lock (dbg_slave_lock) at:
> > [  300.329718] [<ffffd57c951c016c>] kgdb_cpu_enter+0x7ac/0x820
> > [  300.330029] but there are no more locks to release!
> > [  300.330265]
> > [  300.330265] other info that might help us debug this:
> > [  300.330668] 4 locks held by sh/173:
> > [  300.330891]  #0: ffff4f5e454d8438 (sb_writers#3){.+.+}-{0:0}, at: vfs_write+0x98/0x204
> > [  300.331735]  #1: ffffd57c973bc2f0 (dbg_slave_lock){+.+.}-{2:2}, at: kgdb_cpu_enter+0x5b4/0x820
> > [  300.332259]  #2: ffffd57c973a9460 (rcu_read_lock){....}-{1:2}, at: kgdb_cpu_enter+0xe0/0x820
> > [  300.332717]  #3: ffffd57c973bc2a8 (dbg_master_lock){....}-{2:2}, at: kgdb_cpu_enter+0x1ec/0x820
> >
> > So, I choose to keep interrupts disabled specifically for kgdb. This
> > series has been rebased to Linux 5.18-rc1 and I have dropped Doug's
> > review and test tags as there is significant rework involved.
>
> Hmmmm. I guess it's really up to Will here, but re-reading his
> previous email made it pretty clear that he wasn't willing to land a
> solution that he wasn't willing to land a solution that left
> interrupts disabled during step. He also pointed out some things that
> would actually be broken, like single-stepping over a call to
> irqs_disabled() or single stepping over something that caused an
> exception where the exception handler needed interrupts enabled.
>
> I thought he had a proposal at:
>
> https://lore.kernel.org/r/20200626095551.GA9312@willie-the-truck
>
> ...that was supposed to make all the problems go away and it was just
> that nobody had time to implement his proposal?
>

So I took a shot at Will's proposal as a replacement of patch #1 in v2
[1]. I hope that it is aligned with Will's thinking.

[1] https://lkml.org/lkml/2022/4/13/136

-Sumit

>
> > [1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/
> > [2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/
> >
> > Sumit Garg (2):
> >   arm64: kgdb: Fix incorrect single stepping into the irq handler
> >   arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step
> >
> >  arch/arm64/include/asm/debug-monitors.h |  1 +
> >  arch/arm64/kernel/debug-monitors.c      |  5 ++++
> >  arch/arm64/kernel/kgdb.c                | 35 +++++++++++++++++++++++--
> >  3 files changed, 39 insertions(+), 2 deletions(-)
> >
> > --
> > 2.25.1
> >