All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] MIPS: KGDB: Use kernel context for sleeping threads
@ 2017-03-30 15:06 ` James Hogan
  0 siblings, 0 replies; 6+ messages in thread
From: James Hogan @ 2017-03-30 15:06 UTC (permalink / raw)
  To: Ralf Baechle; +Cc: James Hogan, Jason Wessel, linux-mips, stable

KGDB is a kernel debug stub and it can't be used to debug userland as it
can only safely access kernel memory.

On MIPS however KGDB has always got the register state of sleeping
processes from the userland register context at the beginning of the
kernel stack. This is meaningless for kernel threads (which never enter
userland), and for user threads it prevents the user seeing what it is
doing while in the kernel:

(gdb) info threads
  Id   Target Id         Frame
  ...
  3    Thread 2 (kthreadd) 0x0000000000000000 in ?? ()
  2    Thread 1 (init)   0x000000007705c4b4 in ?? ()
  1    Thread -2 (shadowCPU0) 0xffffffff8012524c in arch_kgdb_breakpoint () at arch/mips/kernel/kgdb.c:201

Get the register state instead from the (partial) kernel register
context stored in the task's thread_struct for resume() to restore. All
threads now correctly appear to be in context_switch():

(gdb) info threads
  Id   Target Id         Frame
  ...
  3    Thread 2 (kthreadd) context_switch (rq=<optimized out>, cookie=..., next=<optimized out>, prev=0x0) at kernel/sched/core.c:2903
  2    Thread 1 (init)   context_switch (rq=<optimized out>, cookie=..., next=<optimized out>, prev=0x0) at kernel/sched/core.c:2903
  1    Thread -2 (shadowCPU0) 0xffffffff8012524c in arch_kgdb_breakpoint () at arch/mips/kernel/kgdb.c:201

Call clobbered registers which aren't saved and exception registers
(BadVAddr & Cause) which can't be easily determined without stack
unwinding are reported as 0. The PC is taken from the return address,
such that the state presented matches that found immediately after
returning from resume().

Fixes: 8854700115ec ("[MIPS] kgdb: add arch support for the kernel's kgdb core")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: linux-mips@linux-mips.org
Cc: stable@vger.kernel.org
---
 arch/mips/kernel/kgdb.c | 48 ++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/arch/mips/kernel/kgdb.c b/arch/mips/kernel/kgdb.c
index 1f4bd222ba76..eb6c0d582626 100644
--- a/arch/mips/kernel/kgdb.c
+++ b/arch/mips/kernel/kgdb.c
@@ -244,9 +244,6 @@ static int compute_signal(int tt)
 void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
 {
 	int reg;
-	struct thread_info *ti = task_thread_info(p);
-	unsigned long ksp = (unsigned long)ti + THREAD_SIZE - 32;
-	struct pt_regs *regs = (struct pt_regs *)ksp - 1;
 #if (KGDB_GDB_REG_SIZE == 32)
 	u32 *ptr = (u32 *)gdb_regs;
 #else
@@ -254,25 +251,46 @@ void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
 #endif
 
 	for (reg = 0; reg < 16; reg++)
-		*(ptr++) = regs->regs[reg];
+		*(ptr++) = 0;
 
 	/* S0 - S7 */
-	for (reg = 16; reg < 24; reg++)
-		*(ptr++) = regs->regs[reg];
+	*(ptr++) = p->thread.reg16;
+	*(ptr++) = p->thread.reg17;
+	*(ptr++) = p->thread.reg18;
+	*(ptr++) = p->thread.reg19;
+	*(ptr++) = p->thread.reg20;
+	*(ptr++) = p->thread.reg21;
+	*(ptr++) = p->thread.reg22;
+	*(ptr++) = p->thread.reg23;
 
 	for (reg = 24; reg < 28; reg++)
 		*(ptr++) = 0;
 
 	/* GP, SP, FP, RA */
-	for (reg = 28; reg < 32; reg++)
-		*(ptr++) = regs->regs[reg];
-
-	*(ptr++) = regs->cp0_status;
-	*(ptr++) = regs->lo;
-	*(ptr++) = regs->hi;
-	*(ptr++) = regs->cp0_badvaddr;
-	*(ptr++) = regs->cp0_cause;
-	*(ptr++) = regs->cp0_epc;
+	*(ptr++) = (long)p;
+	*(ptr++) = p->thread.reg29;
+	*(ptr++) = p->thread.reg30;
+	*(ptr++) = p->thread.reg31;
+
+	*(ptr++) = p->thread.cp0_status;
+
+	/* lo, hi */
+	*(ptr++) = 0;
+	*(ptr++) = 0;
+
+	/*
+	 * BadVAddr, Cause
+	 * Ideally these would come from the last exception frame up the stack
+	 * but that requires unwinding, otherwise we can't know much for sure.
+	 */
+	*(ptr++) = 0;
+	*(ptr++) = 0;
+
+	/*
+	 * PC
+	 * use return address (RA), i.e. the moment after return from resume()
+	 */
+	*(ptr++) = p->thread.reg31;
 }
 
 void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
-- 
git-series 0.8.10

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] MIPS: KGDB: Use kernel context for sleeping threads
@ 2017-03-30 15:06 ` James Hogan
  0 siblings, 0 replies; 6+ messages in thread
From: James Hogan @ 2017-03-30 15:06 UTC (permalink / raw)
  To: Ralf Baechle; +Cc: James Hogan, Jason Wessel, linux-mips, stable

KGDB is a kernel debug stub and it can't be used to debug userland as it
can only safely access kernel memory.

On MIPS however KGDB has always got the register state of sleeping
processes from the userland register context at the beginning of the
kernel stack. This is meaningless for kernel threads (which never enter
userland), and for user threads it prevents the user seeing what it is
doing while in the kernel:

(gdb) info threads
  Id   Target Id         Frame
  ...
  3    Thread 2 (kthreadd) 0x0000000000000000 in ?? ()
  2    Thread 1 (init)   0x000000007705c4b4 in ?? ()
  1    Thread -2 (shadowCPU0) 0xffffffff8012524c in arch_kgdb_breakpoint () at arch/mips/kernel/kgdb.c:201

Get the register state instead from the (partial) kernel register
context stored in the task's thread_struct for resume() to restore. All
threads now correctly appear to be in context_switch():

(gdb) info threads
  Id   Target Id         Frame
  ...
  3    Thread 2 (kthreadd) context_switch (rq=<optimized out>, cookie=..., next=<optimized out>, prev=0x0) at kernel/sched/core.c:2903
  2    Thread 1 (init)   context_switch (rq=<optimized out>, cookie=..., next=<optimized out>, prev=0x0) at kernel/sched/core.c:2903
  1    Thread -2 (shadowCPU0) 0xffffffff8012524c in arch_kgdb_breakpoint () at arch/mips/kernel/kgdb.c:201

Call clobbered registers which aren't saved and exception registers
(BadVAddr & Cause) which can't be easily determined without stack
unwinding are reported as 0. The PC is taken from the return address,
such that the state presented matches that found immediately after
returning from resume().

Fixes: 8854700115ec ("[MIPS] kgdb: add arch support for the kernel's kgdb core")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: linux-mips@linux-mips.org
Cc: stable@vger.kernel.org
---
 arch/mips/kernel/kgdb.c | 48 ++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/arch/mips/kernel/kgdb.c b/arch/mips/kernel/kgdb.c
index 1f4bd222ba76..eb6c0d582626 100644
--- a/arch/mips/kernel/kgdb.c
+++ b/arch/mips/kernel/kgdb.c
@@ -244,9 +244,6 @@ static int compute_signal(int tt)
 void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
 {
 	int reg;
-	struct thread_info *ti = task_thread_info(p);
-	unsigned long ksp = (unsigned long)ti + THREAD_SIZE - 32;
-	struct pt_regs *regs = (struct pt_regs *)ksp - 1;
 #if (KGDB_GDB_REG_SIZE == 32)
 	u32 *ptr = (u32 *)gdb_regs;
 #else
@@ -254,25 +251,46 @@ void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
 #endif
 
 	for (reg = 0; reg < 16; reg++)
-		*(ptr++) = regs->regs[reg];
+		*(ptr++) = 0;
 
 	/* S0 - S7 */
-	for (reg = 16; reg < 24; reg++)
-		*(ptr++) = regs->regs[reg];
+	*(ptr++) = p->thread.reg16;
+	*(ptr++) = p->thread.reg17;
+	*(ptr++) = p->thread.reg18;
+	*(ptr++) = p->thread.reg19;
+	*(ptr++) = p->thread.reg20;
+	*(ptr++) = p->thread.reg21;
+	*(ptr++) = p->thread.reg22;
+	*(ptr++) = p->thread.reg23;
 
 	for (reg = 24; reg < 28; reg++)
 		*(ptr++) = 0;
 
 	/* GP, SP, FP, RA */
-	for (reg = 28; reg < 32; reg++)
-		*(ptr++) = regs->regs[reg];
-
-	*(ptr++) = regs->cp0_status;
-	*(ptr++) = regs->lo;
-	*(ptr++) = regs->hi;
-	*(ptr++) = regs->cp0_badvaddr;
-	*(ptr++) = regs->cp0_cause;
-	*(ptr++) = regs->cp0_epc;
+	*(ptr++) = (long)p;
+	*(ptr++) = p->thread.reg29;
+	*(ptr++) = p->thread.reg30;
+	*(ptr++) = p->thread.reg31;
+
+	*(ptr++) = p->thread.cp0_status;
+
+	/* lo, hi */
+	*(ptr++) = 0;
+	*(ptr++) = 0;
+
+	/*
+	 * BadVAddr, Cause
+	 * Ideally these would come from the last exception frame up the stack
+	 * but that requires unwinding, otherwise we can't know much for sure.
+	 */
+	*(ptr++) = 0;
+	*(ptr++) = 0;
+
+	/*
+	 * PC
+	 * use return address (RA), i.e. the moment after return from resume()
+	 */
+	*(ptr++) = p->thread.reg31;
 }
 
 void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc)
-- 
git-series 0.8.10

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] MIPS: KGDB: Use kernel context for sleeping threads
  2017-03-30 15:06 ` James Hogan
  (?)
@ 2017-03-30 15:42 ` Sergei Shtylyov
  2017-03-30 15:55     ` James Hogan
  -1 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2017-03-30 15:42 UTC (permalink / raw)
  To: James Hogan, Ralf Baechle; +Cc: Jason Wessel, linux-mips, stable

Hello!

On 03/30/2017 06:06 PM, James Hogan wrote:

> KGDB is a kernel debug stub and it can't be used to debug userland as it
> can only safely access kernel memory.
>
> On MIPS however KGDB has always got the register state of sleeping
> processes from the userland register context at the beginning of the
> kernel stack. This is meaningless for kernel threads (which never enter
> userland), and for user threads it prevents the user seeing what it is
> doing while in the kernel:
>
> (gdb) info threads
>   Id   Target Id         Frame
>   ...
>   3    Thread 2 (kthreadd) 0x0000000000000000 in ?? ()
>   2    Thread 1 (init)   0x000000007705c4b4 in ?? ()
>   1    Thread -2 (shadowCPU0) 0xffffffff8012524c in arch_kgdb_breakpoint () at arch/mips/kernel/kgdb.c:201
>
> Get the register state instead from the (partial) kernel register
> context stored in the task's thread_struct for resume() to restore. All
> threads now correctly appear to be in context_switch():
>
> (gdb) info threads
>   Id   Target Id         Frame
>   ...
>   3    Thread 2 (kthreadd) context_switch (rq=<optimized out>, cookie=..., next=<optimized out>, prev=0x0) at kernel/sched/core.c:2903
>   2    Thread 1 (init)   context_switch (rq=<optimized out>, cookie=..., next=<optimized out>, prev=0x0) at kernel/sched/core.c:2903
>   1    Thread -2 (shadowCPU0) 0xffffffff8012524c in arch_kgdb_breakpoint () at arch/mips/kernel/kgdb.c:201
>
> Call clobbered registers which aren't saved and exception registers
> (BadVAddr & Cause) which can't be easily determined without stack
> unwinding are reported as 0. The PC is taken from the return address,
> such that the state presented matches that found immediately after
> returning from resume().
>
> Fixes: 8854700115ec ("[MIPS] kgdb: add arch support for the kernel's kgdb core")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Jason Wessel <jason.wessel@windriver.com>
> Cc: linux-mips@linux-mips.org
> Cc: stable@vger.kernel.org
> ---
>  arch/mips/kernel/kgdb.c | 48 ++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/arch/mips/kernel/kgdb.c b/arch/mips/kernel/kgdb.c
> index 1f4bd222ba76..eb6c0d582626 100644
> --- a/arch/mips/kernel/kgdb.c
> +++ b/arch/mips/kernel/kgdb.c
[...]
> @@ -254,25 +251,46 @@ void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
>  #endif
>
>  	for (reg = 0; reg < 16; reg++)
> -		*(ptr++) = regs->regs[reg];
> +		*(ptr++) = 0;

    Parens are not really necessary, you can get rid of them, while at it.

[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] MIPS: KGDB: Use kernel context for sleeping threads
@ 2017-03-30 15:55     ` James Hogan
  0 siblings, 0 replies; 6+ messages in thread
From: James Hogan @ 2017-03-30 15:55 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: Ralf Baechle, Jason Wessel, linux-mips, stable

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

Hi Sergei,

On Thu, Mar 30, 2017 at 06:42:08PM +0300, Sergei Shtylyov wrote:
> On 03/30/2017 06:06 PM, James Hogan wrote:
> > @@ -254,25 +251,46 @@ void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
> >  #endif
> >
> >  	for (reg = 0; reg < 16; reg++)
> > -		*(ptr++) = regs->regs[reg];
> > +		*(ptr++) = 0;
> 
>     Parens are not really necessary, you can get rid of them, while at it.

While not technically required, I disagree that we should get rid of
them, simply because after coding in C for almost 20 years I still had
to look at an operator precedence table to check which of post++ and
dereference operators take precedence.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] MIPS: KGDB: Use kernel context for sleeping threads
@ 2017-03-30 15:55     ` James Hogan
  0 siblings, 0 replies; 6+ messages in thread
From: James Hogan @ 2017-03-30 15:55 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: Ralf Baechle, Jason Wessel, linux-mips, stable

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

Hi Sergei,

On Thu, Mar 30, 2017 at 06:42:08PM +0300, Sergei Shtylyov wrote:
> On 03/30/2017 06:06 PM, James Hogan wrote:
> > @@ -254,25 +251,46 @@ void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
> >  #endif
> >
> >  	for (reg = 0; reg < 16; reg++)
> > -		*(ptr++) = regs->regs[reg];
> > +		*(ptr++) = 0;
> 
>     Parens are not really necessary, you can get rid of them, while at it.

While not technically required, I disagree that we should get rid of
them, simply because after coding in C for almost 20 years I still had
to look at an operator precedence table to check which of post++ and
dereference operators take precedence.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] MIPS: KGDB: Use kernel context for sleeping threads
  2017-03-30 15:55     ` James Hogan
  (?)
@ 2017-04-12 20:33     ` Ralf Baechle
  -1 siblings, 0 replies; 6+ messages in thread
From: Ralf Baechle @ 2017-04-12 20:33 UTC (permalink / raw)
  To: James Hogan; +Cc: Sergei Shtylyov, Jason Wessel, linux-mips, stable

On Thu, Mar 30, 2017 at 04:55:26PM +0100, James Hogan wrote:

> Hi Sergei,
> 
> On Thu, Mar 30, 2017 at 06:42:08PM +0300, Sergei Shtylyov wrote:
> > On 03/30/2017 06:06 PM, James Hogan wrote:
> > > @@ -254,25 +251,46 @@ void sleeping_thread_to_gdb_regs(unsigned long *gdb_regs, struct task_struct *p)
> > >  #endif
> > >
> > >  	for (reg = 0; reg < 16; reg++)
> > > -		*(ptr++) = regs->regs[reg];
> > > +		*(ptr++) = 0;
> > 
> >     Parens are not really necessary, you can get rid of them, while at it.
> 
> While not technically required, I disagree that we should get rid of
> them, simply because after coding in C for almost 20 years I still had
> to look at an operator precedence table to check which of post++ and
> dereference operators take precedence.

I strongly side with James on this one so I applied the patch as-is.

  Ralf

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-04-12 20:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 15:06 [PATCH] MIPS: KGDB: Use kernel context for sleeping threads James Hogan
2017-03-30 15:06 ` James Hogan
2017-03-30 15:42 ` Sergei Shtylyov
2017-03-30 15:55   ` James Hogan
2017-03-30 15:55     ` James Hogan
2017-04-12 20:33     ` Ralf Baechle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.