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

Groups > linux.kernel > #1434760 > unrolled thread

Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit?

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2016-06-30 19:40 +0200
    Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2016-06-30 23:30 +0200
      Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2016-07-01 18:40 +0200

#1434760 — Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit?

On Mon, Dec 21, 2015 at 3:07 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Mon, Dec 21, 2015 at 3:04 PM, Dave Hansen
> <dave.hansen@linux.intel.com> wrote:
>> On 12/18/2015 02:28 PM, Andy Lutomirski wrote:
>> ...
>>>> I could imagine that some kernel person would want to use even more
>>>> keys, but I think two fixed keys are kind of the minimal we'd want to
>>>> use.
>>>
>>> I imagine we'd reserve key 0 for normal page and key 1 for deny-read.
>>> Let me be a bit more concrete about what I'm suggesting:
>>>
>>> We'd have thread_struct.baseline_pkru.  It would start with key 0
>>> allowing all access and key 1 denying reads.
>>
>> Are you sure thread_struct is the right place for this?  I think of
>> signal handlers as a process-wide thing, and it seems a bit goofy if we
>> have the PKRU value in a signal handler depend on the PKRU of the thread
>> that got interrupted.
>
> I think you're right.  mmu_context_t might be a better choice.
>
>>
>>> We'd have a syscall like set_protection_key that could allocate unused
>>> keys and change the values of keys that have been allocated.  Those
>>> changes would be reflected in baseline_pkru.  Changes to keys 0 and 1
>>> in baseline_pkru would not be allowed.
>>
>> FWIW, I think we can do this without *actually* dedicating key 1 to
>> execute-only.  But that's a side issue.
>>
>>> Signal delivery would load baseline_pkru into the PKRU register.
>>> Signal restore would restore PKRU to its previous value.
>>
>> Do you really mean "its previous value" or are you OK with the existing
>> behavior which restores PKRU from the XSAVE buffer in the sigcontext?
>
> By "its previous value" I meant the value in the XSAVE buffer in the
> sigcontext.  So I think I'm okay with that :)
>
>>
>>> WRPKRU would, of course, override baseline_pkru, but it wouldn't
>>> change baseline_pkru.  The set_protection_key syscall would modify
>>> *both* real PKRU and baseline_pkru.
>>
>> How about this:
>>
>> We make baseline_pkru a process-wide baseline and store it in
>> mm->context.  That way, no matter which thread gets interrupted for a
>> signal, they see consistent values.  We only write to it when an app
>> _specifically_ asks for it to be updated with a special flag to
>> sys_pkey_set().
>>
>> When an app uses the execute-only support, we implicitly set the
>> read-disable bit in baseline_pkru for the execute-only pkey.
>
> Sounds good, I think.

Resurrecting an old thread, but:

Looking at your git tree, which I assume is a reasonably approximation
of your current patches, this seems to be unimplemented.  I, at least,
would be nervous about using PKRU for protection of critical data if
signal handlers are unconditionally exempt.

Also, the lazily allocated no-read key for execute-only is done in the
name of performance, but it results in odd semantics.  How much of a
performance win is preserving the init optimization of PKRU in
practice?  (I.e. how much faster are XSAVE and XRSTOR?)  I can't test
because even my Skylake laptop doesn't have PKRU.

--Andy

[toc] | [next] | [standalone]

#1434859

On 06/30/2016 10:36 AM, Andy Lutomirski wrote:
>>> We make baseline_pkru a process-wide baseline and store it in
>>> mm->context.  That way, no matter which thread gets interrupted for a
>>> signal, they see consistent values.  We only write to it when an app
>>> _specifically_ asks for it to be updated with a special flag to
>>> sys_pkey_set().
>>>
>>> When an app uses the execute-only support, we implicitly set the
>>> read-disable bit in baseline_pkru for the execute-only pkey.
...
> Looking at your git tree, which I assume is a reasonably approximation
> of your current patches, this seems to be unimplemented.  I, at least,
> would be nervous about using PKRU for protection of critical data if
> signal handlers are unconditionally exempt.

I actually went along and implemented this using an extra 'flag' for
pkey_get/set().  I just left it out of this stage since I'm having
enough problems getting it in with the existing set of features. :)

I'm confident we can add this later with the flags we can pass to
pkey_get() and pkey_set().

> Also, the lazily allocated no-read key for execute-only is done in the
> name of performance, but it results in odd semantics.  How much of a
> performance win is preserving the init optimization of PKRU in
> practice?  (I.e. how much faster are XSAVE and XRSTOR?)  I can't test
> because even my Skylake laptop doesn't have PKRU.

This is admittedly not the most realistic benchmark because everything
is cache-warm, but I ran Ingo's FPU "measure.c" code on XSAVES/XRSTORS.
This runs things in pretty tight loops where everything is cache hot.

The XSAVE instructions are monsters and I'm not super-confident in my
measurements, but I'm seeing in the neighborhood of XSAVES/XRSTORS
getting 20-30 cycles when PKRU is in play vs. not.  This is with
completely cache-hot data, though.

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

#1435489

On Jun 30, 2016 2:25 PM, "Dave Hansen" <dave.hansen@linux.intel.com> wrote:
>
> On 06/30/2016 10:36 AM, Andy Lutomirski wrote:
> >>> We make baseline_pkru a process-wide baseline and store it in
> >>> mm->context.  That way, no matter which thread gets interrupted for a
> >>> signal, they see consistent values.  We only write to it when an app
> >>> _specifically_ asks for it to be updated with a special flag to
> >>> sys_pkey_set().
> >>>
> >>> When an app uses the execute-only support, we implicitly set the
> >>> read-disable bit in baseline_pkru for the execute-only pkey.
> ...
> > Looking at your git tree, which I assume is a reasonably approximation
> > of your current patches, this seems to be unimplemented.  I, at least,
> > would be nervous about using PKRU for protection of critical data if
> > signal handlers are unconditionally exempt.
>
> I actually went along and implemented this using an extra 'flag' for
> pkey_get/set().  I just left it out of this stage since I'm having
> enough problems getting it in with the existing set of features. :)
>
> I'm confident we can add this later with the flags we can pass to
> pkey_get() and pkey_set().
>
> > Also, the lazily allocated no-read key for execute-only is done in the
> > name of performance, but it results in odd semantics.  How much of a
> > performance win is preserving the init optimization of PKRU in
> > practice?  (I.e. how much faster are XSAVE and XRSTOR?)  I can't test
> > because even my Skylake laptop doesn't have PKRU.
>
> This is admittedly not the most realistic benchmark because everything
> is cache-warm, but I ran Ingo's FPU "measure.c" code on XSAVES/XRSTORS.
> This runs things in pretty tight loops where everything is cache hot.
>
> The XSAVE instructions are monsters and I'm not super-confident in my
> measurements, but I'm seeing in the neighborhood of XSAVES/XRSTORS
> getting 20-30 cycles when PKRU is in play vs. not.  This is with
> completely cache-hot data, though.

That's surprisingly bad, albeit negligible in the grand scheme of
context switches.

But maybe we could optimize differently.  When switching states, mask
out PKRU if it matches between the two states.  This could be messy,
but, if we switch to using WRPKRU directly some day, then the init
optimization becomes moot and this optimization becomes easy.

Hmm.  If we switch to WRPKRU directly and *always* mask out PKRU,
maybe a bunch of your code gets simpler because you won't have to poke
around in the saved state.

Looking forward, I think the xstate partitions into at least three kinds:

1. Pure user state (FPU, etc)

2. User-affecting XSAVES-only state (the CET mess, etc).

3. User accessible state that is needed in user mode *and* kernel
mode.  PKRU is like this.

Type 3 state needs to be switched eagerly.  Types 1 and 2 need to be
*saved* eagerly but not necessarily loaded eagerly.  I still want to
lazy-load it some day.

Do you happen to have WRPKRU cycle counts you can share?

--Andy

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web