i386 doublefault handler is broken with CONFIG_DEBUG_SPINLOCK

i386 doublefault handler is broken with CONFIG_DEBUG_SPINLOCK

[Chuck Ebbert]

The new percpu code has apparently broken the doublefault handler
when CONFIG_DEBUG_SPINLOCK is set. Doublefault is handled by
a hardware task, making the check

        SPIN_BUG_ON(lock->owner == current, lock, "recursion");

fault because it uses the FS register to access the percpu data
for current, and that register is zero in the new TSS. (The trace
I saw was on 2.6.20 where it was GS, but it looks like this will
still happen with FS on 2.6.22.)

Initializing FS in the doublefault_tss should fix it.

Signed-off-by: Chuck Ebbert <cebbert@redhat.com>

---

 NOTE: not even compile tested.

 arch/i386/kernel/doublefault.c |    1 +
 1 file changed, 1 insertion(+)

--- linux-2.6.22.noarch.orig/arch/i386/kernel/doublefault.c
+++ linux-2.6.22.noarch/arch/i386/kernel/doublefault.c
@@ -63,6 +63,7 @@ struct tss_struct doublefault_tss __cach
 		.cs		= __KERNEL_CS,
 		.ss		= __KERNEL_DS,
 		.ds		= __USER_DS,
+		.fs		= __KERNEL_PERCPU,
 
 		.__cr3		= __pa(swapper_pg_dir)
 	}

Re: i386 doublefault handler is broken with CONFIG_DEBUG_SPINLOCK

[Andi Kleen]

Chuck Ebbert <cebbert@redhat.com> writes:
> 
> Initializing FS in the doublefault_tss should fix it.
> 
> Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
> 
> ---
> 
>  NOTE: not even compile tested.

Can you please test it?

-Andi

Re: i386 doublefault handler is broken with CONFIG_DEBUG_SPINLOCK

[Chuck Ebbert]

On 08/09/2007 01:49 PM, Andi Kleen wrote:
> Chuck Ebbert <cebbert@redhat.com> writes:
>> Initializing FS in the doublefault_tss should fix it.
>>
>> Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
>>
>> ---
>>
>>  NOTE: not even compile tested.
> 
> Can you please test it?
> 

It compiles but I can't really test it further right now.

Re: i386 doublefault handler is broken with CONFIG_DEBUG_SPINLOCK

[Jeremy Fitzhardinge]

Chuck Ebbert wrote:
> On 08/09/2007 01:49 PM, Andi Kleen wrote:
>   
>> Chuck Ebbert <cebbert@redhat.com> writes:
>>     
>>> Initializing FS in the doublefault_tss should fix it.
>>>
>>> Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
>>>
>>> ---
>>>
>>>  NOTE: not even compile tested.
>>>       
>> Can you please test it?
>>
>>     
>
> It compiles but I can't really test it further right now.
>   

Looks correct in principle.

    J

Re: i386 doublefault handler is broken with CONFIG_DEBUG_SPINLOCK

[Andi Kleen]

On Thu, Aug 09, 2007 at 02:40:27PM -0400, Chuck Ebbert wrote:
> On 08/09/2007 01:49 PM, Andi Kleen wrote:
> > Chuck Ebbert <cebbert@redhat.com> writes:
> >> Initializing FS in the doublefault_tss should fix it.
> >>
> >> Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
> >>
> >> ---
> >>
> >>  NOTE: not even compile tested.
> > 
> > Can you please test it?
> > 
> 
> It compiles but I can't really test it further right now.

I tested it. Even on a box without spin lock debugging I get a hard
hang after

double fault, gdt at c1404000 [255 bytes]

even though it should have printed the registers.
So it looks like there is more broken in the DF handler than just
this.

-Andi

Re: i386 doublefault handler is broken with CONFIG_DEBUG_SPINLOCK

[Chuck Ebbert]

On 08/09/2007 07:16 PM, Andi Kleen wrote:
> 
> I tested it. Even on a box without spin lock debugging I get a hard
> hang after
> 
> double fault, gdt at c1404000 [255 bytes]
> 
> even though it should have printed the registers.
> So it looks like there is more broken in the DF handler than just
> this.

Looks like it just fails the ptr_ok() test:

#define ptr_ok(x) ((x) > PAGE_OFFSET && (x) < PAGE_OFFSET + 0x1000000)

page_offset  c0000000
            + 1000000

           < c1404000

What should that be changed to, or is there some easier way to test that?

Re: i386 doublefault handler is broken with CONFIG_DEBUG_SPINLOCK

[Andi Kleen]

On Thu, Aug 09, 2007 at 07:35:39PM -0400, Chuck Ebbert wrote:
> On 08/09/2007 07:16 PM, Andi Kleen wrote:
> > 
> > I tested it. Even on a box without spin lock debugging I get a hard
> > hang after
> > 
> > double fault, gdt at c1404000 [255 bytes]
> > 
> > even though it should have printed the registers.
> > So it looks like there is more broken in the DF handler than just
> > this.
> 
> Looks like it just fails the ptr_ok() test:
> 
> #define ptr_ok(x) ((x) > PAGE_OFFSET && (x) < PAGE_OFFSET + 0x1000000)
> 
> page_offset  c0000000
>             + 1000000
> 
>            < c1404000
> 
> What should that be changed to, or is there some easier way to test that?

This is the patch i came up with in the end. Passes testing.
I also fixed some more minor things.


Fix double fault handler

From: Chuck Ebbert <cebbert@redhat.com>

The new percpu code has apparently broken the doublefault handler
when CONFIG_DEBUG_SPINLOCK is set. Doublefault is handled by
a hardware task, making the check

        SPIN_BUG_ON(lock->owner == current, lock, "recursion");

fault because it uses the FS register to access the percpu data
for current, and that register is zero in the new TSS. (The trace
I saw was on 2.6.20 where it was GS, but it looks like this will
still happen with FS on 2.6.22.)

Initializing FS in the doublefault_tss should fix it.

AK: Also fix broken ptr_ok() and turn printks into KERN_EMERG
AK: And add a PANIC prefix to make clear the system will hang
AK: (e.g. x86-64 will recover) 

Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
Signed-off-by: Andi Kleen <ak@suse.de>

 arch/i386/kernel/doublefault.c |    1 +
 1 file changed, 1 insertion(+)

Index: linux/arch/i386/kernel/doublefault.c
===================================================================
--- linux.orig/arch/i386/kernel/doublefault.c
+++ linux/arch/i386/kernel/doublefault.c
@@ -13,7 +13,7 @@
 static unsigned long doublefault_stack[DOUBLEFAULT_STACKSIZE];
 #define STACK_START (unsigned long)(doublefault_stack+DOUBLEFAULT_STACKSIZE)
 
-#define ptr_ok(x) ((x) > PAGE_OFFSET && (x) < PAGE_OFFSET + 0x1000000)
+#define ptr_ok(x) ((x) > PAGE_OFFSET && (x) < PAGE_OFFSET + MAXMEM)
 
 static void doublefault_fn(void)
 {
@@ -23,7 +23,7 @@ static void doublefault_fn(void)
 	store_gdt(&gdt_desc);
 	gdt = gdt_desc.address;
 
-	printk("double fault, gdt at %08lx [%d bytes]\n", gdt, gdt_desc.size);
+	printk(KERN_EMERG "PANIC: double fault, gdt at %08lx [%d bytes]\n", gdt, gdt_desc.size);
 
 	if (ptr_ok(gdt)) {
 		gdt += GDT_ENTRY_TSS << 3;
@@ -35,11 +35,11 @@ static void doublefault_fn(void)
 		if (ptr_ok(tss)) {
 			struct i386_hw_tss *t = (struct i386_hw_tss *)tss;
 
-			printk("eip = %08lx, esp = %08lx\n", t->eip, t->esp);
+			printk(KERN_EMERG "eip = %08lx, esp = %08lx\n", t->eip, t->esp);
 
-			printk("eax = %08lx, ebx = %08lx, ecx = %08lx, edx = %08lx\n",
+			printk(KERN_EMERG "eax = %08lx, ebx = %08lx, ecx = %08lx, edx = %08lx\n",
 				t->eax, t->ebx, t->ecx, t->edx);
-			printk("esi = %08lx, edi = %08lx\n",
+			printk(KERN_EMERG "esi = %08lx, edi = %08lx\n",
 				t->esi, t->edi);
 		}
 	}
@@ -63,6 +63,7 @@ struct tss_struct doublefault_tss __cach
 		.cs		= __KERNEL_CS,
 		.ss		= __KERNEL_DS,
 		.ds		= __USER_DS,
+		.fs		= __KERNEL_PERCPU,
 
 		.__cr3		= __pa(swapper_pg_dir)
 	}

Re: i386 doublefault handler is broken with CONFIG_DEBUG_SPINLOCK

[Chuck Ebbert]

On 08/10/2007 04:23 AM, Andi Kleen wrote:
> 
> This is the patch i came up with in the end. Passes testing.
> I also fixed some more minor things.

Looks good to me. Missed adding KERN_EMERG for the printk()
of the TSS address, though.

> 
> Fix double fault handler
> 
> From: Chuck Ebbert <cebbert@redhat.com>
> 
> The new percpu code has apparently broken the doublefault handler
> when CONFIG_DEBUG_SPINLOCK is set. Doublefault is handled by
> a hardware task, making the check
> 
>         SPIN_BUG_ON(lock->owner == current, lock, "recursion");
> 
> fault because it uses the FS register to access the percpu data
> for current, and that register is zero in the new TSS. (The trace
> I saw was on 2.6.20 where it was GS, but it looks like this will
> still happen with FS on 2.6.22.)
> 
> Initializing FS in the doublefault_tss should fix it.
> 
> AK: Also fix broken ptr_ok() and turn printks into KERN_EMERG
> AK: And add a PANIC prefix to make clear the system will hang
> AK: (e.g. x86-64 will recover) 
> 
> Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
>  arch/i386/kernel/doublefault.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: linux/arch/i386/kernel/doublefault.c
> ===================================================================
> --- linux.orig/arch/i386/kernel/doublefault.c
> +++ linux/arch/i386/kernel/doublefault.c
> @@ -13,7 +13,7 @@
>  static unsigned long doublefault_stack[DOUBLEFAULT_STACKSIZE];
>  #define STACK_START (unsigned long)(doublefault_stack+DOUBLEFAULT_STACKSIZE)
>  
> -#define ptr_ok(x) ((x) > PAGE_OFFSET && (x) < PAGE_OFFSET + 0x1000000)
> +#define ptr_ok(x) ((x) > PAGE_OFFSET && (x) < PAGE_OFFSET + MAXMEM)
>  
>  static void doublefault_fn(void)
>  {
> @@ -23,7 +23,7 @@ static void doublefault_fn(void)
>  	store_gdt(&gdt_desc);
>  	gdt = gdt_desc.address;
>  
> -	printk("double fault, gdt at %08lx [%d bytes]\n", gdt, gdt_desc.size);
> +	printk(KERN_EMERG "PANIC: double fault, gdt at %08lx [%d bytes]\n", gdt, gdt_desc.size);
>  
>  	if (ptr_ok(gdt)) {
>  		gdt += GDT_ENTRY_TSS << 3;
> @@ -35,11 +35,11 @@ static void doublefault_fn(void)
>  		if (ptr_ok(tss)) {
>  			struct i386_hw_tss *t = (struct i386_hw_tss *)tss;
>  
> -			printk("eip = %08lx, esp = %08lx\n", t->eip, t->esp);
> +			printk(KERN_EMERG "eip = %08lx, esp = %08lx\n", t->eip, t->esp);
>  
> -			printk("eax = %08lx, ebx = %08lx, ecx = %08lx, edx = %08lx\n",
> +			printk(KERN_EMERG "eax = %08lx, ebx = %08lx, ecx = %08lx, edx = %08lx\n",
>  				t->eax, t->ebx, t->ecx, t->edx);
> -			printk("esi = %08lx, edi = %08lx\n",
> +			printk(KERN_EMERG "esi = %08lx, edi = %08lx\n",
>  				t->esi, t->edi);
>  		}
>  	}
> @@ -63,6 +63,7 @@ struct tss_struct doublefault_tss __cach
>  		.cs		= __KERNEL_CS,
>  		.ss		= __KERNEL_DS,
>  		.ds		= __USER_DS,
> +		.fs		= __KERNEL_PERCPU,
>  
>  		.__cr3		= __pa(swapper_pg_dir)
>  	}
> 
> 
> 
>