[PATCH 0/5] kdb: Don't implicitly change tasks; plus misc fixups

[PATCH 0/5] kdb: Don't implicitly change tasks; plus misc fixups

[Douglas Anderson]

This started out as just a follow-up to Daniel's post [1].  I wanted
to stop implicitly changing the current task in kdb.  ...but, of
course, everywhere you look in kdb there are things to cleanup, so
this gets a few misc cleanups I found along the way.  Enjoy.

[1] https://lore.kernel.org/r/20191010150735.dhrj3pbjgmjrdpwr@holly.lan


Douglas Anderson (5):
  MIPS: kdb: Remove old workaround for backtracing on other CPUs
  kdb: kdb_current_regs should be private
  kdb: kdb_current_task shouldn't be exported
  kdb: Gid rid of implicit setting of the current task / regs
  kdb: Get rid of confusing diag msg from "rd" if current task has no
    regs

 arch/mips/kernel/traps.c       |  5 -----
 include/linux/kdb.h            |  2 --
 kernel/debug/kdb/kdb_bt.c      |  8 +-------
 kernel/debug/kdb/kdb_main.c    | 31 ++++++++++++++-----------------
 kernel/debug/kdb/kdb_private.h |  2 +-
 5 files changed, 16 insertions(+), 32 deletions(-)

-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 1/5] MIPS: kdb: Remove old workaround for backtracing on other CPUs

[Douglas Anderson]

As of commit 2277b492582d ("kdb: Fix stack crawling on 'running' CPUs
that aren't the master") we no longer need any special case for doing
stack dumps on CPUs that are not the kdb master.  Let's remove.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I have no way to test this personally, so hopefully someone who uses
kdb/kgdb on MIPS can.

Ideally this patch should be Acked by MIPS folks and then land through
the kdb/kgdb tree since the next patch in the series, ("kdb:
kdb_current_regs should be private") depends on it.

 arch/mips/kernel/traps.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 342e41de9d64..46fbcfeaae9a 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -210,11 +210,6 @@ void show_stack(struct task_struct *task, unsigned long *sp)
 			regs.regs[29] = task->thread.reg29;
 			regs.regs[31] = 0;
 			regs.cp0_epc = task->thread.reg31;
-#ifdef CONFIG_KGDB_KDB
-		} else if (atomic_read(&kgdb_active) != -1 &&
-			   kdb_current_regs) {
-			memcpy(&regs, kdb_current_regs, sizeof(regs));
-#endif /* CONFIG_KGDB_KDB */
 		} else {
 			prepare_frametrace(&regs);
 		}
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 2/5] kdb: kdb_current_regs should be private

[Douglas Anderson]

As of the patch ("MIPS: kdb: Remove old workaround for backtracing on
other CPUs") there is no reason for kdb_current_regs to be in the
public "kdb.h".  Let's move it next to kdb_current_task.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 include/linux/kdb.h            | 2 --
 kernel/debug/kdb/kdb_private.h | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/kdb.h b/include/linux/kdb.h
index 68bd88223417..24cd447659e0 100644
--- a/include/linux/kdb.h
+++ b/include/linux/kdb.h
@@ -183,8 +183,6 @@ int kdb_process_cpu(const struct task_struct *p)
 	return cpu;
 }
 
-/* kdb access to register set for stack dumping */
-extern struct pt_regs *kdb_current_regs;
 #ifdef CONFIG_KALLSYMS
 extern const char *kdb_walk_kallsyms(loff_t *pos);
 #else /* ! CONFIG_KALLSYMS */
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index 55d052061ef9..e829b22f3946 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -242,6 +242,7 @@ extern void debug_kusage(void);
 
 extern void kdb_set_current_task(struct task_struct *);
 extern struct task_struct *kdb_current_task;
+extern struct pt_regs *kdb_current_regs;
 
 #ifdef CONFIG_KDB_KEYBOARD
 extern void kdb_kbd_cleanup_state(void);
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 3/5] kdb: kdb_current_task shouldn't be exported

[Douglas Anderson]

The kdb_current_task variable has been declared in
"kernel/debug/kdb/kdb_private.h" since 2010 when kdb was added to the
mainline kernel.  This is not a public header.  There should be no
reason that kdb_current_task should be exported and there are no
in-kernel users that need it.  Remove the export.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 4567fe998c30..4d44b3746836 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -73,7 +73,6 @@ int kdb_nextline = 1;
 int kdb_state;			/* General KDB state */
 
 struct task_struct *kdb_current_task;
-EXPORT_SYMBOL(kdb_current_task);
 struct pt_regs *kdb_current_regs;
 
 const char *kdb_diemsg;
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 4/5] kdb: Gid rid of implicit setting of the current task / regs

[Douglas Anderson]

Some (but not all?) of the kdb backtrace paths would cause the
kdb_current_task and kdb_current_regs to remain changed.  As discussed
in a review of a previous patch [1], this doesn't seem intuitive, so
let's fix that.

...but, it turns out that there's actually no longer any reason to set
the current task / current regs while backtracing anymore anyway.  As
of commit 2277b492582d ("kdb: Fix stack crawling on 'running' CPUs
that aren't the master") if we're backtracing on a task running on a
CPU we ask that CPU to do the backtrace itself.  Linux can do that
without anything fancy.  If we're doing backtrace on a sleeping task
we can also do that fine without updating globals.  So this patch
mostly just turns into deleting a bunch of code.

[1] https://lore.kernel.org/r/20191010150735.dhrj3pbjgmjrdpwr@holly.lan

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_bt.c      | 8 +-------
 kernel/debug/kdb/kdb_main.c    | 2 +-
 kernel/debug/kdb/kdb_private.h | 1 -
 3 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/kernel/debug/kdb/kdb_bt.c b/kernel/debug/kdb/kdb_bt.c
index 4af48ac53625..3de0cc780c16 100644
--- a/kernel/debug/kdb/kdb_bt.c
+++ b/kernel/debug/kdb/kdb_bt.c
@@ -119,7 +119,6 @@ kdb_bt_cpu(unsigned long cpu)
 		return;
 	}
 
-	kdb_set_current_task(kdb_tsk);
 	kdb_bt1(kdb_tsk, ~0UL, false);
 }
 
@@ -166,10 +165,8 @@ kdb_bt(int argc, const char **argv)
 		if (diag)
 			return diag;
 		p = find_task_by_pid_ns(pid, &init_pid_ns);
-		if (p) {
-			kdb_set_current_task(p);
+		if (p)
 			return kdb_bt1(p, ~0UL, false);
-		}
 		kdb_printf("No process with pid == %ld found\n", pid);
 		return 0;
 	} else if (strcmp(argv[0], "btt") == 0) {
@@ -178,11 +175,9 @@ kdb_bt(int argc, const char **argv)
 		diag = kdbgetularg((char *)argv[1], &addr);
 		if (diag)
 			return diag;
-		kdb_set_current_task((struct task_struct *)addr);
 		return kdb_bt1((struct task_struct *)addr, ~0UL, false);
 	} else if (strcmp(argv[0], "btc") == 0) {
 		unsigned long cpu = ~0;
-		struct task_struct *save_current_task = kdb_current_task;
 		if (argc > 1)
 			return KDB_ARGCOUNT;
 		if (argc == 1) {
@@ -204,7 +199,6 @@ kdb_bt(int argc, const char **argv)
 				kdb_bt_cpu(cpu);
 				touch_nmi_watchdog();
 			}
-			kdb_set_current_task(save_current_task);
 		}
 		return 0;
 	} else {
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 4d44b3746836..ba12e9f4661e 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1138,7 +1138,7 @@ static void kdb_dumpregs(struct pt_regs *regs)
 	console_loglevel = old_lvl;
 }
 
-void kdb_set_current_task(struct task_struct *p)
+static void kdb_set_current_task(struct task_struct *p)
 {
 	kdb_current_task = p;
 
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
index e829b22f3946..2e296e4a234c 100644
--- a/kernel/debug/kdb/kdb_private.h
+++ b/kernel/debug/kdb/kdb_private.h
@@ -240,7 +240,6 @@ extern void *debug_kmalloc(size_t size, gfp_t flags);
 extern void debug_kfree(void *);
 extern void debug_kusage(void);
 
-extern void kdb_set_current_task(struct task_struct *);
 extern struct task_struct *kdb_current_task;
 extern struct pt_regs *kdb_current_regs;
 
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 5/5] kdb: Get rid of confusing diag msg from "rd" if current task has no regs

[Douglas Anderson]

If you switch to a sleeping task with the "pid" command and then type
"rd", kdb tells you this:

  No current kdb registers.  You may need to select another task
  diag: -17: Invalid register name

The first message makes sense, but not the second.  Fix it by just
returning 0 after commands accessing the current registers finish if
we've already printed the "No current kdb registers" error.

While fixing kdb_rd(), change the function to use "if" rather than
"ifdef".  It cleans the function up a bit and any modern compiler will
have no trouble handling still producing good code.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 kernel/debug/kdb/kdb_main.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index ba12e9f4661e..b22292b649c4 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -543,9 +543,8 @@ int kdbgetaddrarg(int argc, const char **argv, int *nextarg,
 		if (diag)
 			return diag;
 	} else if (symname[0] == '%') {
-		diag = kdb_check_regs();
-		if (diag)
-			return diag;
+		if (kdb_check_regs())
+			return 0;
 		/* Implement register values with % at a later time as it is
 		 * arch optional.
 		 */
@@ -1836,8 +1835,7 @@ static int kdb_go(int argc, const char **argv)
  */
 static int kdb_rd(int argc, const char **argv)
 {
-	int len = kdb_check_regs();
-#if DBG_MAX_REG_NUM > 0
+	int len = 0;
 	int i;
 	char *rname;
 	int rsize;
@@ -1846,8 +1844,14 @@ static int kdb_rd(int argc, const char **argv)
 	u16 reg16;
 	u8 reg8;
 
-	if (len)
-		return len;
+	if (kdb_check_regs())
+		return 0;
+
+	/* Fallback to Linux showregs() if we don't have DBG_MAX_REG_NUM */
+	if (DBG_MAX_REG_NUM <= 0) {
+		kdb_dumpregs(kdb_current_regs);
+		return 0;
+	}
 
 	for (i = 0; i < DBG_MAX_REG_NUM; i++) {
 		rsize = dbg_reg_def[i].size * 2;
@@ -1889,12 +1893,7 @@ static int kdb_rd(int argc, const char **argv)
 		}
 	}
 	kdb_printf("\n");
-#else
-	if (len)
-		return len;
 
-	kdb_dumpregs(kdb_current_regs);
-#endif
 	return 0;
 }
 
@@ -1928,9 +1927,8 @@ static int kdb_rm(int argc, const char **argv)
 	if (diag)
 		return diag;
 
-	diag = kdb_check_regs();
-	if (diag)
-		return diag;
+	if (kdb_check_regs())
+		return 0;
 
 	diag = KDB_BADREG;
 	for (i = 0; i < DBG_MAX_REG_NUM; i++) {
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 1/5] MIPS: kdb: Remove old workaround for backtracing on other CPUs

[Daniel Thompson]

On Sat, Nov 09, 2019 at 11:16:40AM -0800, Douglas Anderson wrote:
> As of commit 2277b492582d ("kdb: Fix stack crawling on 'running' CPUs
> that aren't the master") we no longer need any special case for doing
> stack dumps on CPUs that are not the kdb master.  Let's remove.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> I have no way to test this personally, so hopefully someone who uses
> kdb/kgdb on MIPS can.

I took this as a hint to add mips support to kgdbtest ;-)

Support is added and working well. Unfortunately lack of familiarity
with mips means I have not yet figured out which mips defconfig gives
us working SMP (and what the corresponding qemu invocation should be).

I think that means I still can't (quite) exercise this code fully.
The most appropriate test is bta on an SMP system, right?


> Ideally this patch should be Acked by MIPS folks and then land through
> the kdb/kgdb tree since the next patch in the series, ("kdb:
> kdb_current_regs should be private") depends on it.

An Acked-by from a MIPS maintainer would be very welcome. Perhaps
with a bit of extra work on the above I might be able to provide
a Tested-by:.

I didn't see anything that particularly bothered me in the patches but
given we're already at -rc7 I'm inclined to target this patchset for 5.6
rather than 5.5.


Daniel.

Re: [PATCH 1/5] MIPS: kdb: Remove old workaround for backtracing on other CPUs

[Doug Anderson]

Hi,

On Thu, Nov 14, 2019 at 2:51 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Sat, Nov 09, 2019 at 11:16:40AM -0800, Douglas Anderson wrote:
> > As of commit 2277b492582d ("kdb: Fix stack crawling on 'running' CPUs
> > that aren't the master") we no longer need any special case for doing
> > stack dumps on CPUs that are not the kdb master.  Let's remove.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > I have no way to test this personally, so hopefully someone who uses
> > kdb/kgdb on MIPS can.
>
> I took this as a hint to add mips support to kgdbtest ;-)
>
> Support is added and working well. Unfortunately lack of familiarity
> with mips means I have not yet figured out which mips defconfig gives
> us working SMP (and what the corresponding qemu invocation should be).

Nice!


> I think that means I still can't (quite) exercise this code fully.
> The most appropriate test is bta on an SMP system, right?

Yeah, or at least "btc".


> > Ideally this patch should be Acked by MIPS folks and then land through
> > the kdb/kgdb tree since the next patch in the series, ("kdb:
> > kdb_current_regs should be private") depends on it.
>
> An Acked-by from a MIPS maintainer would be very welcome. Perhaps
> with a bit of extra work on the above I might be able to provide
> a Tested-by:.
>
> I didn't see anything that particularly bothered me in the patches but
> given we're already at -rc7 I'm inclined to target this patchset for 5.6
> rather than 5.5.

That's fine.  This is all just cleanup stuff so targeting 5.6 is fine.

-Doug

Re: [PATCH 1/5] MIPS: kdb: Remove old workaround for backtracing on other CPUs

[Paul Burton]

Hi Daniel,

On Thu, Nov 14, 2019 at 10:51:25AM +0000, Daniel Thompson wrote:
> On Sat, Nov 09, 2019 at 11:16:40AM -0800, Douglas Anderson wrote:
> > As of commit 2277b492582d ("kdb: Fix stack crawling on 'running' CPUs
> > that aren't the master") we no longer need any special case for doing
> > stack dumps on CPUs that are not the kdb master.  Let's remove.
> > 
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > I have no way to test this personally, so hopefully someone who uses
> > kdb/kgdb on MIPS can.
> 
> I took this as a hint to add mips support to kgdbtest ;-)

Wonderful! :)

> Support is added and working well. Unfortunately lack of familiarity
> with mips means I have not yet figured out which mips defconfig gives
> us working SMP (and what the corresponding qemu invocation should be).

You can build 64r6el_defconfig & boot it something like this:

$ qemu-system-mips64el \
    -M boston -cpu I6500 -smp 4 \
    -kernel arch/mips/boot/vmlinux.gz.itb \
    -serial stdio \
    -hda my-disk-image.bin \
    -append "root=/dev/sda"

Linux should see the system as a single core with 4 hardware threads
(VPs or Virtual Processors in MIPS terminology).

> > Ideally this patch should be Acked by MIPS folks and then land through
> > the kdb/kgdb tree since the next patch in the series, ("kdb:
> > kdb_current_regs should be private") depends on it.
> 
> An Acked-by from a MIPS maintainer would be very welcome. Perhaps
> with a bit of extra work on the above I might be able to provide
> a Tested-by:.

The patches look reasonable to me; I was hoping to test them before
giving an ack but haven't had the time yet. It seems you may be making
that easier :)

Thanks,
    Paul

Re: [PATCH 0/5] kdb: Don't implicitly change tasks; plus misc fixups

[Doug Anderson]

Hi

On Sat, Nov 9, 2019 at 11:17 AM Douglas Anderson <dianders@chromium.org> wrote:
>
> This started out as just a follow-up to Daniel's post [1].  I wanted
> to stop implicitly changing the current task in kdb.  ...but, of
> course, everywhere you look in kdb there are things to cleanup, so
> this gets a few misc cleanups I found along the way.  Enjoy.
>
> [1] https://lore.kernel.org/r/20191010150735.dhrj3pbjgmjrdpwr@holly.lan
>
>
> Douglas Anderson (5):
>   MIPS: kdb: Remove old workaround for backtracing on other CPUs
>   kdb: kdb_current_regs should be private
>   kdb: kdb_current_task shouldn't be exported
>   kdb: Gid rid of implicit setting of the current task / regs
>   kdb: Get rid of confusing diag msg from "rd" if current task has no
>     regs
>
>  arch/mips/kernel/traps.c       |  5 -----
>  include/linux/kdb.h            |  2 --
>  kernel/debug/kdb/kdb_bt.c      |  8 +-------
>  kernel/debug/kdb/kdb_main.c    | 31 ++++++++++++++-----------------
>  kernel/debug/kdb/kdb_private.h |  2 +-
>  5 files changed, 16 insertions(+), 32 deletions(-)

I noticed that this series doesn't seem to be in linux-next, but I
think it was supposed to target v5.6?  Do you know if there is
anything outstanding or if it'll be queued up sometime soon?

Thanks!


-Doug

Re: [PATCH 0/5] kdb: Don't implicitly change tasks; plus misc fixups

[Andrew Morton]

On Tue, 28 Jan 2020 08:59:01 -0800 Doug Anderson <dianders@chromium.org> wrote:

> Hi
> 
> On Sat, Nov 9, 2019 at 11:17 AM Douglas Anderson <dianders@chromium.org> wrote:
> >
> > This started out as just a follow-up to Daniel's post [1].  I wanted
> > to stop implicitly changing the current task in kdb.  ...but, of
> > course, everywhere you look in kdb there are things to cleanup, so
> > this gets a few misc cleanups I found along the way.  Enjoy.
> >
> > [1] https://lore.kernel.org/r/20191010150735.dhrj3pbjgmjrdpwr@holly.lan
> >
> >
> > Douglas Anderson (5):
> >   MIPS: kdb: Remove old workaround for backtracing on other CPUs
> >   kdb: kdb_current_regs should be private
> >   kdb: kdb_current_task shouldn't be exported
> >   kdb: Gid rid of implicit setting of the current task / regs
> >   kdb: Get rid of confusing diag msg from "rd" if current task has no
> >     regs
> >
> >  arch/mips/kernel/traps.c       |  5 -----
> >  include/linux/kdb.h            |  2 --
> >  kernel/debug/kdb/kdb_bt.c      |  8 +-------
> >  kernel/debug/kdb/kdb_main.c    | 31 ++++++++++++++-----------------
> >  kernel/debug/kdb/kdb_private.h |  2 +-
> >  5 files changed, 16 insertions(+), 32 deletions(-)
> 
> I noticed that this series doesn't seem to be in linux-next, but I
> think it was supposed to target v5.6?  Do you know if there is
> anything outstanding or if it'll be queued up sometime soon?
> 

I grabbed them.

Are there any updates on the testing status, particularly on MIPS?

Re: [PATCH 0/5] kdb: Don't implicitly change tasks; plus misc fixups

[Daniel Thompson]

On Tue, Jan 28, 2020 at 05:42:48PM -0800, Andrew Morton wrote:
> On Tue, 28 Jan 2020 08:59:01 -0800 Doug Anderson <dianders@chromium.org> wrote:
> 
> > Hi
> > 
> > On Sat, Nov 9, 2019 at 11:17 AM Douglas Anderson <dianders@chromium.org> wrote:
> > >
> > > This started out as just a follow-up to Daniel's post [1].  I wanted
> > > to stop implicitly changing the current task in kdb.  ...but, of
> > > course, everywhere you look in kdb there are things to cleanup, so
> > > this gets a few misc cleanups I found along the way.  Enjoy.
> > >
> > > [1] https://lore.kernel.org/r/20191010150735.dhrj3pbjgmjrdpwr@holly.lan
> > >
> > >
> > > Douglas Anderson (5):
> > >   MIPS: kdb: Remove old workaround for backtracing on other CPUs
> > >   kdb: kdb_current_regs should be private
> > >   kdb: kdb_current_task shouldn't be exported
> > >   kdb: Gid rid of implicit setting of the current task / regs
> > >   kdb: Get rid of confusing diag msg from "rd" if current task has no
> > >     regs
> > >
> > >  arch/mips/kernel/traps.c       |  5 -----
> > >  include/linux/kdb.h            |  2 --
> > >  kernel/debug/kdb/kdb_bt.c      |  8 +-------
> > >  kernel/debug/kdb/kdb_main.c    | 31 ++++++++++++++-----------------
> > >  kernel/debug/kdb/kdb_private.h |  2 +-
> > >  5 files changed, 16 insertions(+), 32 deletions(-)
> > 
> > I noticed that this series doesn't seem to be in linux-next, but I
> > think it was supposed to target v5.6?  Do you know if there is
> > anything outstanding or if it'll be queued up sometime soon?
> > 
> 
> I grabbed them.
> 
> Are there any updates on the testing status, particularly on MIPS?

I put in a fair bit of work earlier this dev cycle to add MIPS to the
kgdb test suite. Unfortunately that left me catching up on a few other
things... or putting it another way I am late putting together the kgdb
tree for v5.6. It will be done by the weekend.

The first patch never got a formal Acked-by from the MIPS folks but they
commented positively so we can probably go ahead.


Daniel.