sp0, ss1, and sp1 docs and minor fixes
cover sp0, ss1, and sp1 docs and minor fixes
This documents things, clarifies the code, and fixes an apparently inconsequential bug that caused the init sp0 to be wrong.
(My earlier 32-bit bug was caused because I thought that the code I'm fixing was correct. Whoops.)
Andy Lutomirski (3):
x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro
x86: Unify and fix init sp0
x86_32: Document our abuse of ss1 and sp1
arch/x86/include/asm/processor.h | 31 +++++++++++++++++++++++++------ arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ arch/x86/kernel/process.c | 2 +- 3 files changed, 56 insertions(+), 7 deletions(-)
Signature · 1 line
2.3.0
PATCH 1/3 x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro
x86_32, unlike x86_64, pads the top of the kernel stack. Document this padding and give it a name.
This should make no change whatsoever to the compiled kernel image. It also doesn't fix any of the current bugs in this area.
--- arch/x86/include/asm/processor.h | 3 ++- arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)
arch/x86/include/asm/processor.h +2 -1
@@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define task_pt_regs (task) \ ({ \ struct pt_regs *__regs__; \ - __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \ + __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \ + TOP_OF_KERNEL_STACK_PADDING); \ __regs__ - 1; \ })
arch/x86/include/asm/thread_info.h +30 -0
@@ -49,6 +49,36 @@ struct thread_info {#define init_thread_info (init_thread_union.thread_info)#define init_stack (init_thread_union.stack) +#ifdef CONFIG_X86_32 + +/* +* TOP_OF_KERNEL_STACK_PADDING is a number of unused bytes that we +* reserve at the top of the kernel stack. We do it because of a nasty +* 32-bit corner case. On x86_32, the hardware stack frame is +* variable-length. Except for vm86 mode, struct pt_regs assumes a +* maximum-length frame. If we enter from CPL 0, the top 8 bytes of +* pt_regs don't actually exist. Ordinarily this doesn't matter, but it +* does in at least one case: +* +* If we take an NMI early enough in sysenter, the we can end up with +* pt_regs that extends above sp0. On the way out, in the espfix code, +* we can read the saved SS value, but that value will be above sp0. +* Without this offset, that can result in a page fault. (We are +* careful that, in this case, the value we read doesn't matter.) +* +* In vm86 mode, the hardware frame is much longer still, but we neither +* access the extra members from NMI context, nor do we write such a +* frame at sp0 at all. +*/ +#define TOP_OF_KERNEL_STACK_PADDING 8 + +#else + +/* Phew, x86_64 has a fixed-length stack frame! */ +#define TOP_OF_KERNEL_STACK_PADDING 0 + +#endif +#else /* !__ASSEMBLY__ */ #include <asm/asm-offsets.h>
Signature · 1 line
2.3.0
Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro
Quoted 17 lines · Andy Lutomirski
x86_32, unlikex86_64, pads the top of the kernel stack. Document this padding and give it a name.This should make no change whatsoever to the compiled kernel image. It also doesn't fix any of the current bugs in this area.
Signed-off-by Andy Lutomirski <[email protected]>--- arch/x86/include/asm/processor.h | 3 ++- arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)arch/x86/include/asm/processor.h +0
-0
arch/x86/include/asm/processor.h extern unsigned long thread_saved_pc(struct task_struct *tsk); · line 855
Quoted 8 lines
arch/x86/include/asm/processor.h +2
-1@@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define task_pt_regs (task) \ ({ \ struct pt_regs *__regs__; \ - __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \ + __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \ + TOP_OF_KERNEL_STACK_PADDING); \ __regs__ - 1; \ })
The "magic 8" also hides here:
/*
* Push current_thread_info()->sysenter_return to the stack.
* A tiny bit of offset fixup is necessary - 4*4 means the 4 words
* pushed above; +8 corresponds to copy_thread's esp0 setting. */
pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+8+4*4)(%esp)
^^^
CFI_REL_OFFSET eip, 0
It may make sense to mention TOP_OF_KERNEL_STACK_PADDING here (there are no useful comments in copy_thread() anymore):
movl PT_EFLAGS(%esp), %eax # mix EFLAGS, SS and CS
# Warning: PT_OLDSS(%esp) contains the wrong/random values if we # are returning to the kernel.
# See comments in process.c:copy_thread() for details.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ maybe s/comments in process.c:copy_thread()/TOP_OF_KERNEL_STACK_PADDING/ ?
movb PT_OLDSS(%esp), %ah
movb PT_CS(%esp), %al
andl $(X86_EFLAGS_VM | (SEGMENT_TI_MASK << 8) | SEGMENT_RPL_MASK), %eax
cmpl $((SEGMENT_LDT << 8) | USER_RPL), %eax
CFI_REMEMBER_STATERe: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro
Quoted 29 lines · Denys Vlasenko
Quoted 17 lines · Andy Lutomirski
x86_32, unlikex86_64, pads the top of the kernel stack. Document this padding and give it a name.This should make no change whatsoever to the compiled kernel image. It also doesn't fix any of the current bugs in this area.
Signed-off-by Andy Lutomirski <[email protected]>--- arch/x86/include/asm/processor.h | 3 ++- arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)arch/x86/include/asm/processor.h +0
-0arch/x86/include/asm/processor.h extern unsigned long thread_saved_pc(struct task_struct *tsk); · line 855
Quoted 8 lines
arch/x86/include/asm/processor.h +2
-1@@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define task_pt_regs (task) \ ({ \ struct pt_regs *__regs__; \ - __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \ + __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \ + TOP_OF_KERNEL_STACK_PADDING); \ __regs__ - 1; \ })The "magic 8" also hides here:
Quoted 7 lines
/*
* Push current_thread_info()->sysenter_return to the stack.
* A tiny bit of offset fixup is necessary - 4*4 means the 4 words
* pushed above; +8 corresponds to copy_thread's esp0 setting.
*/
pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+8+4*4)(%esp)
^^^I'll just put TOP_OF_KERNEL_STACK_PADDING there, too.
--Andy
Quoted 18 lines
CFI_REL_OFFSETeip, 0It may make sense to mention
TOP_OF_KERNEL_STACK_PADDINGhere (there are no useful comments incopy_thread()anymore):movl
PT_EFLAGS(%esp), %eax # mix EFLAGS, SS and CS
# Warning:PT_OLDSS(%esp)contains the wrong/random values if we # are returning to the kernel.
# See comments in process.c:copy_thread() for details.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ maybe s/comments in process.c:copy_thread()/TOP_OF_KERNEL_STACK_PADDING/ ?movb PT_OLDSS(%esp), %ah movb PT_CS(%esp), %al andl $(X86_EFLAGS_VM | (SEGMENT_TI_MASK << 8) | SEGMENT_RPL_MASK), %eax cmpl $((SEGMENT_LDT << 8) | USER_RPL), %eax CFI_REMEMBER_STATE
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro
In reply to Denys VlasenkoQuoted 13 lines · Denys Vlasenko
Quoted 12 lines · Andy Lutomirski
x86_32, unlikex86_64, pads the top of the kernel stack. Document this padding and give it a name.This should make no change whatsoever to the compiled kernel image. It also doesn't fix any of the current bugs in this area.
Signed-off-by Andy Lutomirski <[email protected]>--- arch/x86/include/asm/processor.h | 3 ++- arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)
Quoted 13 lines
arch/x86/include/asm/processor.h extern unsigned long thread_saved_pc(struct task_struct *tsk); · line 855
Quoted 13 lines
arch/x86/include/asm/processor.h +2
-1@@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define task_pt_regs (task) \ ({ \ struct pt_regs *__regs__; \ - __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \ + __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \ + TOP_OF_KERNEL_STACK_PADDING); \ __regs__ - 1; \ })
While we are at it, this macro can be simplified further. It uses KSTK_TOP, and it's the only use of that macro. In turn, KSTK_TOP uses THREAD_SIZE_LONGS, and it is the only use of that macro too!
(And to nitpick, this macro is whitespace-damaged. Seven space indent).
I'll send a patch based on your current git.
Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro
In reply to Andy LutomirskiQuoted 36 lines · Andy Lutomirski
x86_32, unlikex86_64, pads the top of the kernel stack. Document this padding and give it a name.This should make no change whatsoever to the compiled kernel image. It also doesn't fix any of the current bugs in this area.
Signed-off-by Andy Lutomirski <[email protected]>--- arch/x86/include/asm/processor.h | 3 ++- arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)arch/x86/include/asm/processor.h +2
-1@@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define task_pt_regs (task) \ ({ \ struct pt_regs *__regs__; \ - __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \ + __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \ + TOP_OF_KERNEL_STACK_PADDING); \ __regs__ - 1; \ })arch/x86/include/asm/thread_info.h +2
-0@@ -49,6 +49,36 @@ struct thread_info {#define init_thread_info (init_thread_union.thread_info)#define init_stack (init_thread_union.stack) +#ifdef CONFIG_X86_32 +
arch/x86/include/asm/thread_info.h struct thread_info { · line 63
Quoted 10 lines
arch/x86/include/asm/thread_info.h +10
-0+/* +* TOP_OF_KERNEL_STACK_PADDING is a number of unused bytes that we +* reserve at the top of the kernel stack. We do it because of a nasty +* 32-bit corner case. On x86_32, the hardware stack frame is +* variable-length. Except for vm86 mode, struct pt_regs assumes a +* maximum-length frame. If we enter from CPL 0, the top 8 bytes of +* pt_regs don't actually exist. Ordinarily this doesn't matter, but it +* does in at least one case: +* +* If we take an NMI early enough in sysenter, the we can end up with
s/the/then
I fixed this up in the commit.
arch/x86/include/asm/thread_info.h struct thread_info { · line 72
Quoted 9 lines
arch/x86/include/asm/thread_info.h +9
-0+* pt_regs that extends above sp0. On the way out, in the espfix code, +* we can read the saved SS value, but that value will be above sp0. +* Without this offset, that can result in a page fault. (We are +* careful that, in this case, the value we read doesn't matter.) +* +* In vm86 mode, the hardware frame is much longer still, but we neither +* access the extra members from NMI context, nor do we write such a +* frame at sp0 at all. +*/
Thanks,
Ingo
tip-bot for Andy Lutomirski
merge bot[tip:x86/asm] x86/asm/entry: Create and use a ' TOP_OF_KERNEL_STACK_PADDING' macro
In reply to Andy Lutomirskix86/asm/entry: Create and use a 'TOP_OF_KERNEL_STACK_PADDING' macro
Commit-ID: 3bfef48e27863ebbd47d68a3f4a081d2cb5286aa Gitweb: http://git.kernel.org/tip/3bfef48e27863ebbd47d68a3f4a081d2cb5286aa Author: Andy Lutomirski <[email protected]> AuthorDate: Tue, 10 Mar 2015 11:05:58 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Mon, 16 Mar 2015 11:05:30 +0100
x86/asm/entry: Create and use a 'TOP_OF_KERNEL_STACK_PADDING' macro
x86_32, unlike x86_64, pads the top of the kernel stack, because the hardware stack frame formats are variable in size.
Document this padding and give it a name.
This should make no change whatsoever to the compiled kernel image. It also doesn't fix any of the current bugs in this area.
--- arch/x86/include/asm/processor.h | 3 ++- arch/x86/include/asm/thread_info.h | 27 +++++++++++++++++++++++++++ arch/x86/kernel/entry_32.S | 2 +- 3 files changed, 30 insertions(+), 2 deletions(-)
arch/x86/include/asm/processor.h +2 -1
@@ -848,7 +848,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define task_pt_regs (task) \ ({ \ struct pt_regs *__regs__; \ - __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \ + __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \ + TOP_OF_KERNEL_STACK_PADDING); \ __regs__ - 1; \ })
arch/x86/include/asm/thread_info.h +27 -0
@@ -13,6 +13,33 @@#include <asm/types.h> /* +* TOP_OF_KERNEL_STACK_PADDING is a number of unused bytes that we +* reserve at the top of the kernel stack. We do it because of a nasty +* 32-bit corner case. On x86_32, the hardware stack frame is +* variable-length. Except for vm86 mode, struct pt_regs assumes a +* maximum-length frame. If we enter from CPL 0, the top 8 bytes of +* pt_regs don't actually exist. Ordinarily this doesn't matter, but it +* does in at least one case: +* +* If we take an NMI early enough in SYSENTER, then we can end up with +* pt_regs that extends above sp0. On the way out, in the espfix code, +* we can read the saved SS value, but that value will be above sp0. +* Without this offset, that can result in a page fault. (We are +* careful that, in this case, the value we read doesn't matter.) +* +* In vm86 mode, the hardware frame is much longer still, but we neither +* access the extra members from NMI context, nor do we write such a +* frame at sp0 at all. +* +* x86_64 has a fixed-length stack frame. +*/ +#ifdef CONFIG_X86_32 +# define TOP_OF_KERNEL_STACK_PADDING 8 +#else +# define TOP_OF_KERNEL_STACK_PADDING 0 +#endif + +/* * low level task data that entry.S needs immediate access to * - this struct should fit entirely inside of one cache line * - this struct shares the supervisor stack pages
arch/x86/kernel/entry_32.S +1 -1
@@ -398,7 +398,7 @@ sysenter_past_esp: * A tiny bit of offset fixup is necessary - 4*4 means the 4 words * pushed above; +8 corresponds to copy_thread's esp0 setting. */ - pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+8+4*4)(%esp) + pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+TOP_OF_KERNEL_STACK_PADDING+4*4)(%esp) CFI_REL_OFFSET eip, 0 pushl_cfi %eax
tip-bot for Andy Lutomirski
merge bot[tip:x86/asm] x86/asm/entry: Create and use a ' TOP_OF_KERNEL_STACK_PADDING' macro
In reply to Andy Lutomirskix86/asm/entry: Create and use a 'TOP_OF_KERNEL_STACK_PADDING' macro
Commit-ID: 3ee4298f440c81638cbb5ec06f2497fb7a9a9eb4 Gitweb: http://git.kernel.org/tip/3ee4298f440c81638cbb5ec06f2497fb7a9a9eb4 Author: Andy Lutomirski <[email protected]> AuthorDate: Tue, 10 Mar 2015 11:05:58 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 17 Mar 2015 09:25:26 +0100
x86/asm/entry: Create and use a 'TOP_OF_KERNEL_STACK_PADDING' macro
x86_32, unlike x86_64, pads the top of the kernel stack, because the hardware stack frame formats are variable in size.
Document this padding and give it a name.
This should make no change whatsoever to the compiled kernel image. It also doesn't fix any of the current bugs in this area.
--- arch/x86/include/asm/processor.h | 3 ++- arch/x86/include/asm/thread_info.h | 27 +++++++++++++++++++++++++++ arch/x86/kernel/entry_32.S | 2 +- 3 files changed, 30 insertions(+), 2 deletions(-)
arch/x86/include/asm/processor.h +2 -1
@@ -849,7 +849,8 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define task_pt_regs (task) \ ({ \ struct pt_regs *__regs__; \ - __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task))-8); \ + __regs__ = (struct pt_regs *)(KSTK_TOP(task_stack_page(task)) - \ + TOP_OF_KERNEL_STACK_PADDING); \ __regs__ - 1; \ })
arch/x86/include/asm/thread_info.h +27 -0
@@ -13,6 +13,33 @@#include <asm/types.h> /* +* TOP_OF_KERNEL_STACK_PADDING is a number of unused bytes that we +* reserve at the top of the kernel stack. We do it because of a nasty +* 32-bit corner case. On x86_32, the hardware stack frame is +* variable-length. Except for vm86 mode, struct pt_regs assumes a +* maximum-length frame. If we enter from CPL 0, the top 8 bytes of +* pt_regs don't actually exist. Ordinarily this doesn't matter, but it +* does in at least one case: +* +* If we take an NMI early enough in SYSENTER, then we can end up with +* pt_regs that extends above sp0. On the way out, in the espfix code, +* we can read the saved SS value, but that value will be above sp0. +* Without this offset, that can result in a page fault. (We are +* careful that, in this case, the value we read doesn't matter.) +* +* In vm86 mode, the hardware frame is much longer still, but we neither +* access the extra members from NMI context, nor do we write such a +* frame at sp0 at all. +* +* x86_64 has a fixed-length stack frame. +*/ +#ifdef CONFIG_X86_32 +# define TOP_OF_KERNEL_STACK_PADDING 8 +#else +# define TOP_OF_KERNEL_STACK_PADDING 0 +#endif + +/* * low level task data that entry.S needs immediate access to * - this struct should fit entirely inside of one cache line * - this struct shares the supervisor stack pages
arch/x86/kernel/entry_32.S +1 -1
@@ -398,7 +398,7 @@ sysenter_past_esp: * A tiny bit of offset fixup is necessary - 4*4 means the 4 words * pushed above; +8 corresponds to copy_thread's esp0 setting. */ - pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+8+4*4)(%esp) + pushl_cfi ((TI_sysenter_return)-THREAD_SIZE+TOP_OF_KERNEL_STACK_PADDING+4*4)(%esp) CFI_REL_OFFSET eip, 0 pushl_cfi %eax
PATCH 2/3 x86: Unify and fix init sp0
In reply to Andy Lutomirskix86_32 and x86_64 need slightly different sp0 values, and x86_32's was incorrect for init. (This never mattered -- the init thread never runs user code, so we never used sp0 for anything.)
Fix it and mostly unify them.
--- arch/x86/include/asm/processor.h | 7 +++++-- arch/x86/kernel/process.c | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-)
arch/x86/include/asm/processor.h +5 -2
@@ -811,6 +811,9 @@ static inline void spin_lock_prefetch(const void *x)prefetchw (x); } +#define TOP_OF_INIT_STACK ((unsigned long)&init_stack + sizeof(init_stack) - \ + TOP_OF_KERNEL_STACK_PADDING) +#ifdef CONFIG_X86_32 /* * User space process size: 3GB (default).
@@ -821,7 +824,7 @@ static inline void spin_lock_prefetch(const void *x)#define STACK_TOP_MAX STACK_TOP#define INIT_THREAD { \ - .sp0 = sizeof(init_stack) + (long)&init_stack, \ + .sp0 = TOP_OF_INIT_STACK, \ .vm86_info = NULL, \ .sysenter_cs = __KERNEL_CS, \ .io_bitmap_ptr = NULL, \
@@ -883,7 +886,7 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define STACK_TOP_MAX TASK_SIZE_MAX#define INIT_THREAD { \ - .sp0 = (unsigned long)&init_stack + sizeof(init_stack) \ + .sp0 = TOP_OF_INIT_STACK \ }/*
arch/x86/kernel/process.c +1 -1
@@ -39,7 +39,7 @@*/ __visible DEFINE_PER_CPU_SHARED_ALIGNED (struct tss_struct ,cpu_tss ) = { .x86_tss = { - .sp0 = (unsigned long )&init_stack +sizeof (init_stack), + .sp0 =TOP_OF_INIT_STACK , #ifdef CONFIG_X86_32 .ss0 = __KERNEL_DS, .ss1 = __KERNEL_CS,
Signature · 1 line
2.3.0
Re: [PATCH 2/3] x86: Unify and fix init sp0
Quoted 3 lines · Andy Lutomirski
x86_32andx86_64need slightly different sp0 values, and x86_32's was incorrect for init. (This never mattered -- the init thread never runs user code, so we never used sp0 for anything.)
Damn old x86 cruft - sp0 is the stack pointer for CPL0 in the TSS. Had to go dig that out.
Quoted 3 lines
Fix it and mostly unify them.
Signed-off-by Andy Lutomirski <[email protected]>
Signature · 5 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--tip-bot for Andy Lutomirski
merge bot[tip:x86/asm] x86/asm/entry: Unify and fix initial thread_struct: :sp0 values
In reply to Andy Lutomirskix86/asm/entry: Unify and fix initial thread_struct::sp0 values
Commit-ID: 6ecb31635fd0d342b04b9a8e58f2e11382b99bee Gitweb: http://git.kernel.org/tip/6ecb31635fd0d342b04b9a8e58f2e11382b99bee Author: Andy Lutomirski <[email protected]> AuthorDate: Tue, 10 Mar 2015 11:05:59 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Mon, 16 Mar 2015 11:05:36 +0100
x86/asm/entry: Unify and fix initial thread_struct::sp0 values
x86_32 and x86_64 need slightly different thread_struct::sp0 values, and x86_32's was incorrect for init.
This never mattered -- the init thread never runs user code, so we never used thread_struct::sp0 for anything.
Fix it and mostly unify them.
--- arch/x86/include/asm/processor.h | 7 +++++-- arch/x86/kernel/process.c | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-)
arch/x86/include/asm/processor.h +5 -2
@@ -810,6 +810,9 @@ static inline void spin_lock_prefetch(const void *x)prefetchw (x); } +#define TOP_OF_INIT_STACK ((unsigned long)&init_stack + sizeof(init_stack) - \ + TOP_OF_KERNEL_STACK_PADDING) +#ifdef CONFIG_X86_32 /* * User space process size: 3GB (default).
@@ -820,7 +823,7 @@ static inline void spin_lock_prefetch(const void *x)#define STACK_TOP_MAX STACK_TOP#define INIT_THREAD { \ - .sp0 = sizeof(init_stack) + (long)&init_stack, \ + .sp0 = TOP_OF_INIT_STACK, \ .vm86_info = NULL, \ .sysenter_cs = __KERNEL_CS, \ .io_bitmap_ptr = NULL, \
@@ -882,7 +885,7 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define STACK_TOP_MAX TASK_SIZE_MAX#define INIT_THREAD { \ - .sp0 = (unsigned long)&init_stack + sizeof(init_stack) \ + .sp0 = TOP_OF_INIT_STACK \ }/*
arch/x86/kernel/process.c +1 -1
@@ -39,7 +39,7 @@*/ __visible DEFINE_PER_CPU_SHARED_ALIGNED (struct tss_struct ,cpu_tss ) = { .x86_tss = { - .sp0 = (unsigned long )&init_stack +sizeof (init_stack), + .sp0 =TOP_OF_INIT_STACK , #ifdef CONFIG_X86_32 .ss0 = __KERNEL_DS, .ss1 = __KERNEL_CS,
tip-bot for Andy Lutomirski
merge bot[tip:x86/asm] x86/asm/entry: Unify and fix initial thread_struct: :sp0 values
In reply to Andy Lutomirskix86/asm/entry: Unify and fix initial thread_struct::sp0 values
Commit-ID: d9e05cc5a53246e074dc2b84956252e4bbe392cd Gitweb: http://git.kernel.org/tip/d9e05cc5a53246e074dc2b84956252e4bbe392cd Author: Andy Lutomirski <[email protected]> AuthorDate: Tue, 10 Mar 2015 11:05:59 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 17 Mar 2015 09:25:27 +0100
x86/asm/entry: Unify and fix initial thread_struct::sp0 values
x86_32 and x86_64 need slightly different thread_struct::sp0 values, and x86_32's was incorrect for init.
This never mattered -- the init thread never runs user code, so we never used thread_struct::sp0 for anything.
Fix it and mostly unify them.
--- arch/x86/include/asm/processor.h | 7 +++++-- arch/x86/kernel/process.c | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-)
arch/x86/include/asm/processor.h +5 -2
@@ -811,6 +811,9 @@ static inline void spin_lock_prefetch(const void *x)prefetchw (x); } +#define TOP_OF_INIT_STACK ((unsigned long)&init_stack + sizeof(init_stack) - \ + TOP_OF_KERNEL_STACK_PADDING) +#ifdef CONFIG_X86_32 /* * User space process size: 3GB (default).
@@ -821,7 +824,7 @@ static inline void spin_lock_prefetch(const void *x)#define STACK_TOP_MAX STACK_TOP#define INIT_THREAD { \ - .sp0 = sizeof(init_stack) + (long)&init_stack, \ + .sp0 = TOP_OF_INIT_STACK, \ .vm86_info = NULL, \ .sysenter_cs = __KERNEL_CS, \ .io_bitmap_ptr = NULL, \
@@ -883,7 +886,7 @@ extern unsigned long thread_saved_pc(struct task_struct *tsk);#define STACK_TOP_MAX TASK_SIZE_MAX#define INIT_THREAD { \ - .sp0 = (unsigned long)&init_stack + sizeof(init_stack) \ + .sp0 = TOP_OF_INIT_STACK \ }/*
arch/x86/kernel/process.c +1 -1
@@ -39,7 +39,7 @@*/ __visible DEFINE_PER_CPU_SHARED_ALIGNED (struct tss_struct ,cpu_tss ) = { .x86_tss = { - .sp0 = (unsigned long )&init_stack +sizeof (init_stack), + .sp0 =TOP_OF_INIT_STACK , #ifdef CONFIG_X86_32 .ss0 = __KERNEL_DS, .ss1 = __KERNEL_CS,
PATCH 3/3 x86_32: Document our abuse of ss1 and sp1
In reply to Andy LutomirskiThis has confused me for a while. Now that I figured it out, document it.
--- arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
arch/x86/include/asm/processor.h +18 -3
@@ -209,9 +209,24 @@ struct x86_hw_tss {unsigned short back_link, __blh;unsigned long sp0;unsigned short ss0, __ss0h; -unsigned long sp1; -/* ss1 caches MSR_IA32_SYSENTER_CS: */ -unsigned short ss1, __ss1h; + +/* +* We don't use ring 1, so sp1 and ss1 are convenient scratch +* spaces in the same cacheline as sp0. We use them to cache +* some MSR values to avoid unnecessary wrmsr instructions. +* +* We use SYSENTER_ESP to find sp0 and for the NMI emergency +* stack, but we need to context switch it because we do +* horrible things to the kernel stack in vm86 mode. +* +* We use SYSENTER_CS to disable sysenter in vm86 mode to avoid +* corrupting the stack if we went through the sysenter path +* from vm86 mode. +*/ +unsigned long sp1;/* MSR_IA32_SYSENTER_ESP */ +unsigned short ss1;/* MSR_IA32_SYSENTER_CS */ + +unsigned short __ss1h;unsigned long sp2;unsigned short ss2, __ss2h;unsigned long __cr3;
Signature · 1 line
2.3.0
Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1
Quoted 2 lines · Andy Lutomirski
This has confused me for a while. Now that I figured it out, document it.
Great!
Quoted 11 lines
Signed-off-by Andy Lutomirski <[email protected]>--- arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)arch/x86/include/asm/processor.h +0
-0@@ -209,9 +209,24 @@ struct x86_hw_tss {unsigned short back_link, __blh;
arch/x86/include/asm/processor.h struct x86_hw_tss { · line 216
Quoted 10 lines
arch/x86/include/asm/processor.h +5
-3unsigned long sp0;unsigned short ss0, __ss0h; -unsigned long sp1; -/* ss1 caches MSR_IA32_SYSENTER_CS: */ -unsigned short ss1, __ss1h; + +/* +* We don't use ring 1, so sp1 and ss1 are convenient scratch +* spaces in the same cacheline as sp0. We use them to cache +* some MSR values to avoid unnecessary wrmsr instructions.
I don't see where exactly tss.ss1/sp1 is getting used as cache. Grepping for "sp1" string, I found only this:
$ grep -r '[.>]e*sp1' . ./kernel/cpu/common.c: tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss; ./kernel/cpu/common.c: wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);
void enable_sep_cpu(void)
{
int cpu = get_cpu();
struct tss_struct *tss = &per_cpu(init_tss, cpu);
...
tss->x86_tss.ss1 = __KERNEL_CS;
tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss;
wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0);
wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);
wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long) ia32_sysenter_target, 0);
put_cpu();
}It's trivial to rewrite this wrmsr(MSR_IA32_SYSENTER_ESP) without the detour through x86_tss.sp1.
Apart from this, x86_tss.sp1 appears unused... ????confused????
.ss1 also seems to be a write-only field:
$ grep -r '[.>]ss1' .
./include/asm/processor.h: if (unlikely(tss->x86_tss.ss1 != thread->sysenter_cs)) {
./include/asm/processor.h: tss->x86_tss.ss1 = thread->sysenter_cs;
./include/asm/processor.h: .ss1 = __KERNEL_CS, \
./kernel/cpu/common.c: tss->x86_tss.ss1 = __KERNEL_CS;arch/x86/include/asm/processor.h struct x86_hw_tss {
Quoted 3 lines
arch/x86/include/asm/processor.h +3
-0+* +* We use SYSENTER_ESP to find sp0 and for the NMI emergency +* stack,
We use what? SYSENTER_ESP is a MSR, right? We don't use it (the MSR) to find anything... I don't understand what you are saying here.
but we need to context switch it because we do
arch/x86/include/asm/processor.h struct x86_hw_tss { · line 225
Quoted 6 lines
arch/x86/include/asm/processor.h +6
-0+* horrible things to the kernel stack in vm86 mode. +* +* We use SYSENTER_CS to disable sysenter in vm86 mode to avoid +* corrupting the stack if we went through the sysenter path +* from vm86 mode. +*/
I'm confused how loading ss1/sp1 with anything can disable sysenter. SYSENTER insn does not use those fields.
What you _can_ disable is you can make it impossible to enter RING1 if tss.ss1 is invalid.
arch/x86/include/asm/processor.h struct x86_hw_tss { · line 227
Quoted 2 lines
arch/x86/include/asm/processor.h +2
-0+unsigned long sp1;/* MSR_IA32_SYSENTER_ESP */ +unsigned short ss1;/* MSR_IA32_SYSENTER_CS */
The comments in the right don't explain anything (to me, at least).
Sorry for sounding negative.
Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1
Quoted 48 lines · Denys Vlasenko
Quoted 2 lines · Andy Lutomirski
This has confused me for a while. Now that I figured it out, document it.
Great!
Quoted 11 lines
Signed-off-by Andy Lutomirski <[email protected]>--- arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)arch/x86/include/asm/processor.h +0
-0@@ -209,9 +209,24 @@ struct x86_hw_tss {unsigned short back_link, __blh;arch/x86/include/asm/processor.h struct x86_hw_tss { · line 216
Quoted 10 lines
arch/x86/include/asm/processor.h +5
-3unsigned long sp0;unsigned short ss0, __ss0h; -unsigned long sp1; -/* ss1 caches MSR_IA32_SYSENTER_CS: */ -unsigned short ss1, __ss1h; + +/* +* We don't use ring 1, so sp1 and ss1 are convenient scratch +* spaces in the same cacheline as sp0. We use them to cache +* some MSR values to avoid unnecessary wrmsr instructions. I don't see where exactly tss.ss1/sp1 is getting used as cache. Grepping for "sp1" string, I found only this:
$ grep -r '[.>]e*sp1' . ./kernel/cpu/common.c: tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss; ./kernel/cpu/common.c: wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0);void
enable_sep_cpu(void){ int cpu = get_cpu(); struct tss_struct *tss = &per_cpu(init_tss, cpu); ... tss->x86_tss.ss1 = __KERNEL_CS; tss->x86_tss.sp1 = sizeof(struct tss_struct) + (unsigned long) tss; wrmsr(MSR_IA32_SYSENTER_CS, __KERNEL_CS, 0); wrmsr(MSR_IA32_SYSENTER_ESP, tss->x86_tss.sp1, 0); wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long) ia32_sysenter_target, 0); put_cpu(); }
Quoted 5 lines
It's trivial to rewrite this
wrmsr(MSR_IA32_SYSENTER_ESP)without the detour through x86_tss.sp1.Apart from this, x86_tss.sp1 appears unused... ????confused????
Hmm. Perhaps I hallucinated it. Maybe we should just remove this instead. We change sp0, but not SYSENTER_ESP. I'll add a fourth patch to the series.
Quoted 6 lines
.ss1 also seems to be a write-only field:
$ grep -r '[.>]ss1' . ./include/asm/processor.h: if (unlikely(tss->x86_tss.ss1 != thread->sysenter_cs)) {
This is a read :)
Quoted 6 lines
./include/asm/processor.h:tss->x86_tss.ss1=thread->sysenter_cs;./include/asm/processor.h: .ss1 =__KERNEL_CS, \./kernel/cpu/common.c:tss->x86_tss.ss1=__KERNEL_CS;
Quoted 7 lines
arch/x86/include/asm/processor.h struct x86_hw_tss {
Quoted 3 lines
arch/x86/include/asm/processor.h +3
-0+* +* We use SYSENTER_ESP to find sp0 and for the NMI emergency +* stack, We use what?
SYSENTER_ESPis a MSR, right? We don't use it (the MSR) to find anything... I don't understand what you are saying here.
As noted above, I'm wrong, so I won't bother clarifying. Will fix.
Quoted 9 lines
but we need to context switch it because we do
arch/x86/include/asm/processor.h struct x86_hw_tss { · line 225
Quoted 6 lines
arch/x86/include/asm/processor.h +6
-0+* horrible things to the kernel stack in vm86 mode. +* +* We use SYSENTER_CS to disable sysenter in vm86 mode to avoid +* corrupting the stack if we went through the sysenter path +* from vm86 mode. +*/
Quoted 5 lines
I'm confused how loading ss1/sp1 with anything can disable sysenter. SYSENTER insn does not use those fields.
What you _can_ disable is you can make it impossible to enter RING1 if tss.ss1 is invalid.
Does it make sense now that I pointed out the read of ss1? If not, I'll improve the comments.
Quoted 8 lines
arch/x86/include/asm/processor.h struct x86_hw_tss { · line 227
Quoted 4 lines
arch/x86/include/asm/processor.h +2
-0+unsigned long sp1;/* MSR_IA32_SYSENTER_ESP */ +unsigned short ss1;/* MSR_IA32_SYSENTER_CS */ The comments in the right don't explain anything (to me, at least).
Sorry for sounding negative.
No problem :)
--Andy
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1
Quoted 6 lines · Andy Lutomirski
Quoted 4 lines
.ss1 also seems to be a write-only field:
$ grep -r '[.>]ss1' . ./include/asm/processor.h: if (unlikely(tss->x86_tss.ss1 != thread->sysenter_cs)) {This is a read :)
You are right.
Quoted 12 lines
Quoted 12 lines
./include/asm/processor.h:tss->x86_tss.ss1=thread->sysenter_cs;./include/asm/processor.h: .ss1 =__KERNEL_CS, \./kernel/cpu/common.c:tss->x86_tss.ss1=__KERNEL_CS;Quoted 7 lines
arch/x86/include/asm/processor.h +7
-0+* but we need to context switch it because we do +* horrible things to the kernel stack in vm86 mode. +* +* We use SYSENTER_CS to disable sysenter in vm86 mode to avoid +* corrupting the stack if we went through the sysenter path +* from vm86 mode. +*/
Quoted 8 lines
Quoted 5 lines
I'm confused how loading ss1/sp1 with anything can disable sysenter. SYSENTER insn does not use those fields.
What you _can_ disable is you can make it impossible to enter RING1 if tss.ss1 is invalid.
Does it make sense now that I pointed out the read of ss1? If not, I'll improve the comments.
I propose the following comment about tss.ss1:
/*
tss.ss1 is used to avoid redundant wrmsr(MSR_IA32_SYSENTER_CS). After wrmsr, tss.ss1 is set to the written value. If on future task switches tss.ss1 already contains the value to be written, wrmsr is skipped. */
Quoted 3 lines
Quoted 3 lines
arch/x86/include/asm/processor.h struct x86_hw_tss { · line 224
Quoted 3 lines
arch/x86/include/asm/processor.h +3
-0+* We use SYSENTER_CS to disable sysenter in vm86 mode to avoid +* corrupting the stack if we went through the sysenter path +* from vm86 mode.
This appears to be untrue - SYSENTER_CS isn't used to disable sysenter. Zero is.
Disabling sysenter happens in vm86_32.c here, by setting it to 0:
static void do_sys_vm86(struct kernel_vm86_struct *info, struct task_struct *tsk)
{
...
if (cpu_has_sep)
tsk->thread.sysenter_cs = 0;
load_sp0(tss, &tsk->thread); <-- this setswrmsr(MSR_IA32_SYSENTER_CS, 0);
...
tip-bot for Andy Lutomirski
merge bot[tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1
In reply to Andy Lutomirskix86/asm/entry/32: Document our abuse of x86_hw_tss::ss1 and x86_hw_tss::sp1
Commit-ID: f7113ffa3bcd630066119723f539db75a5721c88 Gitweb: http://git.kernel.org/tip/f7113ffa3bcd630066119723f539db75a5721c88 Author: Andy Lutomirski <[email protected]> AuthorDate: Tue, 10 Mar 2015 11:06:00 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Mon, 16 Mar 2015 11:05:36 +0100
x86/asm/entry/32: Document our abuse of x86_hw_tss::ss1 and x86_hw_tss::sp1
This has confused me for a while. Now that I figured it out, document it.
--- arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
arch/x86/include/asm/processor.h +18 -3
@@ -209,9 +209,24 @@ struct x86_hw_tss {unsigned short back_link, __blh;unsigned long sp0;unsigned short ss0, __ss0h; -unsigned long sp1; -/* ss1 caches MSR_IA32_SYSENTER_CS: */ -unsigned short ss1, __ss1h; + +/* +* We don't use ring 1, so sp1 and ss1 are convenient scratch +* spaces in the same cacheline as sp0. We use them to cache +* some MSR values to avoid unnecessary wrmsr instructions. +* +* We use SYSENTER_ESP to find sp0 and for the NMI emergency +* stack, but we need to context switch it because we do +* horrible things to the kernel stack in vm86 mode. +* +* We use SYSENTER_CS to disable sysenter in vm86 mode to avoid +* corrupting the stack if we went through the sysenter path +* from vm86 mode. +*/ +unsigned long sp1;/* MSR_IA32_SYSENTER_ESP */ +unsigned short ss1;/* MSR_IA32_SYSENTER_CS */ + +unsigned short __ss1h;unsigned long sp2;unsigned short ss2, __ss2h;unsigned long __cr3;
Re: [tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1
Quoted 34 lines · tip-bot for Andy Lutomirski
Commit-ID: f7113ffa3bcd630066119723f539db75a5721c88 Gitweb: http://git.kernel.org/tip/f7113ffa3bcd630066119723f539db75a5721c88 Author: Andy Lutomirski <[email protected]> AuthorDate: Tue, 10 Mar 2015 11:06:00 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Mon, 16 Mar 2015 11:05:36 +0100x86/asm/entry/32: Document our abuse of
x86_hw_tss::ss1andx86_hw_tss::sp1This has confused me for a while. Now that I figured it out, document it.
Signed-off-by Andy Lutomirski <[email protected]>Cc Borislav Petkov <[email protected]>Cc Denys Vlasenko <[email protected]>Cc H. Peter Anvin <[email protected]>Cc Linus Torvalds <[email protected]>Cc Oleg Nesterov <[email protected]>Cc Thomas Gleixner <[email protected]>Link http://lkml.kernel.org/r/b7efc1b7364039824776f68e9ddee9ec1500e894.1426009661.git.luto@amacapital.net open hereSigned-off-by Ingo Molnar <[email protected]>--- arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)arch/x86/include/asm/processor.h +0
-2@@ -209,9 +209,24 @@ struct x86_hw_tss {unsigned short back_link, __blh;unsigned long sp0;unsigned short ss0, __ss0h; -unsigned long sp1; -/* ss1 caches MSR_IA32_SYSENTER_CS: */
arch/x86/include/asm/processor.h struct x86_hw_tss { · line 220
Quoted 10 lines
arch/x86/include/asm/processor.h +9
-1-unsigned short ss1, __ss1h; + +/* +* We don't use ring 1, so sp1 and ss1 are convenient scratch +* spaces in the same cacheline as sp0. We use them to cache +* some MSR values to avoid unnecessary wrmsr instructions. +* +* We use SYSENTER_ESP to find sp0 and for the NMI emergency +* stack, but we need to context switch it because we do +* horrible things to the kernel stack in vm86 mode.
I should have sent out my v2 sooner. Denys correctly noted that the sp1 cache is completely useless. I'll send an incremental patch today.
--Andy
Quoted 12 lines
arch/x86/include/asm/processor.h +9
-0+* +* We use SYSENTER_CS to disable sysenter in vm86 mode to avoid +* corrupting the stack if we went through the sysenter path +* from vm86 mode. +*/ +unsigned long sp1;/* MSR_IA32_SYSENTER_ESP */ +unsigned short ss1;/* MSR_IA32_SYSENTER_CS */ + +unsigned short __ss1h;unsigned long sp2;unsigned short ss2, __ss2h;unsigned long __cr3;
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
tip-bot for Andy Lutomirski
merge bot[tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1
In reply to Andy Lutomirskix86/asm/entry/32: Document our abuse of x86_hw_tss::ss1 and x86_hw_tss::sp1
Commit-ID: 76e4c4908a4904a61aa67ae5eb0b2a7588c4a546 Gitweb: http://git.kernel.org/tip/76e4c4908a4904a61aa67ae5eb0b2a7588c4a546 Author: Andy Lutomirski <[email protected]> AuthorDate: Tue, 10 Mar 2015 11:06:00 -0700 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 17 Mar 2015 09:25:27 +0100
x86/asm/entry/32: Document our abuse of x86_hw_tss::ss1 and x86_hw_tss::sp1
This has confused me for a while. Now that I figured it out, document it.
--- arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)
arch/x86/include/asm/processor.h +18 -3
@@ -209,9 +209,24 @@ struct x86_hw_tss {unsigned short back_link, __blh;unsigned long sp0;unsigned short ss0, __ss0h; -unsigned long sp1; -/* ss1 caches MSR_IA32_SYSENTER_CS: */ -unsigned short ss1, __ss1h; + +/* +* We don't use ring 1, so sp1 and ss1 are convenient scratch +* spaces in the same cacheline as sp0. We use them to cache +* some MSR values to avoid unnecessary wrmsr instructions. +* +* We use SYSENTER_ESP to find sp0 and for the NMI emergency +* stack, but we need to context switch it because we do +* horrible things to the kernel stack in vm86 mode. +* +* We use SYSENTER_CS to disable sysenter in vm86 mode to avoid +* corrupting the stack if we went through the sysenter path +* from vm86 mode. +*/ +unsigned long sp1;/* MSR_IA32_SYSENTER_ESP */ +unsigned short ss1;/* MSR_IA32_SYSENTER_CS */ + +unsigned short __ss1h;unsigned long sp2;unsigned short ss2, __ss2h;unsigned long __cr3;