Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1361975 > unrolled thread

Updated version of RD/WR FS/GS BASE patchkit

Back to article view | Back to linux.kernel

Contents

  Updated version of RD/WR FS/GS BASE patchkit Andi Kleen <andi@firstfloor.org> - 2016-03-21 17:20 +0100
    Re: Updated version of RD/WR FS/GS BASE patchkit Andy Lutomirski <luto@amacapital.net> - 2016-03-21 20:30 +0100
      Re: Updated version of RD/WR FS/GS BASE patchkit Andi Kleen <andi@firstfloor.org> - 2016-03-21 20:50 +0100
        Re: Updated version of RD/WR FS/GS BASE patchkit Andy Lutomirski <luto@amacapital.net> - 2016-03-21 23:10 +0100
          Re: Updated version of RD/WR FS/GS BASE patchkit Andi Kleen <andi@firstfloor.org> - 2016-03-21 23:20 +0100
            Re: Updated version of RD/WR FS/GS BASE patchkit Andy Lutomirski <luto@amacapital.net> - 2016-03-21 23:30 +0100
              Re: Updated version of RD/WR FS/GS BASE patchkit Andi Kleen <andi@firstfloor.org> - 2016-03-21 23:50 +0100
                Re: Updated version of RD/WR FS/GS BASE patchkit Andy Lutomirski <luto@amacapital.net> - 2016-03-21 23:50 +0100
                  Re: Updated version of RD/WR FS/GS BASE patchkit Andy Lutomirski <luto@amacapital.net> - 2016-03-22 00:00 +0100
                    Re: Updated version of RD/WR FS/GS BASE patchkit Andi Kleen <andi@firstfloor.org> - 2016-03-22 00:10 +0100
                  Re: Updated version of RD/WR FS/GS BASE patchkit Andi Kleen <andi@firstfloor.org> - 2016-03-22 00:00 +0100
    Re: Updated version of RD/WR FS/GS BASE patchkit Andi Kleen <andi@firstfloor.org> - 2016-03-21 20:30 +0100
    Re: Updated version of RD/WR FS/GS BASE patchkit Andy Lutomirski <luto@amacapital.net> - 2016-03-21 20:50 +0100

#1361975 — Updated version of RD/WR FS/GS BASE patchkit

This is a reworked version of my older fsgsbase patchkit.
Main changes:
- Ported to new entry/* code, which simplified it somewhat
- Now has a test program
- Fixed ptrace/core dump support
- Better documentation
- Some minor fixes improvement

This adds kernel support for some Intel instructions that
allow fast access to the FS and GS 64bit base. They need
some changes to entry_64.S because they allow user
to fully control the GS base.

Advantages:
- NMIs (and other "paranoid" interrupts) avoid
one RDMSR which makes them faster
- User space can use these instructions, mainly
for efficient context switching with user thread libraries
- Context switches do not need to use MSR writes
anymore to context switch FS/GS base >4GB. This
will speed up applications that have enough thread
local data that it won't fit below 4GB.
- User space can use GS efficiently as an additional
global pointer register

I also included one minor (unrelated) optimization to
disable an unneeded old SWAPGS workaround.

-Andi

[toc] | [next] | [standalone]

#1362080

On Mon, Mar 21, 2016 at 12:03 PM, Andi Kleen <andi@firstfloor.org> wrote:
> On Mon, Mar 21, 2016 at 11:39:07AM -0700, Andy Lutomirski wrote:
>> 4. Does the sigcontext format need to change?
>
> I don't think it needs to be. fs/gs are global state and the
> signal handlers are not likely to change it.
>
> Also there is no default that could be restored.
>
>>
>> For maximum safely, comprehensibility, and sanity, there's an argument
>> to be made that 1a and 2a should leave the state exactly as it started
>> and that 1b and 2b should leave it alone unless percpu bases are in
>> use.  For maximum simplicity of implementation, there's an argument
>> that, if the fs or gs selector is nonzero and the base doesn't match
>> the in-memory descriptor, then the kernel can do whatever it wants.
>>
>> I propose the following semantics:
>
> So you want to change the existing semantics. We had this discussion
> before. I think it is out of scope of my patch, which just extends the
> existing semantics to support the instructions.
>
> (what happened in the system call before is now possible in ring 3)
>
> If you want to invent some new overengineered semantics you can do it in some
> followon patch.
>
> Personally i think it is pointless. The existing semantics are fine.

I strongly disagree.

You're adding an hwcap bit because you expect user code to use this
thing, which means you're adding an ABI, which means that the
semantics should be given due consideration.

If the goal were just to speed up context switches, then just maybe it
would make sense to enable it in such a way that user code *doesn't*
use it.  But Intel wasn't nice enough to let us switch the bit for
CPL0 only, and you added an hwcap bit, so here we are...

--Andy

[toc] | [prev] | [next] | [standalone]

#1362114

> You're adding an hwcap bit because you expect user code to use this
> thing, which means you're adding an ABI, which means that the
> semantics should be given due consideration.

Right I did that and concluded the existing semantics are fine.
They also worked fine for many years with the system call.

We have two different modi:

- Code uses old FS/GS selector, gs selector is not zero 
In this case the selector base in GDT/LDT takes preference.

This is legacy, but still works fine.

- Code uses 64bit base, either through arch_prctl or the new
instructions. In this case FS/GS selector has to be zero.

This is the new expected mode for 64bit code.

With the new instructions the modi can be temporarily 
out of sync (GS/FS != 0, but a different base loaded),
but will always be reset on the next context switch.

Your previous objection was that this allows to detect
context switches, but that's already possible in other
ways so I think it's a red hering.

Also if you really want to change it you can do so
in a followon patch under your own name.

-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

[toc] | [prev] | [next] | [standalone]

#1362173

On Mon, Mar 21, 2016 at 12:40 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> You're adding an hwcap bit because you expect user code to use this
>> thing, which means you're adding an ABI, which means that the
>> semantics should be given due consideration.
>
> Right I did that and concluded the existing semantics are fine.
> They also worked fine for many years with the system call.
>
> We have two different modi:
>
> - Code uses old FS/GS selector, gs selector is not zero
> In this case the selector base in GDT/LDT takes preference.

In this case the selector base in GDT/LDT is the whole story because
arch_prctl zeroes the selector.

>
> This is legacy, but still works fine.
>
> - Code uses 64bit base, either through arch_prctl or the new
> instructions. In this case FS/GS selector has to be zero.
>
> This is the new expected mode for 64bit code.
>
> With the new instructions the modi can be temporarily
> out of sync (GS/FS != 0, but a different base loaded),
> but will always be reset on the next context switch.
>
> Your previous objection was that this allows to detect
> context switches, but that's already possible in other
> ways so I think it's a red hering.
>
> Also if you really want to change it you can do so
> in a followon patch under your own name.

ARCH_SET_FS and ARCH_SET_GS *zero the selector*.  WRFSBASE and
WRGSBASE *do not zero the selector*.  This design is, in my mind,
obnoxious and represents an error on Intel's part, but it's what the
docs say the cpu does and I have no reason to doubt the docs.

So a patchset to enable these asinine new instructions needs to take
this into account, and the ABI issue needs to be addressed, even if
the answer is that the proposed code is fine.

(Also, the existing code is fscked up.  Guess what xor %eax, %eax; mov
%ax, %gs does to the base on AMD?  The existing code is *wrong*, and I
don't want to see it get wronger.)

And no, I don't really care about programs detecting context switches.
I do, however, care about allowing non-determinism in things that
ought to behave deterministically.  Writing a nonzero value to %gs and
then doing WRGSBASE is something that user code will be able to do
whether we like it or not, some shitty threading library is likely to
do this just to spite us, the the kernel needs to do *something* when
this happens.

[toc] | [prev] | [next] | [standalone]

#1362178

> So a patchset to enable these asinine new instructions needs to take
> this into account, and the ABI issue needs to be addressed, even if

What's the ABI issue?

AFAIK we're perfectly consistent.

> the answer is that the proposed code is fine.
> 
> (Also, the existing code is fscked up.  Guess what xor %eax, %eax; mov
> %ax, %gs does to the base on AMD?  The existing code is *wrong*, and I
> don't want to see it get wronger.)

I have no idea, but changing it is definitely not in scope for my patches.

> 
> And no, I don't really care about programs detecting context switches.
> I do, however, care about allowing non-determinism in things that
> ought to behave deterministically.  Writing a nonzero value to %gs and
> then doing WRGSBASE is something that user code will be able to do
> whether we like it or not, some shitty threading library is likely to
> do this just to spite us, the the kernel needs to do *something* when
> this happens.

They will quickly notice it if there is a problem, so I don't think
we need to worry about that.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

[toc] | [prev] | [next] | [standalone]

#1362181

On Mon, Mar 21, 2016 at 3:11 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> So a patchset to enable these asinine new instructions needs to take
>> this into account, and the ABI issue needs to be addressed, even if
>
> What's the ABI issue?
>
> AFAIK we're perfectly consistent.
>
>> the answer is that the proposed code is fine.
>>
>> (Also, the existing code is fscked up.  Guess what xor %eax, %eax; mov
>> %ax, %gs does to the base on AMD?  The existing code is *wrong*, and I
>> don't want to see it get wronger.)
>
> I have no idea, but changing it is definitely not in scope for my patches.
>
>>
>> And no, I don't really care about programs detecting context switches.
>> I do, however, care about allowing non-determinism in things that
>> ought to behave deterministically.  Writing a nonzero value to %gs and
>> then doing WRGSBASE is something that user code will be able to do
>> whether we like it or not, some shitty threading library is likely to
>> do this just to spite us, the the kernel needs to do *something* when
>> this happens.
>
> They will quickly notice it if there is a problem, so I don't think
> we need to worry about that.

Really?

Imagine that some brilliant lightweight threading library does:

 - set GS to nonzero (by whatever means -- arch_prctl(ARCH_SET_GS,
whatever) on a pre-IVB host followed by migration, some modify_ldt
garbage, simple bloody-mindedness, whatever);
 - WRGSBASE
 - Use GS for a bit

This will work most of the time until it gets unlucky with preemption.
And yes, runtime library authors really do mess up in amazing ways.

It's an issue.  It needs conscious design.

[toc] | [prev] | [next] | [standalone]

#1362190

> Imagine that some brilliant lightweight threading library does:
> 
>  - set GS to nonzero (by whatever means -- arch_prctl(ARCH_SET_GS,
> whatever) on a pre-IVB host followed by migration, some modify_ldt
> garbage, simple bloody-mindedness, whatever);

Migration is only possible when the CPUID flags match.

>  - WRGSBASE
>  - Use GS for a bit
> 
> This will work most of the time until it gets unlucky with preemption.

As soon as a kernel thread or something else schedules the value
will be lost.

> And yes, runtime library authors really do mess up in amazing ways.
> 
> It's an issue.  It needs conscious design.

Ok. So your only objection is the order of the context switch
updates?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

[toc] | [prev] | [next] | [standalone]

#1362193

On Mon, Mar 21, 2016 at 3:41 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> Imagine that some brilliant lightweight threading library does:
>>
>>  - set GS to nonzero (by whatever means -- arch_prctl(ARCH_SET_GS,
>> whatever) on a pre-IVB host followed by migration, some modify_ldt
>> garbage, simple bloody-mindedness, whatever);
>
> Migration is only possible when the CPUID flags match.
>
>>  - WRGSBASE
>>  - Use GS for a bit
>>
>> This will work most of the time until it gets unlucky with preemption.
>
> As soon as a kernel thread or something else schedules the value
> will be lost.
>
>> And yes, runtime library authors really do mess up in amazing ways.
>>
>> It's an issue.  It needs conscious design.
>
> Ok. So your only objection is the order of the context switch
> updates?

No.  My objection is that there needs to be an explicit statement what
the semantics are.  If the agreed-upon semantics are "undefined
behavior if GS != 0 and GSBASE doesn't match the descriptor", so be
it, but this needs to be a conscious decision and needs to be weighed
against the alternatives.

The actual implementation details are just details.  They need to
match the intended semantics, of course.

--Andy

[toc] | [prev] | [next] | [standalone]

#1362198

On Mon, Mar 21, 2016 at 3:52 PM, Andi Kleen <andi@firstfloor.org> wrote:
>> No.  My objection is that there needs to be an explicit statement what
>> the semantics are.  If the agreed-upon semantics are "undefined
>> behavior if GS != 0 and GSBASE doesn't match the descriptor", so be
>> it, but this needs to be a conscious decision and needs to be weighed
>> against the alternatives.
>
> Documentation/x86/fsgs.txt already has this statement:
>
>>>>
> Another requirement is that the FS or GS selector has to be zero
> (is normally true unless changed explicitly). When it is non-zero
> the context switch assumes the bases were loaded through the LDT/GDT,
> and will reload that.
> <<<
>
> Is that sufficient?
>

Maybe.  Are there better options?  Could we, for example, actually try
to preserve the state if this happens?  Would it be worth it?

>>
>> The actual implementation details are just details.  They need to
>> match the intended semantics, of course.
>
> I believe my implementation matches the paragraph above.
>
> -Andi
>
> --
> ak@linux.intel.com -- Speaking for myself only.



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1362202

> Maybe.  Are there better options?  Could we, for example, actually try
> to preserve the state if this happens? 

We probably could, at the cost of making the context switch a bit 
more expensive.

> Would it be worth it?

I doubt it.

I expect once WR*BASE is widely used noone will bother with selectors
for 64bit programs anymore.

-Andi

[toc] | [prev] | [next] | [standalone]

#1362199

> No.  My objection is that there needs to be an explicit statement what
> the semantics are.  If the agreed-upon semantics are "undefined
> behavior if GS != 0 and GSBASE doesn't match the descriptor", so be
> it, but this needs to be a conscious decision and needs to be weighed
> against the alternatives.

Documentation/x86/fsgs.txt already has this statement:

>>>
Another requirement is that the FS or GS selector has to be zero
(is normally true unless changed explicitly). When it is non-zero
the context switch assumes the bases were loaded through the LDT/GDT,
and will reload that.
<<<

Is that sufficient?

> 
> The actual implementation details are just details.  They need to
> match the intended semantics, of course.

I believe my implementation matches the paragraph above.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

[toc] | [prev] | [next] | [standalone]

#1362090

On Mon, Mar 21, 2016 at 11:39:07AM -0700, Andy Lutomirski wrote:
> 4. Does the sigcontext format need to change?

I don't think it needs to be. fs/gs are global state and the
signal handlers are not likely to change it.

Also there is no default that could be restored.

> 
> For maximum safely, comprehensibility, and sanity, there's an argument
> to be made that 1a and 2a should leave the state exactly as it started
> and that 1b and 2b should leave it alone unless percpu bases are in
> use.  For maximum simplicity of implementation, there's an argument
> that, if the fs or gs selector is nonzero and the base doesn't match
> the in-memory descriptor, then the kernel can do whatever it wants.
> 
> I propose the following semantics:

So you want to change the existing semantics. We had this discussion
before. I think it is out of scope of my patch, which just extends the
existing semantics to support the instructions.

(what happened in the system call before is now possible in ring 3)

If you want to invent some new overengineered semantics you can do it in some
followon patch.

Personally i think it is pointless. The existing semantics are fine.

> Does all this make sense?  Do people agree with me?

I think you are overcomplicated something fundamentally simple.


-Andi
-- 
ak@linux.intel.com -- Speaking for myself only.

[toc] | [prev] | [next] | [standalone]

#1362118

On Mon, Mar 21, 2016 at 9:16 AM, Andi Kleen <andi@firstfloor.org> wrote:
> This is a reworked version of my older fsgsbase patchkit.
> Main changes:
> - Ported to new entry/* code, which simplified it somewhat
> - Now has a test program
> - Fixed ptrace/core dump support
> - Better documentation
> - Some minor fixes improvement

I think that the biggest remaining issue is to define the semantics.

As an architectural matter, the relevant user state is (fs selector,
fs base, gs selector, gs base).  With FSGSBASE enabled, user code can
more or less independently control all four of those values.  (It's
slightly more complicated than that because set_thread_area and
modify_ldt both forget to reload segment registers IIRC, but we can
fix that independently.)

Keeping in mind that we'll probably want to add percpu segment bases
at some point (to allow very fast atomic percpu data access for user
code), the questions I have are:

1a. What happens when a task switches out and back in on the same CPU?

1b. What happens when a task switches out and back in on a different CPU?

2a. What happens when a tracer reads the state out and writes exactly
the same thing back in and the task resumes on the CPU it started on?

2b. What happens when a tracer reads the state out and writes exactly
the same thing back in and the task resumes on a different CPU?

3. What happens if fs or gs points to a real descriptor and that
descriptor changes?

4. Does the sigcontext format need to change?

For maximum safely, comprehensibility, and sanity, there's an argument
to be made that 1a and 2a should leave the state exactly as it started
and that 1b and 2b should leave it alone unless percpu bases are in
use.  For maximum simplicity of implementation, there's an argument
that, if the fs or gs selector is nonzero and the base doesn't match
the in-memory descriptor, then the kernel can do whatever it wants.

I propose the following semantics:

 - All "save state" or "report state" events unconditionally save the
base and selector as they actually were in the CPU state.  (Keep it
simple.  Also, with these patches applied, on an FSGSBASE-capable CPU,
selector != 0 is a slow path.)

 - When restoring state, if selector == 0, then the base is restored as it was.

 - When restoring state, if selector != 0, then the base is restored
to whatever the in-memory descriptor says.  (Optionally, down the
road, we could make it so that a save + restore without an intervening
migration, set_thread_area, or modify_ldt would restore the base as it
was.  This would make things more predictable.)

 - If/when we add percpu bases, they are associated with a nonzero selector.

The big open question is: should signal delivery and restore do
anything to the selectors or bases?  I think that, by default, it
can't, but maybe we'll want an option to do it some day.

Does all this make sense?  Do people agree with me?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web