* [PATCH 0/3] sp0, ss1, and sp1 docs and minor fixes
@ 2015-03-10 18:05 Andy Lutomirski
2015-03-10 18:05 ` [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro Andy Lutomirski
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Andy Lutomirski @ 2015-03-10 18:05 UTC (permalink / raw)
To: x86, linux-kernel
Cc: Borislav Petkov, Oleg Nesterov, Denys Vlasenko, Andy Lutomirski
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(-)
--
2.3.0
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro 2015-03-10 18:05 [PATCH 0/3] sp0, ss1, and sp1 docs and minor fixes Andy Lutomirski @ 2015-03-10 18:05 ` Andy Lutomirski 2015-03-10 19:22 ` Denys Vlasenko ` (3 more replies) 2015-03-10 18:05 ` [PATCH 2/3] x86: Unify and fix init sp0 Andy Lutomirski 2015-03-10 18:06 ` [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 Andy Lutomirski 2 siblings, 4 replies; 19+ messages in thread From: Andy Lutomirski @ 2015-03-10 18:05 UTC (permalink / raw) To: x86, linux-kernel Cc: Borislav Petkov, Oleg Nesterov, Denys Vlasenko, Andy Lutomirski 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. Signed-off-by: Andy Lutomirski <luto@amacapital.net> --- arch/x86/include/asm/processor.h | 3 ++- arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 48a61c1c626e..88d9aa745898 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -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; \ }) diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h index 7740edd56fed..74fd74ca50d3 100644 --- a/arch/x86/include/asm/thread_info.h +++ b/arch/x86/include/asm/thread_info.h @@ -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> -- 2.3.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro 2015-03-10 18:05 ` [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro Andy Lutomirski @ 2015-03-10 19:22 ` Denys Vlasenko 2015-03-10 19:47 ` Andy Lutomirski 2015-03-13 14:08 ` Denys Vlasenko 2015-03-16 8:56 ` Ingo Molnar ` (2 subsequent siblings) 3 siblings, 2 replies; 19+ messages in thread From: Denys Vlasenko @ 2015-03-10 19:22 UTC (permalink / raw) To: Andy Lutomirski, x86, linux-kernel; +Cc: Borislav Petkov, Oleg Nesterov On 03/10/2015 07:05 PM, Andy Lutomirski wrote: > 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. > > Signed-off-by: Andy Lutomirski <luto@amacapital.net> > --- > arch/x86/include/asm/processor.h | 3 ++- > arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > index 48a61c1c626e..88d9aa745898 100644 > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -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_STATE ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro 2015-03-10 19:22 ` Denys Vlasenko @ 2015-03-10 19:47 ` Andy Lutomirski 2015-03-13 14:08 ` Denys Vlasenko 1 sibling, 0 replies; 19+ messages in thread From: Andy Lutomirski @ 2015-03-10 19:47 UTC (permalink / raw) To: Denys Vlasenko; +Cc: X86 ML, linux-kernel, Borislav Petkov, Oleg Nesterov On Tue, Mar 10, 2015 at 12:22 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote: > On 03/10/2015 07:05 PM, Andy Lutomirski wrote: >> 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. >> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net> >> --- >> arch/x86/include/asm/processor.h | 3 ++- >> arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ >> 2 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h >> index 48a61c1c626e..88d9aa745898 100644 >> --- a/arch/x86/include/asm/processor.h >> +++ b/arch/x86/include/asm/processor.h >> @@ -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) > ^^^ I'll just put TOP_OF_KERNEL_STACK_PADDING there, too. --Andy > 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_STATE > -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro 2015-03-10 19:22 ` Denys Vlasenko 2015-03-10 19:47 ` Andy Lutomirski @ 2015-03-13 14:08 ` Denys Vlasenko 1 sibling, 0 replies; 19+ messages in thread From: Denys Vlasenko @ 2015-03-13 14:08 UTC (permalink / raw) To: Denys Vlasenko Cc: Andy Lutomirski, X86 ML, Linux Kernel Mailing List, Borislav Petkov, Oleg Nesterov On Tue, Mar 10, 2015 at 8:22 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote: > On 03/10/2015 07:05 PM, Andy Lutomirski wrote: >> 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. >> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net> >> --- >> arch/x86/include/asm/processor.h | 3 ++- >> arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ >> 2 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h >> index 48a61c1c626e..88d9aa745898 100644 >> --- a/arch/x86/include/asm/processor.h >> +++ b/arch/x86/include/asm/processor.h >> @@ -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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro 2015-03-10 18:05 ` [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro Andy Lutomirski 2015-03-10 19:22 ` Denys Vlasenko @ 2015-03-16 8:56 ` Ingo Molnar 2015-03-16 12:08 ` [tip:x86/asm] x86/asm/entry: Create and use a ' TOP_OF_KERNEL_STACK_PADDING' macro tip-bot for Andy Lutomirski 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 3 siblings, 0 replies; 19+ messages in thread From: Ingo Molnar @ 2015-03-16 8:56 UTC (permalink / raw) To: Andy Lutomirski Cc: x86, linux-kernel, Borislav Petkov, Oleg Nesterov, Denys Vlasenko * Andy Lutomirski <luto@amacapital.net> wrote: > 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. > > Signed-off-by: Andy Lutomirski <luto@amacapital.net> > --- > arch/x86/include/asm/processor.h | 3 ++- > arch/x86/include/asm/thread_info.h | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > index 48a61c1c626e..88d9aa745898 100644 > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -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; \ > }) > > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h > index 7740edd56fed..74fd74ca50d3 100644 > --- a/arch/x86/include/asm/thread_info.h > +++ b/arch/x86/include/asm/thread_info.h > @@ -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 s/the/then I fixed this up in the commit. > + * 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tip:x86/asm] x86/asm/entry: Create and use a ' TOP_OF_KERNEL_STACK_PADDING' macro 2015-03-10 18:05 ` [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro Andy Lutomirski 2015-03-10 19:22 ` Denys Vlasenko 2015-03-16 8:56 ` Ingo Molnar @ 2015-03-16 12:08 ` tip-bot for Andy Lutomirski 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 3 siblings, 0 replies; 19+ messages in thread From: tip-bot for Andy Lutomirski @ 2015-03-16 12:08 UTC (permalink / raw) To: linux-tip-commits Cc: luto, hpa, dvlasenk, mingo, torvalds, bp, linux-kernel, tglx, oleg Commit-ID: 3bfef48e27863ebbd47d68a3f4a081d2cb5286aa Gitweb: http://git.kernel.org/tip/3bfef48e27863ebbd47d68a3f4a081d2cb5286aa Author: Andy Lutomirski <luto@amacapital.net> AuthorDate: Tue, 10 Mar 2015 11:05:58 -0700 Committer: Ingo Molnar <mingo@kernel.org> 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. Signed-off-by: Andy Lutomirski <luto@amacapital.net> Acked-by: Denys Vlasenko <dvlasenk@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: H. Peter Anvin <hpa@zytor.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/02bf2f54b8dcb76a62a142b6dfe07d4ef7fc582e.1426009661.git.luto@amacapital.net [ Fixed small details, such as a missed magic constant in entry_32.S pointed out by Denys Vlasenko. ] Signed-off-by: Ingo Molnar <mingo@kernel.org> --- 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(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index c77605d..554da61 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -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; \ }) diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h index 7740edd..ba115eb 100644 --- a/arch/x86/include/asm/thread_info.h +++ b/arch/x86/include/asm/thread_info.h @@ -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 diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S index e33ba51..4c8cc34 100644 --- a/arch/x86/kernel/entry_32.S +++ b/arch/x86/kernel/entry_32.S @@ -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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [tip:x86/asm] x86/asm/entry: Create and use a ' TOP_OF_KERNEL_STACK_PADDING' macro 2015-03-10 18:05 ` [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro Andy Lutomirski ` (2 preceding siblings ...) 2015-03-16 12:08 ` [tip:x86/asm] x86/asm/entry: Create and use a ' TOP_OF_KERNEL_STACK_PADDING' macro tip-bot for Andy Lutomirski @ 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 3 siblings, 0 replies; 19+ messages in thread From: tip-bot for Andy Lutomirski @ 2015-03-17 8:45 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, tglx, torvalds, bp, hpa, linux-kernel, dvlasenk, luto, oleg Commit-ID: 3ee4298f440c81638cbb5ec06f2497fb7a9a9eb4 Gitweb: http://git.kernel.org/tip/3ee4298f440c81638cbb5ec06f2497fb7a9a9eb4 Author: Andy Lutomirski <luto@amacapital.net> AuthorDate: Tue, 10 Mar 2015 11:05:58 -0700 Committer: Ingo Molnar <mingo@kernel.org> 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. Signed-off-by: Andy Lutomirski <luto@amacapital.net> Acked-by: Denys Vlasenko <dvlasenk@redhat.com> Cc: Borislav Petkov <bp@alien8.de> Cc: H. Peter Anvin <hpa@zytor.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/02bf2f54b8dcb76a62a142b6dfe07d4ef7fc582e.1426009661.git.luto@amacapital.net [ Fixed small details, such as a missed magic constant in entry_32.S pointed out by Denys Vlasenko. ] Signed-off-by: Ingo Molnar <mingo@kernel.org> --- 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(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 48a61c1..88d9aa7 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -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; \ }) diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h index 7740edd..ba115eb 100644 --- a/arch/x86/include/asm/thread_info.h +++ b/arch/x86/include/asm/thread_info.h @@ -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 diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S index e33ba51..4c8cc34 100644 --- a/arch/x86/kernel/entry_32.S +++ b/arch/x86/kernel/entry_32.S @@ -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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] x86: Unify and fix init sp0 2015-03-10 18:05 [PATCH 0/3] sp0, ss1, and sp1 docs and minor fixes Andy Lutomirski 2015-03-10 18:05 ` [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro Andy Lutomirski @ 2015-03-10 18:05 ` Andy Lutomirski 2015-03-11 11:21 ` Borislav Petkov ` (2 more replies) 2015-03-10 18:06 ` [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 Andy Lutomirski 2 siblings, 3 replies; 19+ messages in thread From: Andy Lutomirski @ 2015-03-10 18:05 UTC (permalink / raw) To: x86, linux-kernel Cc: Borislav Petkov, Oleg Nesterov, Denys Vlasenko, Andy Lutomirski x86_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. Signed-off-by: Andy Lutomirski <luto@amacapital.net> --- arch/x86/include/asm/processor.h | 7 +++++-- arch/x86/kernel/process.c | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 88d9aa745898..fc6d8d0d8d53 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -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 \ } /* diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index f4c0af7fc3a0..12b1cf606ddf 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -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, -- 2.3.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] x86: Unify and fix init sp0 2015-03-10 18:05 ` [PATCH 2/3] x86: Unify and fix init sp0 Andy Lutomirski @ 2015-03-11 11:21 ` Borislav Petkov 2015-03-16 12:09 ` [tip:x86/asm] x86/asm/entry: Unify and fix initial thread_struct: :sp0 values tip-bot for Andy Lutomirski 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 2 siblings, 0 replies; 19+ messages in thread From: Borislav Petkov @ 2015-03-11 11:21 UTC (permalink / raw) To: Andy Lutomirski; +Cc: x86, linux-kernel, Oleg Nesterov, Denys Vlasenko On Tue, Mar 10, 2015 at 11:05:59AM -0700, Andy Lutomirski wrote: > x86_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.) Damn old x86 cruft - sp0 is the stack pointer for CPL0 in the TSS. Had to go dig that out. > Fix it and mostly unify them. > > Signed-off-by: Andy Lutomirski <luto@amacapital.net> Acked-by: Borislav Petkov <bp@suse.de> -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tip:x86/asm] x86/asm/entry: Unify and fix initial thread_struct: :sp0 values 2015-03-10 18:05 ` [PATCH 2/3] x86: Unify and fix init sp0 Andy Lutomirski 2015-03-11 11:21 ` Borislav Petkov @ 2015-03-16 12:09 ` tip-bot for Andy Lutomirski 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 2 siblings, 0 replies; 19+ messages in thread From: tip-bot for Andy Lutomirski @ 2015-03-16 12:09 UTC (permalink / raw) To: linux-tip-commits Cc: torvalds, luto, hpa, mingo, oleg, dvlasenk, linux-kernel, tglx, bp Commit-ID: 6ecb31635fd0d342b04b9a8e58f2e11382b99bee Gitweb: http://git.kernel.org/tip/6ecb31635fd0d342b04b9a8e58f2e11382b99bee Author: Andy Lutomirski <luto@amacapital.net> AuthorDate: Tue, 10 Mar 2015 11:05:59 -0700 Committer: Ingo Molnar <mingo@kernel.org> 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. Signed-off-by: Andy Lutomirski <luto@amacapital.net> Cc: Borislav Petkov <bp@alien8.de> Cc: Denys Vlasenko <dvlasenk@redhat.com> Cc: H. Peter Anvin <hpa@zytor.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/1b810c1d2e797e27bb4a7708c426101161edd1f6.1426009661.git.luto@amacapital.net Signed-off-by: Ingo Molnar <mingo@kernel.org> --- arch/x86/include/asm/processor.h | 7 +++++-- arch/x86/kernel/process.c | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 554da61..aed6d4f 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -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 \ } /* diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index f4c0af7..12b1cf6 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -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, ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [tip:x86/asm] x86/asm/entry: Unify and fix initial thread_struct: :sp0 values 2015-03-10 18:05 ` [PATCH 2/3] x86: Unify and fix init sp0 Andy Lutomirski 2015-03-11 11:21 ` Borislav Petkov 2015-03-16 12:09 ` [tip:x86/asm] x86/asm/entry: Unify and fix initial thread_struct: :sp0 values tip-bot for Andy Lutomirski @ 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 2 siblings, 0 replies; 19+ messages in thread From: tip-bot for Andy Lutomirski @ 2015-03-17 8:45 UTC (permalink / raw) To: linux-tip-commits Cc: mingo, dvlasenk, linux-kernel, torvalds, hpa, luto, tglx, oleg, bp Commit-ID: d9e05cc5a53246e074dc2b84956252e4bbe392cd Gitweb: http://git.kernel.org/tip/d9e05cc5a53246e074dc2b84956252e4bbe392cd Author: Andy Lutomirski <luto@amacapital.net> AuthorDate: Tue, 10 Mar 2015 11:05:59 -0700 Committer: Ingo Molnar <mingo@kernel.org> 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. Signed-off-by: Andy Lutomirski <luto@amacapital.net> Cc: Borislav Petkov <bp@alien8.de> Cc: Denys Vlasenko <dvlasenk@redhat.com> Cc: H. Peter Anvin <hpa@zytor.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/1b810c1d2e797e27bb4a7708c426101161edd1f6.1426009661.git.luto@amacapital.net Signed-off-by: Ingo Molnar <mingo@kernel.org> --- arch/x86/include/asm/processor.h | 7 +++++-- arch/x86/kernel/process.c | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 88d9aa7..fc6d8d0 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -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 \ } /* diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index f4c0af7..12b1cf6 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -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, ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 2015-03-10 18:05 [PATCH 0/3] sp0, ss1, and sp1 docs and minor fixes Andy Lutomirski 2015-03-10 18:05 ` [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro Andy Lutomirski 2015-03-10 18:05 ` [PATCH 2/3] x86: Unify and fix init sp0 Andy Lutomirski @ 2015-03-10 18:06 ` Andy Lutomirski 2015-03-10 19:13 ` Denys Vlasenko ` (2 more replies) 2 siblings, 3 replies; 19+ messages in thread From: Andy Lutomirski @ 2015-03-10 18:06 UTC (permalink / raw) To: x86, linux-kernel Cc: Borislav Petkov, Oleg Nesterov, Denys Vlasenko, Andy Lutomirski This has confused me for a while. Now that I figured it out, document it. Signed-off-by: Andy Lutomirski <luto@amacapital.net> --- arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index fc6d8d0d8d53..b26208998b7c 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -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; -- 2.3.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 2015-03-10 18:06 ` [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 Andy Lutomirski @ 2015-03-10 19:13 ` Denys Vlasenko 2015-03-10 20:06 ` Andy Lutomirski 2015-03-16 12:09 ` [tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1 tip-bot for Andy Lutomirski 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 2 siblings, 1 reply; 19+ messages in thread From: Denys Vlasenko @ 2015-03-10 19:13 UTC (permalink / raw) To: Andy Lutomirski, x86, linux-kernel; +Cc: Borislav Petkov, Oleg Nesterov On 03/10/2015 07:06 PM, Andy Lutomirski wrote: > This has confused me for a while. Now that I figured it out, > document it. Great! > Signed-off-by: Andy Lutomirski <luto@amacapital.net> > --- > arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > index fc6d8d0d8d53..b26208998b7c 100644 > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -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. 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; > + * > + * 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 > + * 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. > + 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 2015-03-10 19:13 ` Denys Vlasenko @ 2015-03-10 20:06 ` Andy Lutomirski 2015-03-10 20:52 ` Denys Vlasenko 0 siblings, 1 reply; 19+ messages in thread From: Andy Lutomirski @ 2015-03-10 20:06 UTC (permalink / raw) To: Denys Vlasenko; +Cc: X86 ML, linux-kernel, Borislav Petkov, Oleg Nesterov On Tue, Mar 10, 2015 at 12:13 PM, Denys Vlasenko <dvlasenk@redhat.com> wrote: > On 03/10/2015 07:06 PM, Andy Lutomirski wrote: >> This has confused me for a while. Now that I figured it out, >> document it. > > Great! > >> Signed-off-by: Andy Lutomirski <luto@amacapital.net> >> --- >> arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h >> index fc6d8d0d8d53..b26208998b7c 100644 >> --- a/arch/x86/include/asm/processor.h >> +++ b/arch/x86/include/asm/processor.h >> @@ -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. > > 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???? > 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. > > > .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 :) > ./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; > > > >> + * >> + * 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. > As noted above, I'm wrong, so I won't bother clarifying. Will fix. > > 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. >> + */ > > 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. > > >> + 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 -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 2015-03-10 20:06 ` Andy Lutomirski @ 2015-03-10 20:52 ` Denys Vlasenko 0 siblings, 0 replies; 19+ messages in thread From: Denys Vlasenko @ 2015-03-10 20:52 UTC (permalink / raw) To: Andy Lutomirski Cc: Denys Vlasenko, X86 ML, linux-kernel, Borislav Petkov, Oleg Nesterov On Tue, Mar 10, 2015 at 9:06 PM, Andy Lutomirski <luto@amacapital.net> wrote: >> .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. >> ./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; >>> + * 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. >>> + */ >> >> 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. */ >>> + * 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 sets wrmsr(MSR_IA32_SYSENTER_CS, 0); ... ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1 2015-03-10 18:06 ` [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 Andy Lutomirski 2015-03-10 19:13 ` Denys Vlasenko @ 2015-03-16 12:09 ` tip-bot for Andy Lutomirski 2015-03-16 15:36 ` Andy Lutomirski 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 2 siblings, 1 reply; 19+ messages in thread From: tip-bot for Andy Lutomirski @ 2015-03-16 12:09 UTC (permalink / raw) To: linux-tip-commits Cc: oleg, mingo, bp, luto, dvlasenk, torvalds, linux-kernel, tglx, hpa Commit-ID: f7113ffa3bcd630066119723f539db75a5721c88 Gitweb: http://git.kernel.org/tip/f7113ffa3bcd630066119723f539db75a5721c88 Author: Andy Lutomirski <luto@amacapital.net> AuthorDate: Tue, 10 Mar 2015 11:06:00 -0700 Committer: Ingo Molnar <mingo@kernel.org> 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. Signed-off-by: Andy Lutomirski <luto@amacapital.net> Cc: Borislav Petkov <bp@alien8.de> Cc: Denys Vlasenko <dvlasenk@redhat.com> Cc: H. Peter Anvin <hpa@zytor.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/b7efc1b7364039824776f68e9ddee9ec1500e894.1426009661.git.luto@amacapital.net Signed-off-by: Ingo Molnar <mingo@kernel.org> --- arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index aed6d4f..c3a037b 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -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; ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1 2015-03-16 12:09 ` [tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1 tip-bot for Andy Lutomirski @ 2015-03-16 15:36 ` Andy Lutomirski 0 siblings, 0 replies; 19+ messages in thread From: Andy Lutomirski @ 2015-03-16 15:36 UTC (permalink / raw) To: Andy Lutomirski, Oleg Nesterov, Borislav Petkov, Ingo Molnar, Denys Vlasenko, Linus Torvalds, linux-kernel, Thomas Gleixner, H. Peter Anvin Cc: linux-tip-commits On Mon, Mar 16, 2015 at 5:09 AM, tip-bot for Andy Lutomirski <tipbot@zytor.com> wrote: > Commit-ID: f7113ffa3bcd630066119723f539db75a5721c88 > Gitweb: http://git.kernel.org/tip/f7113ffa3bcd630066119723f539db75a5721c88 > Author: Andy Lutomirski <luto@amacapital.net> > AuthorDate: Tue, 10 Mar 2015 11:06:00 -0700 > Committer: Ingo Molnar <mingo@kernel.org> > 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. > > Signed-off-by: Andy Lutomirski <luto@amacapital.net> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Denys Vlasenko <dvlasenk@redhat.com> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Oleg Nesterov <oleg@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Link: http://lkml.kernel.org/r/b7efc1b7364039824776f68e9ddee9ec1500e894.1426009661.git.luto@amacapital.net > Signed-off-by: Ingo Molnar <mingo@kernel.org> > --- > arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h > index aed6d4f..c3a037b 100644 > --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -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. 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 > + * > + * 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; -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 19+ messages in thread
* [tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1 2015-03-10 18:06 ` [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 Andy Lutomirski 2015-03-10 19:13 ` Denys Vlasenko 2015-03-16 12:09 ` [tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1 tip-bot for Andy Lutomirski @ 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 2 siblings, 0 replies; 19+ messages in thread From: tip-bot for Andy Lutomirski @ 2015-03-17 8:45 UTC (permalink / raw) To: linux-tip-commits Cc: tglx, linux-kernel, luto, dvlasenk, bp, oleg, mingo, hpa, torvalds Commit-ID: 76e4c4908a4904a61aa67ae5eb0b2a7588c4a546 Gitweb: http://git.kernel.org/tip/76e4c4908a4904a61aa67ae5eb0b2a7588c4a546 Author: Andy Lutomirski <luto@amacapital.net> AuthorDate: Tue, 10 Mar 2015 11:06:00 -0700 Committer: Ingo Molnar <mingo@kernel.org> 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. Signed-off-by: Andy Lutomirski <luto@amacapital.net> Cc: Borislav Petkov <bp@alien8.de> Cc: Denys Vlasenko <dvlasenk@redhat.com> Cc: H. Peter Anvin <hpa@zytor.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Thomas Gleixner <tglx@linutronix.de> Link: http://lkml.kernel.org/r/b7efc1b7364039824776f68e9ddee9ec1500e894.1426009661.git.luto@amacapital.net Signed-off-by: Ingo Molnar <mingo@kernel.org> --- arch/x86/include/asm/processor.h | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index fc6d8d0..b262089 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -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; ^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-03-17 8:46 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-10 18:05 [PATCH 0/3] sp0, ss1, and sp1 docs and minor fixes Andy Lutomirski 2015-03-10 18:05 ` [PATCH 1/3] x86: Create and use a TOP_OF_KERNEL_STACK_PADDING macro Andy Lutomirski 2015-03-10 19:22 ` Denys Vlasenko 2015-03-10 19:47 ` Andy Lutomirski 2015-03-13 14:08 ` Denys Vlasenko 2015-03-16 8:56 ` Ingo Molnar 2015-03-16 12:08 ` [tip:x86/asm] x86/asm/entry: Create and use a ' TOP_OF_KERNEL_STACK_PADDING' macro tip-bot for Andy Lutomirski 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 2015-03-10 18:05 ` [PATCH 2/3] x86: Unify and fix init sp0 Andy Lutomirski 2015-03-11 11:21 ` Borislav Petkov 2015-03-16 12:09 ` [tip:x86/asm] x86/asm/entry: Unify and fix initial thread_struct: :sp0 values tip-bot for Andy Lutomirski 2015-03-17 8:45 ` tip-bot for Andy Lutomirski 2015-03-10 18:06 ` [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1 Andy Lutomirski 2015-03-10 19:13 ` Denys Vlasenko 2015-03-10 20:06 ` Andy Lutomirski 2015-03-10 20:52 ` Denys Vlasenko 2015-03-16 12:09 ` [tip:x86/asm] x86/asm/entry/32: Document our abuse of x86_hw_tss: :ss1 and x86_hw_tss::sp1 tip-bot for Andy Lutomirski 2015-03-16 15:36 ` Andy Lutomirski 2015-03-17 8:45 ` tip-bot for Andy Lutomirski
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.