Commit-ID: 43d79a250bdb10e30b08a79e906befc691c3f5ce
Gitweb: http://git.kernel.org/tip/43d79a250bdb10e30b08a79e906befc691c3f5ce
Author: H. Peter Anvin <[email protected]>
AuthorDate: Fri, 4 Feb 2011 16:14:11 -0800
Committer: H. Peter Anvin <[email protected]>
CommitDate: Fri, 4 Feb 2011 16:38:35 -0800
x86-32: Make sure the stack is set up before we use it
Since checkin ebba638ae723d8a8fc2f7abce5ec18b688b791d7 we call
verify_cpu even in 32-bit mode. Unfortunately, calling a function
means using the stack, and the stack pointer was not initialized in
the 32-bit setup code! This code initializes the stack pointer, and
simplifies the interface slightly since it is easier to rely on just a
pointer value rather than a descriptor; we need to have different
values for the segment register anyway.
This retains start_stack as a virtual address, even though a physical
address would be more convenient for 32 bits; the 64-bit code wants
the other way around...
Reported-by: Matthieu Castet <[email protected]>
LKML-Reference: <[email protected]>
Cc: Kees Cook <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/include/asm/smp.h | 5 +----
arch/x86/kernel/acpi/sleep.c | 2 +-
arch/x86/kernel/head_32.S | 30 +++++++++++++-----------------
arch/x86/kernel/head_64.S | 1 -
arch/x86/kernel/smpboot.c | 4 ++--
5 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 4c2f63c..1f46951 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -40,10 +40,7 @@ DECLARE_EARLY_PER_CPU(u16, x86_cpu_to_apicid);
DECLARE_EARLY_PER_CPU(u16, x86_bios_cpu_apicid);
/* Static state in head.S used to set up a CPU */
-extern struct {
- void *sp;
- unsigned short ss;
-} stack_start;
+extern unsigned long stack_start; /* Initial stack pointer address */
struct smp_ops {
void (*smp_prepare_boot_cpu)(void);
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 69fd72a..4d9ebba 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -100,7 +100,7 @@ int acpi_save_state_mem(void)
#else /* CONFIG_64BIT */
header->trampoline_segment = setup_trampoline() >> 4;
#ifdef CONFIG_SMP
- stack_start.sp = temp_stack + sizeof(temp_stack);
+ stack_start = (unsigned long)temp_stack + sizeof(temp_stack);
early_gdt_descr.address =
(unsigned long)get_cpu_gdt_table(smp_processor_id());
initial_gs = per_cpu_offset(smp_processor_id());
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index fc293dc..767d6c4 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -85,6 +85,8 @@ RESERVE_BRK(pagetables, INIT_MAP_SIZE)
*/
__HEAD
ENTRY(startup_32)
+ movl pa(stack_start),%ecx
+
/* test KEEP_SEGMENTS flag to see if the bootloader is asking
us to not reload segments */
testb $(1<<6), BP_loadflags(%esi)
@@ -99,7 +101,9 @@ ENTRY(startup_32)
movl %eax,%es
movl %eax,%fs
movl %eax,%gs
+ movl %eax,%ss
2:
+ leal -__PAGE_OFFSET(%ecx),%esp
/*
* Clear BSS first so that there are no surprises...
@@ -145,8 +149,6 @@ ENTRY(startup_32)
* _brk_end is set up to point to the first "safe" location.
* Mappings are created both at virtual address 0 (identity mapping)
* and PAGE_OFFSET for up to _end.
- *
- * Note that the stack is not yet set up!
*/
#ifdef CONFIG_X86_PAE
@@ -282,6 +284,9 @@ ENTRY(startup_32_smp)
movl %eax,%es
movl %eax,%fs
movl %eax,%gs
+ movl pa(stack_start),%ecx
+ movl %eax,%ss
+ leal -__PAGE_OFFSET(%ecx),%esp
#endif /* CONFIG_SMP */
default_entry:
@@ -347,8 +352,8 @@ default_entry:
movl %eax,%cr0 /* ..and set paging (PG) bit */
ljmp $__BOOT_CS,$1f /* Clear prefetch and normalize %eip */
1:
- /* Set up the stack pointer */
- lss stack_start,%esp
+ /* Shift the stack pointer to a virtual address */
+ addl $__PAGE_OFFSET, %esp
/*
* Initialize eflags. Some BIOS's leave bits like NT set. This would
@@ -360,9 +365,7 @@ default_entry:
#ifdef CONFIG_SMP
cmpb $0, ready
- jz 1f /* Initial CPU cleans BSS */
- jmp checkCPUtype
-1:
+ jnz checkCPUtype
#endif /* CONFIG_SMP */
/*
@@ -470,14 +473,7 @@ is386: movl $2,%ecx # set MP
cld # gcc2 wants the direction flag cleared at all times
pushl $0 # fake return address for unwinder
-#ifdef CONFIG_SMP
- movb ready, %cl
movb $1, ready
- cmpb $0,%cl # the first CPU calls start_kernel
- je 1f
- movl (stack_start), %esp
-1:
-#endif /* CONFIG_SMP */
jmp *(initial_code)
/*
@@ -670,15 +666,15 @@ ENTRY(initial_page_table)
#endif
.data
+.balign 4
ENTRY(stack_start)
.long init_thread_union+THREAD_SIZE
- .long __BOOT_DS
-
-ready: .byte 0
early_recursion_flag:
.long 0
+ready: .byte 0
+
int_msg:
.asciz "Unknown interrupt or fault at: %p %p %p\n"
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 239046b..1ee8493 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -264,7 +264,6 @@ ENTRY(secondary_startup_64)
ENTRY(stack_start)
.quad init_thread_union+THREAD_SIZE-8
- .word 0
__FINITDATA
bad_address:
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 0cbe8c0..03273b6 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -638,7 +638,7 @@ wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
* target processor state.
*/
startup_ipi_hook(phys_apicid, (unsigned long) start_secondary,
- (unsigned long)stack_start.sp);
+ stack_start);
/*
* Run STARTUP IPI loop.
@@ -785,7 +785,7 @@ do_rest:
#endif
early_gdt_descr.address = (unsigned long)get_cpu_gdt_table(cpu);
initial_code = (unsigned long)start_secondary;
- stack_start.sp = (void *) c_idle.idle->thread.sp;
+ stack_start = c_idle.idle->thread.sp;
/* start_ip had better be page-aligned! */
start_ip = setup_trampoline();
Hi Peter,
This works for me doing CPU hotplugging on ia32, but on x86_64 it hangs.
(Both boot, though.)
Thanks,
-Kees
On Sat, Feb 05, 2011 at 12:45:54AM +0000, tip-bot for H. Peter Anvin wrote:
> Commit-ID: 43d79a250bdb10e30b08a79e906befc691c3f5ce
> Gitweb: http://git.kernel.org/tip/43d79a250bdb10e30b08a79e906befc691c3f5ce
> Author: H. Peter Anvin <[email protected]>
> AuthorDate: Fri, 4 Feb 2011 16:14:11 -0800
> Committer: H. Peter Anvin <[email protected]>
> CommitDate: Fri, 4 Feb 2011 16:38:35 -0800
>
> x86-32: Make sure the stack is set up before we use it
>
> Since checkin ebba638ae723d8a8fc2f7abce5ec18b688b791d7 we call
> verify_cpu even in 32-bit mode. Unfortunately, calling a function
> means using the stack, and the stack pointer was not initialized in
> the 32-bit setup code! This code initializes the stack pointer, and
> simplifies the interface slightly since it is easier to rely on just a
> pointer value rather than a descriptor; we need to have different
> values for the segment register anyway.
>
> This retains start_stack as a virtual address, even though a physical
> address would be more convenient for 32 bits; the 64-bit code wants
> the other way around...
>
> Reported-by: Matthieu Castet <[email protected]>
> LKML-Reference: <[email protected]>
> Cc: Kees Cook <[email protected]>
> Signed-off-by: H. Peter Anvin <[email protected]>
> ---
> arch/x86/include/asm/smp.h | 5 +----
> arch/x86/kernel/acpi/sleep.c | 2 +-
> arch/x86/kernel/head_32.S | 30 +++++++++++++-----------------
> arch/x86/kernel/head_64.S | 1 -
> arch/x86/kernel/smpboot.c | 4 ++--
> 5 files changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index 4c2f63c..1f46951 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -40,10 +40,7 @@ DECLARE_EARLY_PER_CPU(u16, x86_cpu_to_apicid);
> DECLARE_EARLY_PER_CPU(u16, x86_bios_cpu_apicid);
>
> /* Static state in head.S used to set up a CPU */
> -extern struct {
> - void *sp;
> - unsigned short ss;
> -} stack_start;
> +extern unsigned long stack_start; /* Initial stack pointer address */
>
> struct smp_ops {
> void (*smp_prepare_boot_cpu)(void);
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 69fd72a..4d9ebba 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -100,7 +100,7 @@ int acpi_save_state_mem(void)
> #else /* CONFIG_64BIT */
> header->trampoline_segment = setup_trampoline() >> 4;
> #ifdef CONFIG_SMP
> - stack_start.sp = temp_stack + sizeof(temp_stack);
> + stack_start = (unsigned long)temp_stack + sizeof(temp_stack);
> early_gdt_descr.address =
> (unsigned long)get_cpu_gdt_table(smp_processor_id());
> initial_gs = per_cpu_offset(smp_processor_id());
> diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
> index fc293dc..767d6c4 100644
> --- a/arch/x86/kernel/head_32.S
> +++ b/arch/x86/kernel/head_32.S
> @@ -85,6 +85,8 @@ RESERVE_BRK(pagetables, INIT_MAP_SIZE)
> */
> __HEAD
> ENTRY(startup_32)
> + movl pa(stack_start),%ecx
> +
> /* test KEEP_SEGMENTS flag to see if the bootloader is asking
> us to not reload segments */
> testb $(1<<6), BP_loadflags(%esi)
> @@ -99,7 +101,9 @@ ENTRY(startup_32)
> movl %eax,%es
> movl %eax,%fs
> movl %eax,%gs
> + movl %eax,%ss
> 2:
> + leal -__PAGE_OFFSET(%ecx),%esp
>
> /*
> * Clear BSS first so that there are no surprises...
> @@ -145,8 +149,6 @@ ENTRY(startup_32)
> * _brk_end is set up to point to the first "safe" location.
> * Mappings are created both at virtual address 0 (identity mapping)
> * and PAGE_OFFSET for up to _end.
> - *
> - * Note that the stack is not yet set up!
> */
> #ifdef CONFIG_X86_PAE
>
> @@ -282,6 +284,9 @@ ENTRY(startup_32_smp)
> movl %eax,%es
> movl %eax,%fs
> movl %eax,%gs
> + movl pa(stack_start),%ecx
> + movl %eax,%ss
> + leal -__PAGE_OFFSET(%ecx),%esp
> #endif /* CONFIG_SMP */
> default_entry:
>
> @@ -347,8 +352,8 @@ default_entry:
> movl %eax,%cr0 /* ..and set paging (PG) bit */
> ljmp $__BOOT_CS,$1f /* Clear prefetch and normalize %eip */
> 1:
> - /* Set up the stack pointer */
> - lss stack_start,%esp
> + /* Shift the stack pointer to a virtual address */
> + addl $__PAGE_OFFSET, %esp
>
> /*
> * Initialize eflags. Some BIOS's leave bits like NT set. This would
> @@ -360,9 +365,7 @@ default_entry:
>
> #ifdef CONFIG_SMP
> cmpb $0, ready
> - jz 1f /* Initial CPU cleans BSS */
> - jmp checkCPUtype
> -1:
> + jnz checkCPUtype
> #endif /* CONFIG_SMP */
>
> /*
> @@ -470,14 +473,7 @@ is386: movl $2,%ecx # set MP
>
> cld # gcc2 wants the direction flag cleared at all times
> pushl $0 # fake return address for unwinder
> -#ifdef CONFIG_SMP
> - movb ready, %cl
> movb $1, ready
> - cmpb $0,%cl # the first CPU calls start_kernel
> - je 1f
> - movl (stack_start), %esp
> -1:
> -#endif /* CONFIG_SMP */
> jmp *(initial_code)
>
> /*
> @@ -670,15 +666,15 @@ ENTRY(initial_page_table)
> #endif
>
> .data
> +.balign 4
> ENTRY(stack_start)
> .long init_thread_union+THREAD_SIZE
> - .long __BOOT_DS
> -
> -ready: .byte 0
>
> early_recursion_flag:
> .long 0
>
> +ready: .byte 0
> +
> int_msg:
> .asciz "Unknown interrupt or fault at: %p %p %p\n"
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 239046b..1ee8493 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -264,7 +264,6 @@ ENTRY(secondary_startup_64)
>
> ENTRY(stack_start)
> .quad init_thread_union+THREAD_SIZE-8
> - .word 0
> __FINITDATA
>
> bad_address:
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 0cbe8c0..03273b6 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -638,7 +638,7 @@ wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
> * target processor state.
> */
> startup_ipi_hook(phys_apicid, (unsigned long) start_secondary,
> - (unsigned long)stack_start.sp);
> + stack_start);
>
> /*
> * Run STARTUP IPI loop.
> @@ -785,7 +785,7 @@ do_rest:
> #endif
> early_gdt_descr.address = (unsigned long)get_cpu_gdt_table(cpu);
> initial_code = (unsigned long)start_secondary;
> - stack_start.sp = (void *) c_idle.idle->thread.sp;
> + stack_start = c_idle.idle->thread.sp;
>
> /* start_ip had better be page-aligned! */
> start_ip = setup_trampoline();
--
Kees Cook
Ubuntu Security Team
On 02/04/2011 06:19 PM, Kees Cook wrote:
> Hi Peter,
>
> This works for me doing CPU hotplugging on ia32, but on x86_64 it hangs.
> (Both boot, though.)
>
> Thanks,
>
> -Kees
I can't reproduce the failure here, and it's hard to see how this patch
would make a significant change on 64 bits. Could you try reverting
*only* the head_64.S hunk and try it on your platform again?
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
On Fri, Feb 04, 2011 at 08:37:37PM -0800, H. Peter Anvin wrote:
> On 02/04/2011 06:19 PM, Kees Cook wrote:
> > This works for me doing CPU hotplugging on ia32, but on x86_64 it hangs.
> > (Both boot, though.)
>
> I can't reproduce the failure here, and it's hard to see how this patch
> would make a significant change on 64 bits. Could you try reverting
> *only* the head_64.S hunk and try it on your platform again?
With the head_64.S hunk dropped, it runs fine. My highly fine-tuned
test-case is:
while :; do ls -la / -R >/dev/null 2>/dev/null; done &
while :; do echo 0 > /sys/devices/system/cpu/cpu1/online ; echo 1 > /sys/devices/system/cpu/cpu1/online ; done &
Looks good from this end. Thanks!
-Kees
--
Kees Cook
Ubuntu Security Team
On 02/04/2011 09:37 PM, Kees Cook wrote:
> On Fri, Feb 04, 2011 at 08:37:37PM -0800, H. Peter Anvin wrote:
>> On 02/04/2011 06:19 PM, Kees Cook wrote:
>>> This works for me doing CPU hotplugging on ia32, but on x86_64 it hangs.
>>> (Both boot, though.)
>>
>> I can't reproduce the failure here, and it's hard to see how this patch
>> would make a significant change on 64 bits. Could you try reverting
>> *only* the head_64.S hunk and try it on your platform again?
>
> With the head_64.S hunk dropped, it runs fine. My highly fine-tuned
> test-case is:
>
> while :; do ls -la / -R >/dev/null 2>/dev/null; done &
> while :; do echo 0 > /sys/devices/system/cpu/cpu1/online ; echo 1 > /sys/devices/system/cpu/cpu1/online ; done &
>
> Looks good from this end. Thanks!
>
OK, that bothers me, because it would seem that something is accessing
data it shouldn't, but ok, it fixes it for now. I'll update it in -tip.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.