Breakage in arch/mips/kernel/traps.c for 64bit

Breakage in arch/mips/kernel/traps.c for 64bit

[Thomas Bogendoerfer]

Hi,

it would be nice, if people started thinking before supplying such
crappy^Winteresting code:

arch/mips/kernel/traps.c:

#define IS_KVA01(a) ((((unsigned int)a) & 0xc0000000) == 0x80000000)

Kills every 64bit kernel build...

Why is this needed at all ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: Breakage in arch/mips/kernel/traps.c for 64bit

[Maciej W. Rozycki]

On Thu, 1 May 2008, Thomas Bogendoerfer wrote:

> it would be nice, if people started thinking before supplying such
> crappy^Winteresting code:
> 
> arch/mips/kernel/traps.c:
> 
> #define IS_KVA01(a) ((((unsigned int)a) & 0xc0000000) == 0x80000000)
> 
> Kills every 64bit kernel build...

 Not everybody tests 64-bit stuff as some people limit themselves to
32-bit systems only.  It looks like a step backwards, but there you go.

> Why is this needed at all ?

 It looks like an attempt to avoid TLB exceptions for the stack dump -- if
that is the case, then obviously a piece of code like one in
arch/mips/lib/uncached.c should be used to check for CKSEG0/1 and XKPHYS.  
If there are two uses of this code, then it should be wrapped in an inline
function and put in a header; <asm/addrspace.h>, perhaps.

  Maciej

Re: Breakage in arch/mips/kernel/traps.c for 64bit

[Ralf Baechle]

On Thu, May 01, 2008 at 06:33:14PM +0200, Thomas Bogendoerfer wrote:

> it would be nice, if people started thinking before supplying such
> crappy^Winteresting code:
> 
> arch/mips/kernel/traps.c:
> 
> #define IS_KVA01(a) ((((unsigned int)a) & 0xc0000000) == 0x80000000)
> 
> Kills every 64bit kernel build...
> 
> Why is this needed at all ?

It came as part of 39b8d5254246ac56342b72f812255c8f7a74dca9 which is a
patch amalgated from several other patches.  Below is the original patch
it came with.  I think the idea of the patch is valid but the idea needs a
bit of mending.

From: Chris Dearman <chris@mips.com>
Date: Wed, 3 Oct 2007 10:19:18 +0100
Subject: [PATCH] Skip raw backtrace for non KSEG stack addresses
 This is to avoid recursive stackdumps as the kernel kernel falls off
 of the user stack.

Signed-off-by: Chris Dearman <chris@mips.com>

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 2948b86..3d56171 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -79,19 +79,22 @@ void (*board_bind_eic_interrupt)(int irq, int regset);
 
 static void show_raw_backtrace(unsigned long reg29)
 {
-	unsigned long *sp = (unsigned long *)reg29;
+	unsigned long *sp = (unsigned long *)(reg29 & ~3);
 	unsigned long addr;
 
 	printk("Call Trace:");
 #ifdef CONFIG_KALLSYMS
 	printk("\n");
 #endif
-	while (!kstack_end(sp)) {
-		addr = *sp++;
-		if (__kernel_text_address(addr))
-			print_ip_sym(addr);
+#define IS_KVA01(a) ((((unsigned int)a) & 0xc0000000) == 0x80000000)
+	if (IS_KVA01(sp)) {
+		while (!kstack_end(sp)) {
+			addr = *sp++;
+			if (__kernel_text_address(addr))
+				print_ip_sym(addr);
+		}
+		printk("\n");
 	}
-	printk("\n");
 }
 
 #ifdef CONFIG_KALLSYMS

Re: Breakage in arch/mips/kernel/traps.c for 64bit

[Atsushi Nemoto]

On Fri, 2 May 2008 11:11:13 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:
> It came as part of 39b8d5254246ac56342b72f812255c8f7a74dca9 which is a
> patch amalgated from several other patches.  Below is the original patch
> it came with.  I think the idea of the patch is valid but the idea needs a
> bit of mending.

Then how about this fix?

---------------------------------------------------------------------
Subject: [PATCH] Fix detection of kernel segment on 64-bit

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index cb8b0e2..7893bb3 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -78,6 +78,19 @@ void (*board_nmi_handler_setup)(void);
 void (*board_ejtag_handler_setup)(void);
 void (*board_bind_eic_interrupt)(int irq, int regset);
 
+static inline int kernel_unmapped_seg(void *addr)
+{
+	unsigned long a = (unsigned long)addr;
+
+#ifdef CONFIG_32BIT
+	/* KSEG0 or KSEG1 */
+	return (a & 0xc0000000) == KSEG0;
+#else
+	/* CKSEG0, CKSEG1 or XKPHYS  */
+	return ((a & 0xffffffffc0000000L) == CKSEG0) ||
+		((a & 0xc000000000000000L) == XKPHYS);
+#endif
+}
 
 static void show_raw_backtrace(unsigned long reg29)
 {
@@ -88,8 +101,7 @@ static void show_raw_backtrace(unsigned long reg29)
 #ifdef CONFIG_KALLSYMS
 	printk("\n");
 #endif
-#define IS_KVA01(a) ((((unsigned int)a) & 0xc0000000) == 0x80000000)
-	if (IS_KVA01(sp)) {
+	if (kernel_unmapped_seg(sp)) {
 		while (!kstack_end(sp)) {
 			addr = *sp++;
 			if (__kernel_text_address(addr))

Re: Breakage in arch/mips/kernel/traps.c for 64bit

[Ralf Baechle]

On Sun, May 04, 2008 at 01:16:47AM +0900, Atsushi Nemoto wrote:

> Then how about this fix?
> 
> ---------------------------------------------------------------------
> Subject: [PATCH] Fix detection of kernel segment on 64-bit
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> ---
> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> index cb8b0e2..7893bb3 100644
> --- a/arch/mips/kernel/traps.c
> +++ b/arch/mips/kernel/traps.c
> @@ -78,6 +78,19 @@ void (*board_nmi_handler_setup)(void);
>  void (*board_ejtag_handler_setup)(void);
>  void (*board_bind_eic_interrupt)(int irq, int regset);
>  
> +static inline int kernel_unmapped_seg(void *addr)
> +{
> +	unsigned long a = (unsigned long)addr;
> +
> +#ifdef CONFIG_32BIT
> +	/* KSEG0 or KSEG1 */
> +	return (a & 0xc0000000) == KSEG0;

Slightly cleaner:

  return KSEGX(a) == KSEG0;

Unfortunately there is no such macro for the 64-bit segments nor does
the existing KSEGX() work correctly for non-CKSEGx 64-bit addresses.

  Ralf

Re: Breakage in arch/mips/kernel/traps.c for 64bit

[Maciej W. Rozycki]

On Sat, 3 May 2008, Ralf Baechle wrote:

> Slightly cleaner:
> 
>   return KSEGX(a) == KSEG0;

 You mean:

return KSEGX(a) == KSEG0 || KSEGX(a) == KSEG1;

right?

> Unfortunately there is no such macro for the 64-bit segments nor does
> the existing KSEGX() work correctly for non-CKSEGx 64-bit addresses.

 As I mentioned there is suitable code doing exactly this in
arch/mips/lib/uncached.c and it can be extracted to an inline function to
be put in <asm/addrspace.h> to be reused here and in the future possibly
elsewhere.

  Maciej

Re: Breakage in arch/mips/kernel/traps.c for 64bit

[Thomas Bogendoerfer]

On Sun, May 04, 2008 at 01:16:47AM +0900, Atsushi Nemoto wrote:
> On Fri, 2 May 2008 11:11:13 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:
> > It came as part of 39b8d5254246ac56342b72f812255c8f7a74dca9 which is a
> > patch amalgated from several other patches.  Below is the original patch
> > it came with.  I think the idea of the patch is valid but the idea needs a
> > bit of mending.
> 
> Then how about this fix?

hmm, why not simply use __get_user() when accessing the stack content ?
show_stacktrace() already does it for stack dumping ? This would
avoid any work for whatever sick stack mappings. Below is a patch,
which does this.

Thomas.

The newly added check for valid stack pointer address breaks at least for
64bit kernels.  Use __get_user() for accessing stack content to avoid crashes,
when doing the backtrace.

Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---

 arch/mips/kernel/traps.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index cb8b0e2..c9ce8d6 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -81,22 +81,22 @@ void (*board_bind_eic_interrupt)(int irq, int regset);
 
 static void show_raw_backtrace(unsigned long reg29)
 {
-	unsigned long *sp = (unsigned long *)(reg29 & ~3);
+	unsigned long __user *sp = (unsigned long __user *)(reg29 & ~3);
 	unsigned long addr;
 
 	printk("Call Trace:");
 #ifdef CONFIG_KALLSYMS
 	printk("\n");
 #endif
-#define IS_KVA01(a) ((((unsigned int)a) & 0xc0000000) == 0x80000000)
-	if (IS_KVA01(sp)) {
-		while (!kstack_end(sp)) {
-			addr = *sp++;
-			if (__kernel_text_address(addr))
-				print_ip_sym(addr);
+	while (!kstack_end(sp)) {
+		if (__get_user(addr, sp++)) {
+			printk(" (Bad stack address)");
+			break;
 		}
-		printk("\n");
+		if (__kernel_text_address(addr))
+			print_ip_sym(addr);
 	}
+	printk("\n");
 }
 
 #ifdef CONFIG_KALLSYMS

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: Breakage in arch/mips/kernel/traps.c for 64bit

[Atsushi Nemoto]

On Sun, 4 May 2008 00:48:49 +0200, tsbogend@alpha.franken.de (Thomas Bogendoerfer) wrote:
> hmm, why not simply use __get_user() when accessing the stack content ?
> show_stacktrace() already does it for stack dumping ? This would
> avoid any work for whatever sick stack mappings. Below is a patch,
> which does this.

I like this patch.  One minor request:

> +	unsigned long __user *sp = (unsigned long __user *)(reg29 & ~3);
...
> +	while (!kstack_end(sp)) {
> +		if (__get_user(addr, sp++)) {

This will leads a sparse warning since an argument for kstack_end is 'void *'.

	while (!kstack_end((void *)(unsigned long)sp)) {

will make this part sparse-free, though it seems a bit ugly.

---
Atsushi Nemoto

Re: Breakage in arch/mips/kernel/traps.c for 64bit

[Thomas Bogendoerfer]

On Sun, May 04, 2008 at 10:39:44PM +0900, Atsushi Nemoto wrote:
> > +	unsigned long __user *sp = (unsigned long __user *)(reg29 & ~3);
> ...
> > +	while (!kstack_end(sp)) {
> > +		if (__get_user(addr, sp++)) {
> 
> This will leads a sparse warning since an argument for kstack_end is 'void *'.
> 
> 	while (!kstack_end((void *)(unsigned long)sp)) {
> 
> will make this part sparse-free, though it seems a bit ugly.

hmm, would leaving sp as unsigned long * and casting it for __get_user()
make sparse happy ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: Breakage in arch/mips/kernel/traps.c for 64bit

[Atsushi Nemoto]

On Mon, 5 May 2008 00:08:04 +0200, tsbogend@alpha.franken.de (Thomas Bogendoerfer) wrote:
> > 	while (!kstack_end((void *)(unsigned long)sp)) {
> > 
> > will make this part sparse-free, though it seems a bit ugly.
> 
> hmm, would leaving sp as unsigned long * and casting it for __get_user()
> make sparse happy ?

Well, not as expected...  Here is some warning patterns.

1.	unsigned long __user *sp = (unsigned long __user *)(reg29 & ~3);
	...
	while (!kstack_end(sp)) {
		if (__get_user(addr, sp++)) {

linux/arch/mips/kernel/traps.c:91:21: warning: incorrect type in argument 1 (different address spaces)
linux/arch/mips/kernel/traps.c:91:21:    expected void *addr
linux/arch/mips/kernel/traps.c:91:21:    got unsigned long [noderef] <asn:1>*sp

2.	unsigned long *sp = (unsigned long *)(reg29 & ~3);
	...
	while (!kstack_end(sp)) {
		if (__get_user(addr, (unsigned long __user *)sp++)) {

linux/arch/mips/kernel/traps.c:92:7: warning: cast adds address space to expression (<asn:1>)
linux/arch/mips/kernel/traps.c:92:7: warning: cast adds address space to expression (<asn:1>)
linux/arch/mips/kernel/traps.c:92:7: warning: cast adds address space to expression (<asn:1>)
linux/arch/mips/kernel/traps.c:92:7: warning: cast adds address space to expression (<asn:1>)
linux/arch/mips/kernel/traps.c:92:7: warning: cast adds address space to expression (<asn:1>)
linux/arch/mips/kernel/traps.c:92:7: warning: cast adds address space to expression (<asn:1>)
linux/arch/mips/kernel/traps.c:92:7: warning: cast adds address space to expression (<asn:1>)
linux/arch/mips/kernel/traps.c:92:7: warning: cast adds address space to expression (<asn:1>)
linux/arch/mips/kernel/traps.c:92:7: warning: cast adds address space to expression (<asn:1>)

3.	unsigned long __user *sp = (unsigned long __user *)(reg29 & ~3);
	...
	while (!kstack_end((void *)(unsigned long)sp)) {
		if (__get_user(addr, sp++)) {

No warnings.

4.	unsigned long *sp = (unsigned long *)(reg29 & ~3);
	...
	while (!kstack_end(sp)) {
		unsigned long __user *p = (unsigned long __user *)sp++;
		if (__get_user(addr, p)) {

linux/arch/mips/kernel/traps.c:92:30: warning: cast adds address space to expression (<asn:1>)

4.	unsigned long *sp = (unsigned long *)(reg29 & ~3);
	...
	while (!kstack_end(sp)) {
		unsigned long __user *p =
			(unsigned long __user *)(unsigned long)sp++;
		if (__get_user(addr, p)) {

No warnings.


I think the "cast adds address space to expression" sparse warning is
not worth to handle so seriously.  So I'm OK with any of (2) to (4).

---
Atsushi Nemoto