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

Groups > linux.kernel > #1294387 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@kernel.org> - 2015-12-18 02:50 +0100
    Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-18 03:20 +0100
      Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-18 03:40 +0100
        Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-18 04:00 +0100
          Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-18 06:30 +0100
            Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? "H. Peter Anvin" <hpa@zytor.com> - 2015-12-18 07:50 +0100
              Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-18 17:10 +0100
                Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-18 18:00 +0100
                Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-18 19:50 +0100
                  Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-18 20:30 +0100
                    Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-18 21:10 +0100
                      Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-18 21:30 +0100
                      Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-18 21:40 +0100
                        Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-18 21:50 +0100
                          Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? "H. Peter Anvin" <hpa@zytor.com> - 2015-12-18 22:00 +0100
                            Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-18 22:10 +0100
                              Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-18 22:10 +0100
                          Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-18 22:10 +0100
                            Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-18 22:20 +0100
                              Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-18 22:50 +0100
                                Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-18 23:30 +0100
                                  Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-19 00:10 +0100
                                    Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-19 00:20 +0100
                                      Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-19 00:30 +0100
                                      Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-21 18:10 +0100
                                        Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-22 00:00 +0100
                                          Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-22 00:10 +0100
                                            Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-22 00:10 +0100
                                              Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-22 00:10 +0100
                                  Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-22 00:10 +0100
                                    Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Andy Lutomirski <luto@amacapital.net> - 2015-12-22 00:10 +0100
                                Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Dave Hansen <dave.hansen@linux.intel.com> - 2015-12-30 00:50 +0100
                            Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Linus Torvalds <torvalds@linux-foundation.org> - 2015-12-18 22:20 +0100
            Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Ingo Molnar <mingo@kernel.org> - 2015-12-18 09:40 +0100
    Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Christoph Hellwig <hch@infradead.org> - 2015-12-18 10:00 +0100
      Re: Rethinking sigcontext's xfeatures slightly for PKRU's benefit? Borislav Petkov <bp@alien8.de> - 2015-12-18 14:00 +0100

Page 1 of 2  [1] 2  Next page →

#1294387 — Rethinking sigcontext's xfeatures slightly for PKRU's benefit?

Hi all-

I think that, for PKRU in particular, we want the default signal
handling behavior to be a bit unusual.

When a signal is delivered, I think we should save the entire xstate
including PKRU.  I see no reason to do anything other than that.

When a signal returns (sigreturn is called), though, I think we should
*not* restore PKRU.  To me, PKRU seems like a global per-thread state,
not something that signal handlers are likely to clobber and should
therefore have restored.  It's also unusual in that it doesn't fit
into the usual xstate feature model of being a bunch of registers that
are mostly caller-saved.

Does this make sense?  Should we do this?

We have _fpx_sw_bytes.xfeatures and _xstate._header.xfeatures.  They
appear to do more or less the same thing.  Could we say that, for
certain new features (e.g. PKRU), if they're not in
_fpx_sw_bytes.xfeatures, then sigreturn will preserve the old content
rather than copying it?  User code that wants to change it on
sigreturn would manually or the feature in to xfeatures, which would
cause the feature to go to its init state if it's not in
_header.xfeatures or to go into the saved state if it is in
_header.xfeatures?

Other means of signaling this could work, too.

It's not all that much code, I think.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1294398

On 12/17/2015 05:48 PM, Andy Lutomirski wrote:
> I think that, for PKRU in particular, we want the default signal
> handling behavior to be a bit unusual.
> 
> When a signal is delivered, I think we should save the entire xstate
> including PKRU.  I see no reason to do anything other than that.

Yep, agreed.

But what about the register state when delivering a signal?  Don't we
set the registers to the init state?  Do we need to preserve PKRU state
instead of init'ing it?  The init state _is_ nice here because it is
permissive allows us to do something useful no matter what PKRU gets set to.

But, if we leave the init state in place when entering a delivering a
signal, we _can't_ decide to (by default at least) preserve the
in-signal state.

> When a signal returns (sigreturn is called), though, I think we should
> *not* restore PKRU.  To me, PKRU seems like a global per-thread state,
> not something that signal handlers are likely to clobber and should
> therefore have restored.  It's also unusual in that it doesn't fit
> into the usual xstate feature model of being a bunch of registers that
> are mostly caller-saved.
> 
> Does this make sense?  Should we do this?

Well, the signal handler isn't necessarily going to clobber it, but the
delivery code already clobbers it when going to the init state.

> We have _fpx_sw_bytes.xfeatures and _xstate._header.xfeatures.  They
> appear to do more or less the same thing.

I thought the _fpx_sw_bytes were only for 32-bit (or FXSAVE/FXRSTOR?).

> Could we say that, for
> certain new features (e.g. PKRU), if they're not in
> _fpx_sw_bytes.xfeatures, then sigreturn will preserve the old content
> rather than copying it?  User code that wants to change it on
> sigreturn would manually or the feature in to xfeatures, which would
> cause the feature to go to its init state if it's not in
> _header.xfeatures or to go into the saved state if it is in
> _header.xfeatures?

I think we first need to decide on the state upon signal delivery.

A practial problem at the moment is that we always call XRSTOR (aka
copy_user_to_xregs()) with RFBM (aka 'mask') with all of the supported
xfeatures.  So RFBM[i]=1 for each state, effectively.  A state with
XSTATE_BV[i]=0 (aka header.xfeatures) and RFBM[i]=1 will init the state.

We'd need to rig up the copy_user_to_xregs() to first read in
header.xfeatures and then or RFBM with it.

Not a huge deal, but something we want to think about, especially as it
pertains to the init/modified optimizations.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1294408

On Thu, Dec 17, 2015 at 6:13 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
> On 12/17/2015 05:48 PM, Andy Lutomirski wrote:
>> I think that, for PKRU in particular, we want the default signal
>> handling behavior to be a bit unusual.
>>
>> When a signal is delivered, I think we should save the entire xstate
>> including PKRU.  I see no reason to do anything other than that.
>
> Yep, agreed.
>
> But what about the register state when delivering a signal?  Don't we
> set the registers to the init state?  Do we need to preserve PKRU state
> instead of init'ing it?  The init state _is_ nice here because it is
> permissive allows us to do something useful no matter what PKRU gets set to.

I think we leave the extended regs alone.  Don't we?

I think that, for most signals, we want to leave PKRU as is,
especially for things that aren't SIGSEGV.  For SIGSEGV, maybe we want
an option to reset PKRU on delivery (and then set the flag to restore
on return?).

In any case, I think there are a decent number of programs out there
that use siglongjmp and therefore never actually hit sigreturn.  They
probably won't restore PKRU, so we shouldn't zero it out when
delivering most signals, I think.

>
> But, if we leave the init state in place when entering a delivering a
> signal, we _can't_ decide to (by default at least) preserve the
> in-signal state.
>
>> When a signal returns (sigreturn is called), though, I think we should
>> *not* restore PKRU.  To me, PKRU seems like a global per-thread state,
>> not something that signal handlers are likely to clobber and should
>> therefore have restored.  It's also unusual in that it doesn't fit
>> into the usual xstate feature model of being a bunch of registers that
>> are mostly caller-saved.
>>
>> Does this make sense?  Should we do this?
>
> Well, the signal handler isn't necessarily going to clobber it, but the
> delivery code already clobbers it when going to the init state.

Can you point to that code?

>
>> We have _fpx_sw_bytes.xfeatures and _xstate._header.xfeatures.  They
>> appear to do more or less the same thing.
>
> I thought the _fpx_sw_bytes were only for 32-bit (or FXSAVE/FXRSTOR?).

I thought they were everywhere.  fpu/signal.c looks that way to me.  I
could be missing something -- this code isn't the most straightforward
in the world.

>
>> Could we say that, for
>> certain new features (e.g. PKRU), if they're not in
>> _fpx_sw_bytes.xfeatures, then sigreturn will preserve the old content
>> rather than copying it?  User code that wants to change it on
>> sigreturn would manually or the feature in to xfeatures, which would
>> cause the feature to go to its init state if it's not in
>> _header.xfeatures or to go into the saved state if it is in
>> _header.xfeatures?
>
> I think we first need to decide on the state upon signal delivery.
>

Agreed.

> A practial problem at the moment is that we always call XRSTOR (aka
> copy_user_to_xregs()) with RFBM (aka 'mask') with all of the supported
> xfeatures.  So RFBM[i]=1 for each state, effectively.  A state with
> XSTATE_BV[i]=0 (aka header.xfeatures) and RFBM[i]=1 will init the state.
>
> We'd need to rig up the copy_user_to_xregs() to first read in
> header.xfeatures and then or RFBM with it.

Indeed.

>
> Not a huge deal, but something we want to think about, especially as it
> pertains to the init/modified optimizations.

Fair point.  FWIW, I don't think that sigreturn performance matters
all that much, so if we inadvertently lose some of the optimizations,
it may not be the end of the world.

I do wonder: are there any modern CPUs on which copy_user_to_xregs (as
opposed to first copying to the task_struct buffer and then
copy_kernel_to_xregs) is ever a win?  It avoids clobbering a few
cachelines and saves some memory bandwidth, but that may come at the
cost of disabling the init/modified optimizations when the subsequent
save on context switch hits a different VA/PA than the
copy_user_to_xregs used.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1294412

On 12/17/2015 06:32 PM, Andy Lutomirski wrote:
> On Thu, Dec 17, 2015 at 6:13 PM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>> But what about the register state when delivering a signal?  Don't we
>> set the registers to the init state?  Do we need to preserve PKRU state
>> instead of init'ing it?  The init state _is_ nice here because it is
>> permissive allows us to do something useful no matter what PKRU gets set to.
> 
> I think we leave the extended regs alone.  Don't we?
> 
> I think that, for most signals, we want to leave PKRU as is,
> especially for things that aren't SIGSEGV.  For SIGSEGV, maybe we want
> an option to reset PKRU on delivery (and then set the flag to restore
> on return?).

Is there some precedent for doing the state differently for different
signals?

>> Well, the signal handler isn't necessarily going to clobber it, but the
>> delivery code already clobbers it when going to the init state.
> 
> Can you point to that code?

handle_signal() -> fpu__clear()

The comment around it says:

"Ensure the signal handler starts with the new fpu state."

>>> We have _fpx_sw_bytes.xfeatures and _xstate._header.xfeatures.  They
>>> appear to do more or less the same thing.
>>
>> I thought the _fpx_sw_bytes were only for 32-bit (or FXSAVE/FXRSTOR?).
> 
> I thought they were everywhere.  fpu/signal.c looks that way to me.  I
> could be missing something -- this code isn't the most straightforward
> in the world.

I think there's some extra space on the ia32 frame for this stuff, but
some clarity from someone who knows the history would be appreciated.

>> Not a huge deal, but something we want to think about, especially as it
>> pertains to the init/modified optimizations.
> 
> Fair point.  FWIW, I don't think that sigreturn performance matters
> all that much, so if we inadvertently lose some of the optimizations,
> it may not be the end of the world.

Once we lose the init optimization, we're sunk for good.  We never get
it back until we restore the init state again.  Having it in the init
state can save writing the state at XSAVE time, which can now save up to
~2k of writes at each context switch.

I'm sure we can preserve it, we just need to be _careful_.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1294439

On Dec 17, 2015 6:53 PM, "Dave Hansen" <dave.hansen@linux.intel.com> wrote:
>
> On 12/17/2015 06:32 PM, Andy Lutomirski wrote:
> > On Thu, Dec 17, 2015 at 6:13 PM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
> >> But what about the register state when delivering a signal?  Don't we
> >> set the registers to the init state?  Do we need to preserve PKRU state
> >> instead of init'ing it?  The init state _is_ nice here because it is
> >> permissive allows us to do something useful no matter what PKRU gets set to.
> >
> > I think we leave the extended regs alone.  Don't we?
> >
> > I think that, for most signals, we want to leave PKRU as is,
> > especially for things that aren't SIGSEGV.  For SIGSEGV, maybe we want
> > an option to reset PKRU on delivery (and then set the flag to restore
> > on return?).
>
> Is there some precedent for doing the state differently for different
> signals?

Yes, to a very limited extent: SA_ONSTACK.

>
> >> Well, the signal handler isn't necessarily going to clobber it, but the
> >> delivery code already clobbers it when going to the init state.
> >
> > Can you point to that code?
>
> handle_signal() -> fpu__clear()
>
> The comment around it says:
>
> "Ensure the signal handler starts with the new fpu state."
>

You win this round :)

So maybe we should have a mask of xfeatures that aren't cleared on
signal delivery (e.g. PKRU, perhaps) and that are, by default,
preserved across signal returns.

> >>> We have _fpx_sw_bytes.xfeatures and _xstate._header.xfeatures.  They
> >>> appear to do more or less the same thing.
> >>
> >> I thought the _fpx_sw_bytes were only for 32-bit (or FXSAVE/FXRSTOR?).
> >
> > I thought they were everywhere.  fpu/signal.c looks that way to me.  I
> > could be missing something -- this code isn't the most straightforward
> > in the world.
>
> I think there's some extra space on the ia32 frame for this stuff, but
> some clarity from someone who knows the history would be appreciated.
>
> >> Not a huge deal, but something we want to think about, especially as it
> >> pertains to the init/modified optimizations.
> >
> > Fair point.  FWIW, I don't think that sigreturn performance matters
> > all that much, so if we inadvertently lose some of the optimizations,
> > it may not be the end of the world.
>
> Once we lose the init optimization, we're sunk for good.  We never get
> it back until we restore the init state again.  Having it in the init
> state can save writing the state at XSAVE time, which can now save up to
> ~2k of writes at each context switch.
>
> I'm sure we can preserve it, we just need to be _careful_.

Right.

How much does XSAVEOPT help here?  IOW if we're careful to save to the
same place we restored from and we don't modify the state in the mean
time, how much of the time do we save?  In the best case, I guess we
save the memory writes but not the reads?

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1294458

On December 17, 2015 9:29:21 PM PST, Andy Lutomirski <luto@amacapital.net> wrote:
>On Dec 17, 2015 6:53 PM, "Dave Hansen" <dave.hansen@linux.intel.com>
>wrote:
>>
>> On 12/17/2015 06:32 PM, Andy Lutomirski wrote:
>> > On Thu, Dec 17, 2015 at 6:13 PM, Dave Hansen
><dave.hansen@linux.intel.com> wrote:
>> >> But what about the register state when delivering a signal?  Don't
>we
>> >> set the registers to the init state?  Do we need to preserve PKRU
>state
>> >> instead of init'ing it?  The init state _is_ nice here because it
>is
>> >> permissive allows us to do something useful no matter what PKRU
>gets set to.
>> >
>> > I think we leave the extended regs alone.  Don't we?
>> >
>> > I think that, for most signals, we want to leave PKRU as is,
>> > especially for things that aren't SIGSEGV.  For SIGSEGV, maybe we
>want
>> > an option to reset PKRU on delivery (and then set the flag to
>restore
>> > on return?).
>>
>> Is there some precedent for doing the state differently for different
>> signals?
>
>Yes, to a very limited extent: SA_ONSTACK.
>
>>
>> >> Well, the signal handler isn't necessarily going to clobber it,
>but the
>> >> delivery code already clobbers it when going to the init state.
>> >
>> > Can you point to that code?
>>
>> handle_signal() -> fpu__clear()
>>
>> The comment around it says:
>>
>> "Ensure the signal handler starts with the new fpu state."
>>
>
>You win this round :)
>
>So maybe we should have a mask of xfeatures that aren't cleared on
>signal delivery (e.g. PKRU, perhaps) and that are, by default,
>preserved across signal returns.
>
>> >>> We have _fpx_sw_bytes.xfeatures and _xstate._header.xfeatures. 
>They
>> >>> appear to do more or less the same thing.
>> >>
>> >> I thought the _fpx_sw_bytes were only for 32-bit (or
>FXSAVE/FXRSTOR?).
>> >
>> > I thought they were everywhere.  fpu/signal.c looks that way to me.
> I
>> > could be missing something -- this code isn't the most
>straightforward
>> > in the world.
>>
>> I think there's some extra space on the ia32 frame for this stuff,
>but
>> some clarity from someone who knows the history would be appreciated.
>>
>> >> Not a huge deal, but something we want to think about, especially
>as it
>> >> pertains to the init/modified optimizations.
>> >
>> > Fair point.  FWIW, I don't think that sigreturn performance matters
>> > all that much, so if we inadvertently lose some of the
>optimizations,
>> > it may not be the end of the world.
>>
>> Once we lose the init optimization, we're sunk for good.  We never
>get
>> it back until we restore the init state again.  Having it in the init
>> state can save writing the state at XSAVE time, which can now save up
>to
>> ~2k of writes at each context switch.
>>
>> I'm sure we can preserve it, we just need to be _careful_.
>
>Right.
>
>How much does XSAVEOPT help here?  IOW if we're careful to save to the
>same place we restored from and we don't modify the state in the mean
>time, how much of the time do we save?  In the best case, I guess we
>save the memory writes but not the reads?
>
>--Andy

I find the notion of PKRU not being initialized at the entry to a signal handler a bit odd.  Saving/restoring it seems like the right thing to me.

What am I missing?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1294997

On Thu, Dec 17, 2015 at 10:43 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On December 17, 2015 9:29:21 PM PST, Andy Lutomirski <luto@amacapital.net> wrote:
>>On Dec 17, 2015 6:53 PM, "Dave Hansen" <dave.hansen@linux.intel.com>
>>wrote:
>>>
>>> On 12/17/2015 06:32 PM, Andy Lutomirski wrote:
>>> > On Thu, Dec 17, 2015 at 6:13 PM, Dave Hansen
>><dave.hansen@linux.intel.com> wrote:
>>> >> But what about the register state when delivering a signal?  Don't
>>we
>>> >> set the registers to the init state?  Do we need to preserve PKRU
>>state
>>> >> instead of init'ing it?  The init state _is_ nice here because it
>>is
>>> >> permissive allows us to do something useful no matter what PKRU
>>gets set to.
>>> >
>>> > I think we leave the extended regs alone.  Don't we?
>>> >
>>> > I think that, for most signals, we want to leave PKRU as is,
>>> > especially for things that aren't SIGSEGV.  For SIGSEGV, maybe we
>>want
>>> > an option to reset PKRU on delivery (and then set the flag to
>>restore
>>> > on return?).
>>>
>>> Is there some precedent for doing the state differently for different
>>> signals?
>>
>>Yes, to a very limited extent: SA_ONSTACK.
>>
>>>
>>> >> Well, the signal handler isn't necessarily going to clobber it,
>>but the
>>> >> delivery code already clobbers it when going to the init state.
>>> >
>>> > Can you point to that code?
>>>
>>> handle_signal() -> fpu__clear()
>>>
>>> The comment around it says:
>>>
>>> "Ensure the signal handler starts with the new fpu state."
>>>
>>
>>You win this round :)
>>
>>So maybe we should have a mask of xfeatures that aren't cleared on
>>signal delivery (e.g. PKRU, perhaps) and that are, by default,
>>preserved across signal returns.
>>

[...]

>
> I find the notion of PKRU not being initialized at the entry to a signal handler a bit odd.  Saving/restoring it seems like the right thing to me.
>
> What am I missing?

On Fri, Dec 18, 2015 at 12:32 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> So the principle is this: signals are conceptually like creating a new thread of
> execution, and that's a very powerful programming concept, like fork() or
> pthread_create() are powerful concepts. So we definitely want to keep that default
> behavior, and I don't think we want to deviate from that for typical new extended
> CPU context features, even if signal delivery slows down as a result.

I think that PKRU is in almost no respect a typical no extended CPU
context feature.  It's a control register, not a data register.

Let's try a concrete example.  One big use case for PKRU will be in
conjunction with DAX-style pmem.  Imagine some program (database
server, perhaps) that mmaps 1 TB of pmem MAP_SHARED, PROT_READ |
PROT_WRITE with protection key 1.  It sets PKRU so that key 1 can't be
written.  All of its accesses to the pmem store are in tight code
regions that do wrpkru; mov; wrpkru (just like what the kernel does
with stac/clac on SMAP systems).

From the app's perspective, it's imperative that stray writes that
coincidentally hit the pmem region fail.  (1 TB is big enough that a
random write to canonical user memory hits it almost 1% of the time.)
This means:

1. If a signal is delivered, the app wants PKRU to be set to some safe
value as early as possible.  The app can do it by itself if it doesn't
use libraries that interfere, but it would be IMO much, much nicer if
the kernel made this happen automatically.

1b. If the app malfunctions such that RSP points to pmem, the kernel
MUST NOT clobber the pmem space.  I think that this basically mandates
that PKRU needs to have some safe state (i.e. definitely not the init
state) on signal delivery: the kernel is going to write a signal frame
at the address identified by RSP, and that address is in pmem, so
those writes need to fail.

2. clone with shared VM should preserve PKRU.

So in this regard, I think signals are kind of like new threads after all.

Now that I think about it more, there are at least two possibilities
that work for this use case.

Option A: signal delivery preserves PKRU.  If we go this route, I
think we should decide whether PKRU is preserved on exit.  Given that
we're talking about adding syscalls to adjust PKRU, it seems a bit odd
to me that sigreturn would, by default, undo whatever those syscalls
do.

Option B: signal delivery resets PKRU to user-specified values.  We're
talking about having syscalls to write PKRU.  We could have clone and
signal entry use the values set by syscall instead of the values in
the actual PKRU register.  That way:

set_protection_key(1, no writes);
...
wrpkru(allow writes to key 1)
mov something    <<<<<<<<<<< fault or async signal here
wrpkru(disallow writes to key 1)

leaves key 1 protected in the signal handler.

If we go that route, I think we must restore PKRU just like any other
xstate on sigreturn, so in some sense it's the simplest of all to
implement.  We'll need to change the signal delivery stuff to do the
fpu__clear and the automatic PKRU write before trying to set up the
signal frame so that the frame is written with the syscall-specified
protections instead of the wrpkru-overridden values, but that should
be easy.


Which reminds me: __get_user, etc all respect PKRU because the SDM
says that PKRU affects all user *and* kernel accesses to user memory
(i.e. anything with _PAGE_USER).  Should get_user_pages also respect
PKRU?

>
> But we've been arguing about 'lightweight signals' for up to two decades that I
> can remember. (The first such suggestion was to not save the FPU state, back when
> FPU saves were ridiculously slow compared to other parts of saving/restoring a
> context.)

I'm not making any claims about lightweightness here.  While
performance is an issue in some cases, I'd like to hash out the
baseline functionality first.

>
> So having a well-enumerated, extensible opt-in mask (which defaults to 'all state
> saved') that allows smart signal handlers to skip the save/restore of certain CPU
> context components would be acceptable I think.

I have a patch that helps considerably with a probably-unnoticeable
ABI change by letting signal delivery use sysret.   I'll dust it off.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295037

On 12/18/2015 08:04 AM, Andy Lutomirski wrote:
> Which reminds me: __get_user, etc all respect PKRU because the SDM
> says that PKRU affects all user *and* kernel accesses to user memory
> (i.e. anything with _PAGE_USER).  Should get_user_pages also respect
> PKRU?

It does.  There are a couple of parts to it, but the most important are
in these patches:

> http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-pkeys.git/commit/?h=pkeys-v018&id=c64acbe036487d07968ea6a5b9090169d6e40160
> http://git.kernel.org/cgit/linux/kernel/git/daveh/x86-pkeys.git/commit/?h=pkeys-v018&id=331c62eb789f969e187964e28ac80b1e0e26b69d


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295093

On 12/18/2015 08:04 AM, Andy Lutomirski wrote:
> 1b. If the app malfunctions such that RSP points to pmem, the kernel
> MUST NOT clobber the pmem space.  I think that this basically mandates
> that PKRU needs to have some safe state (i.e. definitely not the init
> state) on signal delivery: the kernel is going to write a signal frame
> at the address identified by RSP, and that address is in pmem, so
> those writes need to fail.

The kernel is writing the signal frame using normal old copy_to_user().
 Those are writing through mappings with _PAGE_USER set and should be
subject to the PKRU state of the thread before the signal started to be
delivered.

We don't do the fpu__clear() until after this copy, so I think pkeys
enforcement is being done properly for this today.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295107

On Fri, Dec 18, 2015 at 10:42 AM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
> On 12/18/2015 08:04 AM, Andy Lutomirski wrote:
>> 1b. If the app malfunctions such that RSP points to pmem, the kernel
>> MUST NOT clobber the pmem space.  I think that this basically mandates
>> that PKRU needs to have some safe state (i.e. definitely not the init
>> state) on signal delivery: the kernel is going to write a signal frame
>> at the address identified by RSP, and that address is in pmem, so
>> those writes need to fail.
>
> The kernel is writing the signal frame using normal old copy_to_user().
>  Those are writing through mappings with _PAGE_USER set and should be
> subject to the PKRU state of the thread before the signal started to be
> delivered.
>
> We don't do the fpu__clear() until after this copy, so I think pkeys
> enforcement is being done properly for this today.

True, but I think only in a very limited sense.  Your average signal
handler is reasonably like to execute "push $rbp" as its very first
instruction, at which point we're immediately screwed with the current
arrangement.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295120

On 12/18/2015 11:21 AM, Andy Lutomirski wrote:
> On Fri, Dec 18, 2015 at 10:42 AM, Dave Hansen
> <dave.hansen@linux.intel.com> wrote:
>> On 12/18/2015 08:04 AM, Andy Lutomirski wrote:
>>> 1b. If the app malfunctions such that RSP points to pmem, the kernel
>>> MUST NOT clobber the pmem space.  I think that this basically mandates
>>> that PKRU needs to have some safe state (i.e. definitely not the init
>>> state) on signal delivery: the kernel is going to write a signal frame
>>> at the address identified by RSP, and that address is in pmem, so
>>> those writes need to fail.
>>
>> The kernel is writing the signal frame using normal old copy_to_user().
>>  Those are writing through mappings with _PAGE_USER set and should be
>> subject to the PKRU state of the thread before the signal started to be
>> delivered.
>>
>> We don't do the fpu__clear() until after this copy, so I think pkeys
>> enforcement is being done properly for this today.
> 
> True, but I think only in a very limited sense.  Your average signal
> handler is reasonably like to execute "push $rbp" as its very first
> instruction, at which point we're immediately screwed with the current
> arrangement.

I completely agree that there's a window for corruption.

But, I think it's a small one.  Basically, RSP would have to pointing at
a place which was allowed by protection keys for all of the sigframe
setup.  Then, _just_ happened to be at a place which was denied by
protection keys when it enters the signal handler back in userspace.
It's possible, but it's a small window.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295127

On Fri, Dec 18, 2015 at 12:07 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
> On 12/18/2015 11:21 AM, Andy Lutomirski wrote:
>> On Fri, Dec 18, 2015 at 10:42 AM, Dave Hansen
>> <dave.hansen@linux.intel.com> wrote:
>>> On 12/18/2015 08:04 AM, Andy Lutomirski wrote:
>>>> 1b. If the app malfunctions such that RSP points to pmem, the kernel
>>>> MUST NOT clobber the pmem space.  I think that this basically mandates
>>>> that PKRU needs to have some safe state (i.e. definitely not the init
>>>> state) on signal delivery: the kernel is going to write a signal frame
>>>> at the address identified by RSP, and that address is in pmem, so
>>>> those writes need to fail.
>>>
>>> The kernel is writing the signal frame using normal old copy_to_user().
>>>  Those are writing through mappings with _PAGE_USER set and should be
>>> subject to the PKRU state of the thread before the signal started to be
>>> delivered.
>>>
>>> We don't do the fpu__clear() until after this copy, so I think pkeys
>>> enforcement is being done properly for this today.
>>
>> True, but I think only in a very limited sense.  Your average signal
>> handler is reasonably like to execute "push $rbp" as its very first
>> instruction, at which point we're immediately screwed with the current
>> arrangement.
>
> I completely agree that there's a window for corruption.
>
> But, I think it's a small one.  Basically, RSP would have to pointing at
> a place which was allowed by protection keys for all of the sigframe
> setup.  Then, _just_ happened to be at a place which was denied by
> protection keys when it enters the signal handler back in userspace.
> It's possible, but it's a small window.

That's true.

OTOH, I think that the signal in the middle of the wrpkru-allowed
window is also a compelling case.  So maybe my new preference is
option B, where we save and restore PKRU like all the other xfeatures
but, rather than clearing it in the signal context, we set it to the
syscall-specified value.

If we do this, then we'd have to clearly separate the
syscall-specified value from the xstate value, and we'd probably want
one of the syscalls to offer an option to force user PKRU to match the
syscall value.  Maybe your code already does this -- I didn't check.

This has the nice property that sigreturn isn't affected at all except
insofar as we should give a little bit of thought to the ordering of
restoring PKRU relative to any other uaccess.  I'd imagine that we'd
just restore PKRU last, in case the stack we're restoring from has
access disallowed by the saved PKRU value.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295131

On Fri, Dec 18, 2015 at 12:07 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
> But, I think it's a small one.  Basically, RSP would have to pointing at
> a place which was allowed by protection keys for all of the sigframe
> setup.

Note that the whole "stack is special" is not at all a new issue.

It's the main reason why sigaltstack() and SS_ONSTACK exists. This is
in no way new to PKRU, people have had to handle the issue of
stack-related SIGSEGV faults for a long time.

So any application that uses PKRU and may play games that affects the
stack, will always have to have a separate "safe stack" that it uses
for signal handling. But that is in no way PKRU-specific, it's been
the case for a lot of other memory management faults.

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295139

On Fri, Dec 18, 2015 at 12:37 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Dec 18, 2015 at 12:07 PM, Dave Hansen
> <dave.hansen@linux.intel.com> wrote:
>>
>> But, I think it's a small one.  Basically, RSP would have to pointing at
>> a place which was allowed by protection keys for all of the sigframe
>> setup.
>
> Note that the whole "stack is special" is not at all a new issue.
>
> It's the main reason why sigaltstack() and SS_ONSTACK exists. This is
> in no way new to PKRU, people have had to handle the issue of
> stack-related SIGSEGV faults for a long time.
>
> So any application that uses PKRU and may play games that affects the
> stack, will always have to have a separate "safe stack" that it uses
> for signal handling. But that is in no way PKRU-specific, it's been
> the case for a lot of other memory management faults.
>

The trouble is that this isn't just limited to special "safe stack"
SA_ONSTACK aware code.  It even affects normally innocuous things.

The thing that's new is that, before PKRU, the HW stack might have
been unsafe in the sense that accessing it would fault and we need a
different backup stack for some signals.  With PKRU, we might be in a
transient extra-privileged (permissive PKRU) state, and we want to
tighten permissions for *all* signal deliveries so that, in case the
current stack is erroneous, we don't corrupt it.

Admittedly, ending up with permissive PKRU in some short sequence and
corrupt RSP is likely to corrupt things no matter what the kernel
does.  But I don't think we want to deliver SIGALRM to a library while
the main program is in a specially permissive PKRU region.

IOW, I like my idea in which signal delivery always sets PKRU to the
application-requested-by-syscall values and sigreturn restores it.
Kinda like sigaltstack, but applies to all signals and affects PKRU
instead of RSP.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295140

On 12/18/2015 12:49 PM, Andy Lutomirski wrote:
> 
> IOW, I like my idea in which signal delivery always sets PKRU to the
> application-requested-by-syscall values and sigreturn restores it.
> Kinda like sigaltstack, but applies to all signals and affects PKRU
> instead of RSP.
> 

I think this is the only sensible option, with the default being all zero.

	-hpa


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295145

On Fri, Dec 18, 2015 at 12:58 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/18/2015 12:49 PM, Andy Lutomirski wrote:
>>
>> IOW, I like my idea in which signal delivery always sets PKRU to the
>> application-requested-by-syscall values and sigreturn restores it.
>> Kinda like sigaltstack, but applies to all signals and affects PKRU
>> instead of RSP.
>>
>
> I think this is the only sensible option, with the default being all zero.
>

Or not quite all zero if we do Dave's experimental PROT_EXEC thing.

Actually, I want to introduce a set of per-mm "incompatible" bits.  By
default, they'd be zero.  We can, as needed, define bits that do
something nice but break old code.  I want one of the bits to turn
vsyscalls off entirely.  Another bit could say that the kernel is
allowed to steal a protection key for PROT_EXEC.

These bits would be read and written by prctl, but there could also be
an ELF note mechanism to initialize them on execve without a syscall.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295156

On 12/18/2015 01:02 PM, Andy Lutomirski wrote:
> On Fri, Dec 18, 2015 at 12:58 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> > On 12/18/2015 12:49 PM, Andy Lutomirski wrote:
>>> >>
>>> >> IOW, I like my idea in which signal delivery always sets PKRU to the
>>> >> application-requested-by-syscall values and sigreturn restores it.
>>> >> Kinda like sigaltstack, but applies to all signals and affects PKRU
>>> >> instead of RSP.
>>> >>
>> > I think this is the only sensible option, with the default being all zero.
>> >
> Or not quite all zero if we do Dave's experimental PROT_EXEC thing.
> 
> Actually, I want to introduce a set of per-mm "incompatible" bits.  By
> default, they'd be zero.  We can, as needed, define bits that do
> something nice but break old code.  I want one of the bits to turn
> vsyscalls off entirely.  Another bit could say that the kernel is
> allowed to steal a protection key for PROT_EXEC.

That really only makes sense if we have userspace that expects all the
protection keys to be available.  If we go the route of having
pkey_alloc/free() syscalls, then the kernel can easily tell userspace to
keep its mitts off a particular pkey by not ever returning it from a
pkey_alloc().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295149

On Fri, Dec 18, 2015 at 12:49 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> IOW, I like my idea in which signal delivery always sets PKRU to the
> application-requested-by-syscall values and sigreturn restores it.

So I don't mind that, as long as the whole "sigreturn restores it" is
part of things.

Your original email with the suggestion to *not* resture PKRU I didn't
like. Setting it and restoring it is fine.

I do wonder if you need an explicit value, though. I think it's
reasonable to say that PKRU value 0 is special. It's what we'd start
processes with, and why not just say that it's what we run signal
handlers in?

Would any other value ever make sense, really?

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295172

On 12/18/2015 01:04 PM, Linus Torvalds wrote:
> On Fri, Dec 18, 2015 at 12:49 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> IOW, I like my idea in which signal delivery always sets PKRU to the
>> application-requested-by-syscall values and sigreturn restores it.
> 
> So I don't mind that, as long as the whole "sigreturn restores it" is
> part of things.
> 
> Your original email with the suggestion to *not* resture PKRU I didn't
> like. Setting it and restoring it is fine.
> 
> I do wonder if you need an explicit value, though. I think it's
> reasonable to say that PKRU value 0 is special. It's what we'd start
> processes with, and why not just say that it's what we run signal
> handlers in?
> 
> Would any other value ever make sense, really?

Having a PKRU with the execute-only permissions set is the only one I
can think of.  For a system with a _dedicated_ PKEY for execute-only,
this is easy and could even be made a part of init_fpstate with no other
code changes.

But, if we are picking out an execute-only pkey more dynamically, we've
got to keep the default value for the entire process somewhere.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1295201

On Fri, Dec 18, 2015 at 1:12 PM, Dave Hansen
<dave.hansen@linux.intel.com> wrote:
>
> But, if we are picking out an execute-only pkey more dynamically, we've
> got to keep the default value for the entire process somewhere.

How dynamic do we want to make this, though?

I haven't looked at the details, and perhaps more importantly, I don't
know what exactly are the requirements you've gotten from the people
who are expected to actually use this.

I think we might want to hardcode a couple of keys as "kernel
reserved". And I'd rather reserve them up-front than have some user
program be unhappy later when we want to use them.

I guess we want to leave key #0 for "normal page", so my suggesting to
use that for the execute-only was probably misguided.

But I do think we might want to have that "no read access" as a real
fixed key too, because I think the kernel itself would want to use it:

 (a) to make sure that it gets the right fault when user space passes
in a execute-only address to a system call.

 (b) for much more efficient PAGEALLOC_DEBUG for kernel mappings.

so I do think that we'd want to reserve two of the 16 keys up front.

Would it be ok for the expected users to have those keys simply be
fixed? With key 0 being used for all default pages, and key 1 being
used for all execute-only pages? And then defaulting PKRU to 4,
disallowing access to that key #1?

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.

           Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web