From: Andy Lutomirski <luto@amacapital.net>
To: Denys Vlasenko <dvlasenk@redhat.com>
Cc: X86 ML <x86@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Borislav Petkov <bp@alien8.de>, Oleg Nesterov <oleg@redhat.com>
Subject: Re: [PATCH 3/3] x86_32: Document our abuse of ss1 and sp1
Date: Tue, 10 Mar 2015 13:06:37 -0700 [thread overview]
Message-ID: <CALCETrWLOk=imRQH3r2xq7UviB_Xodq14MMSSht_2aMnjqNSsw@mail.gmail.com> (raw)
In-Reply-To: <54FF4244.5080600@redhat.com>
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
next prev parent reply other threads:[~2015-03-10 20:07 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALCETrWLOk=imRQH3r2xq7UviB_Xodq14MMSSht_2aMnjqNSsw@mail.gmail.com' \
--to=luto@amacapital.net \
--cc=bp@alien8.de \
--cc=dvlasenk@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=oleg@redhat.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.