From: Dave Hansen <hidden> Date: 2017-12-10 00:21:57
On 12/09/2017 01:16 PM, Florian Weimer wrote:
The attached patch addresses a problem with the current x86 pkey
implementation, which makes default-readable pkeys unusable from signal
handlers because the default init_pkru value blocks access.
Thanks for looking into this!
What do you mean by "default-readable pkeys"?
I think you mean that, for any data that needs to be accessed to enter a
signal handler, it must be set to pkey=0 with the current
implementation. All other keys are inaccessible when entering a signal
handler because the "init" value disables access.
My only nit with this is whether it is the *right* interface. The
signal vs. XSAVE state thing is pretty x86 specific and I doubt that
this will be the last feature that we encounter that needs special
signal behavior.
A question more for the x86 maintainers is whether they would rather see
a pkeys-specific interface for this, or an XSAVE-specific interface
where you could specify a non-init XSAVE state for a set of XSAVE
components.
The attached patch addresses a problem with the current x86 pkey
implementation, which makes default-readable pkeys unusable from signal
handlers because the default init_pkru value blocks access.
Thanks for looking into this!
What do you mean by "default-readable pkeys"?
I think you mean that, for any data that needs to be accessed to enter a
signal handler, it must be set to pkey=0 with the current
implementation. All other keys are inaccessible when entering a signal
handler because the "init" value disables access.
Right, and for keys which are readable (but not writable) most of the
time, so that date is readable, this breaks things.
My only nit with this is whether it is the *right* interface. The
signal vs. XSAVE state thing is pretty x86 specific and I doubt that
this will be the last feature that we encounter that needs special
signal behavior.
The interface is not specific to XSAVE. To generic code, only the two
signal mask manipulation functions are exposed. And I expect that we're
going to need that for other (non-x86) implementations because they will
have the same issue because the signal handler behavior will be identical.
Thanks,
Florian
From: Dave Hansen <hidden> Date: 2017-12-11 16:13:16
On 12/09/2017 10:42 PM, Florian Weimer wrote:
quoted
My only nit with this is whether it is the *right* interface. The
signal vs. XSAVE state thing is pretty x86 specific and I doubt
that this will be the last feature that we encounter that needs
special signal behavior.
The interface is not specific to XSAVE. To generic code, only the
two signal mask manipulation functions are exposed. And I expect
that we're going to need that for other (non-x86) implementations
because they will have the same issue because the signal handler
behavior will be identical.
Let's check with the other implementation...
Ram, this is a question about the signal handler behavior on POWER. I
thought you ended up having different behavior in signal handlers than x86.
In any case, I think the question still stands: Do we want this to be
pkeys-only, or build it so that it can be used for MPX and any future
XSAVE features that need non-init values when entering a signal handler.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
On Mon, Dec 11, 2017 at 08:13:12AM -0800, Dave Hansen wrote:
On 12/09/2017 10:42 PM, Florian Weimer wrote:
quoted
quoted
My only nit with this is whether it is the *right* interface. The
signal vs. XSAVE state thing is pretty x86 specific and I doubt
that this will be the last feature that we encounter that needs
special signal behavior.
The interface is not specific to XSAVE. To generic code, only the
two signal mask manipulation functions are exposed. And I expect
that we're going to need that for other (non-x86) implementations
because they will have the same issue because the signal handler
behavior will be identical.
Let's check with the other implementation...
Ram, this is a question about the signal handler behavior on POWER. I
thought you ended up having different behavior in signal handlers than x86.
On POWER, the value of the pkey_read() i.e contents the AMR
register(pkru equivalent), is always the same regardless of its
context; signal handler or not.
In other words, the permission of any allocated key will not
reset in a signal handler context.
I was not aware that x86 would reset the key permissions in signal
handler. I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
actually be the default behavior.
RP
On POWER, the value of the pkey_read() i.e contents the AMR
register(pkru equivalent), is always the same regardless of its
context; signal handler or not.
In other words, the permission of any allocated key will not
reset in a signal handler context.
That's certainly the simpler semantics, but I don't like how they differ
from x86.
Is the AMR register reset to the original value upon (regular) return
from the signal handler?
I was not aware that x86 would reset the key permissions in signal
handler. I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
actually be the default behavior.
Note that PKEY_ALLOC_SETSIGNAL does something different: It requests
that the kernel sets the access rights for the key to the bits specified
at pkey_alloc time when the signal handler is invoked. So there is
still a reset with PKEY_ALLOC_SETSIGNAL, but to a different value. It
did not occur to me that it might be desirable to avoid resetting the
value on a per-key basis.
Thanks,
Florian
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
On Wed, Dec 13, 2017 at 03:14:36AM +0100, Florian Weimer wrote:
On 12/13/2017 12:13 AM, Ram Pai wrote:
quoted
On POWER, the value of the pkey_read() i.e contents the AMR
register(pkru equivalent), is always the same regardless of its
context; signal handler or not.
In other words, the permission of any allocated key will not
reset in a signal handler context.
That's certainly the simpler semantics, but I don't like how they
differ from x86.
Is the AMR register reset to the original value upon (regular)
return from the signal handler?
The AMR bits are not touched upon (regular) return from the signal
handler.
If the signal handler changes the bits in the AMR, they will continue
to be so, even after return from the signal handler.
To illustrate with an example, lets say AMR value is 'x' and signal
handler is invoked. The value of AMR will be 'x' in the context of the
signal handler. On return from the signal handler the value of AMR will
continue to be 'x'. However if signal handler changes the value of AMR
to 'y', the value of AMR will be 'y' on return from the signal handler.
quoted
I was not aware that x86 would reset the key permissions in signal
handler. I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
actually be the default behavior.
Note that PKEY_ALLOC_SETSIGNAL does something different: It requests
that the kernel sets the access rights for the key to the bits
specified at pkey_alloc time when the signal handler is invoked. So
there is still a reset with PKEY_ALLOC_SETSIGNAL, but to a different
value. It did not occur to me that it might be desirable to avoid
resetting the value on a per-key basis.
Ah. ok i see the subtle difference proposed by your semantics.
Will the following behavior work?
'No bits will be reset to its initial value unless the key has been
allocated with PKEY_ALLOC_*RE*SETSIGNAL flag'.
On Wed, Dec 13, 2017 at 03:14:36AM +0100, Florian Weimer wrote:
quoted
On 12/13/2017 12:13 AM, Ram Pai wrote:
quoted
On POWER, the value of the pkey_read() i.e contents the AMR
register(pkru equivalent), is always the same regardless of its
context; signal handler or not.
In other words, the permission of any allocated key will not
reset in a signal handler context.
That's certainly the simpler semantics, but I don't like how they
differ from x86.
Is the AMR register reset to the original value upon (regular)
return from the signal handler?
The AMR bits are not touched upon (regular) return from the signal
handler.
If the signal handler changes the bits in the AMR, they will continue
to be so, even after return from the signal handler.
To illustrate with an example, lets say AMR value is 'x' and signal
handler is invoked. The value of AMR will be 'x' in the context of the
signal handler. On return from the signal handler the value of AMR will
continue to be 'x'. However if signal handler changes the value of AMR
to 'y', the value of AMR will be 'y' on return from the signal handler.
Okay, this model is really quite different from x86. Is there a good
reason for the difference? Could we change the x86 implementation to
behave in the same way? Or alternatively, change the POWER
implementation to match the existing x86 behavior?
quoted
quoted
I was not aware that x86 would reset the key permissions in signal
handler. I think, the proposed behavior for PKEY_ALLOC_SETSIGNAL should
actually be the default behavior.
Note that PKEY_ALLOC_SETSIGNAL does something different: It requests
that the kernel sets the access rights for the key to the bits
specified at pkey_alloc time when the signal handler is invoked. So
there is still a reset with PKEY_ALLOC_SETSIGNAL, but to a different
value. It did not occur to me that it might be desirable to avoid
resetting the value on a per-key basis.
Ah. ok i see the subtle difference proposed by your semantics.
Will the following behavior work?
'No bits will be reset to its initial value unless the key has been
allocated with PKEY_ALLOC_*RE*SETSIGNAL flag'.
The existing x86 interface defaults to resetting the bits,
unfortunately. I'm not sure if we can or should change this now.
For my purposes, the POWER semantics would work fine as far as I can
see. The reset-to-default is really problematic. I don't actually need
the configurable behavior, but I implemented it this way to achieve a
maximum of backwards compatibility.
Thanks,
Florian
From: Dave Hansen <hidden> Date: 2017-12-13 15:22:04
On 12/13/2017 07:08 AM, Florian Weimer wrote:
Okay, this model is really quite different from x86. Is there a
good reason for the difference?
Yes, both implementations are simple and take the "natural" behavior.
x86 changes XSAVE-controlled register values on entering a signal, so we
let them be changed (including PKRU). POWER hardware does not do this
to its PKRU-equivalent, so we do not force it to.
x86 didn't have to do this for *signals*. But, we kinda went on this
trajectory when we decided to clear/restore FPU state on
entering/exiting signals before XSAVE even existed.
FWIW, I do *not* think we have to do this for future XSAVE states. But,
if we do that, we probably need an interface for apps to tell us which
states to save/restore and which state to set upon entering a signal
handler. That's what I was trying to get you to consider instead of
just a one-off hack to fix this for pkeys.
Okay, this model is really quite different from x86. Is there a
good reason for the difference?
Yes, both implementations are simple and take the "natural" behavior.
x86 changes XSAVE-controlled register values on entering a signal, so we
let them be changed (including PKRU). POWER hardware does not do this
to its PKRU-equivalent, so we do not force it to.
Why? Is there a technical reason not have fully-aligned behavior? Can
POWER at least implement the original PKEY_ALLOC_SETSIGNAL semantics
(reset the access rights for certain keys before switching to the signal
handler) in a reasonably efficient manner?
At the very least, if we add a pkey_alloc flag, it should have identical
behavior on both POWER and x86. So it should either reset the access
rights to a fixed value (as posted) or mask out the PKRU reset on x86
(if that's even possible). In the latter case, the POWER would not even
have to change if we keep saying that the default key behavior (without
the flag) is undefined regarding signal handlers.
x86 didn't have to do this for *signals*. But, we kinda went on this
trajectory when we decided to clear/restore FPU state on
entering/exiting signals before XSAVE even existed.
From a userspace perspective, I find this variance rather
disappointing. It's particularly problematic for something like PKRU,
which comes with an entire set of separately configurable keys. I
implemented a per-key knob, but who says that someone else doesn't need
a per-thread or per-signal knob to switch between these incompatible
behaviors?
What can a library assume regarding pkeys behavior if there are
process-global flags that completely alter certain aspects of their
behavior?
FWIW, I do *not* think we have to do this for future XSAVE states. But,
if we do that, we probably need an interface for apps to tell us which
states to save/restore and which state to set upon entering a signal
handler. That's what I was trying to get you to consider instead of
just a one-off hack to fix this for pkeys.
I get that now.
But for pkeys and their access rights, having this configurable at the
PKRU level (as opposed the individual key level) would completely rule
out any use of pkeys in the glibc dynamic linker.
Thanks,
Florian
On Wed, Dec 13, 2017 at 04:40:11PM +0100, Florian Weimer wrote:
On 12/13/2017 04:22 PM, Dave Hansen wrote:
quoted
On 12/13/2017 07:08 AM, Florian Weimer wrote:
quoted
Okay, this model is really quite different from x86. Is there a
good reason for the difference?
Yes, both implementations are simple and take the "natural" behavior.
x86 changes XSAVE-controlled register values on entering a signal, so we
let them be changed (including PKRU). POWER hardware does not do this
to its PKRU-equivalent, so we do not force it to.
Whuy? Is there a technical reason not have fully-aligned behavior?
Can POWER at least implement the original PKEY_ALLOC_SETSIGNAL
semantics (reset the access rights for certain keys before switching
to the signal handler) in a reasonably efficient manner?
This can be done on POWER. I can also change the behavior on POWER
to exactly match x86; i.e reset the value to init value before
calling the signal handler.
But I think, we should clearly define the default behavior, the behavior
when no flag is specified. Applications tend to rely on default behavior
and expect the most intuitive behavior to be the default behavior.
I tend to think; keeping my biases aside, that the most intuitive
behavior is to preserve access/write permissions of any key, i.e not
reset to the init value. If the application has set the permissions of
a key to some value, it would'nt expect anyone to change them,
irrespective of which context it is in.
RP
On Wed, Dec 13, 2017 at 04:40:11PM +0100, Florian Weimer wrote:
quoted
On 12/13/2017 04:22 PM, Dave Hansen wrote:
quoted
On 12/13/2017 07:08 AM, Florian Weimer wrote:
quoted
Okay, this model is really quite different from x86. Is there a
good reason for the difference?
Yes, both implementations are simple and take the "natural" behavior.
x86 changes XSAVE-controlled register values on entering a signal, so we
let them be changed (including PKRU). POWER hardware does not do this
to its PKRU-equivalent, so we do not force it to.
Whuy? Is there a technical reason not have fully-aligned behavior?
Can POWER at least implement the original PKEY_ALLOC_SETSIGNAL
semantics (reset the access rights for certain keys before switching
to the signal handler) in a reasonably efficient manner?
This can be done on POWER. I can also change the behavior on POWER
to exactly match x86; i.e reset the value to init value before
calling the signal handler.
Maybe we can implement a compromise?
Assuming I got the attached patch right, it implements PKRU inheritance
in signal handlers, similar to what you intend to implement for POWER.
It still restores the PKRU register value upon regular exit from the
signal handler, which I think is something we should keep.
I think we still should add a flag, so that applications can easily
determine if a kernel has this patch. Setting up a signal handler,
sending the signal, and thus checking for inheritance is a bit involved,
and we'd have to do this in the dynamic linker before we can use pkeys
to harden lazy binding. The flag could just be a no-op, apart from the
lack of an EINVAL failure if it is specified.
But I think, we should clearly define the default behavior, the behavior
when no flag is specified. Applications tend to rely on default behavior
and expect the most intuitive behavior to be the default behavior.
Because this feature already shipped on x86, we already have the
unspecified signal handler behavior in the wild, and if applications
need the new, clearly defined semantics, there has to be a way to detect
that the kernel makes this guarantee.
I tend to think; keeping my biases aside, that the most intuitive
behavior is to preserve access/write permissions of any key, i.e not
reset to the init value. If the application has set the permissions of
a key to some value, it would'nt expect anyone to change them,
irrespective of which context it is in.
Sure, it also fixes the siglongjmp issue:
https://sourceware.org/bugzilla/show_bug.cgi?id=22396
If we do not reset the PKRU register on x86 anymore, a non-pkeys-aware
signal handler will not clobber it.
Thanks,
Florian
On Thu, Dec 14, 2017 at 12:21:44PM +0100, Florian Weimer wrote:
On 12/14/2017 01:17 AM, Ram Pai wrote:
quoted
On Wed, Dec 13, 2017 at 04:40:11PM +0100, Florian Weimer wrote:
quoted
On 12/13/2017 04:22 PM, Dave Hansen wrote:
quoted
On 12/13/2017 07:08 AM, Florian Weimer wrote:
quoted
Okay, this model is really quite different from x86. Is there a
good reason for the difference?
Yes, both implementations are simple and take the "natural" behavior.
x86 changes XSAVE-controlled register values on entering a signal, so we
let them be changed (including PKRU). POWER hardware does not do this
to its PKRU-equivalent, so we do not force it to.
Whuy? Is there a technical reason not have fully-aligned behavior?
Can POWER at least implement the original PKEY_ALLOC_SETSIGNAL
semantics (reset the access rights for certain keys before switching
to the signal handler) in a reasonably efficient manner?
This can be done on POWER. I can also change the behavior on POWER
to exactly match x86; i.e reset the value to init value before
calling the signal handler.
Maybe we can implement a compromise?
Assuming I got the attached patch right, it implements PKRU
inheritance in signal handlers, similar to what you intend to
implement for POWER.
Ok.
It still restores the PKRU register value upon
regular exit from the signal handler, which I think is something we
should keep.
On x86, the pkru value is restored, on return from the signal handler,
to the value before the signal handler was called. right?
In other words, if 'x' was the value when signal handler was called, it
will be 'x' when return from the signal handler.
If correct, than it is consistent with the behavior on POWER.
I think we still should add a flag, so that applications can easily
determine if a kernel has this patch. Setting up a signal handler,
sending the signal, and thus checking for inheritance is a bit
involved, and we'd have to do this in the dynamic linker before we
can use pkeys to harden lazy binding. The flag could just be a
no-op, apart from the lack of an EINVAL failure if it is specified.
Sorry. I am little confused. What should I implement on POWER?
PKEY_ALLOC_SETSIGNAL semantics?
Let me know. Thanks for driving this to some consistency.
RP
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
It still restores the PKRU register value upon
regular exit from the signal handler, which I think is something we
should keep.
On x86, the pkru value is restored, on return from the signal handler,
to the value before the signal handler was called. right?
In other words, if 'x' was the value when signal handler was called, it
will be 'x' when return from the signal handler.
If correct, than it is consistent with the behavior on POWER.
That's good to know. I tended to implement the same semantics on x86.
quoted
I think we still should add a flag, so that applications can easily
determine if a kernel has this patch. Setting up a signal handler,
sending the signal, and thus checking for inheritance is a bit
involved, and we'd have to do this in the dynamic linker before we
can use pkeys to harden lazy binding. The flag could just be a
no-op, apart from the lack of an EINVAL failure if it is specified.
Sorry. I am little confused. What should I implement on POWER?
PKEY_ALLOC_SETSIGNAL semantics?
No, we would add a flag, with a different name, and this patch only:
@@ -523,14 +523,17 @@ static int do_mprotect_pkey(unsigned long start,
size_t l
return do_mprotect_pkey(start, len, prot, pkey);
}
+#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SETSIGNAL))
+
SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
{
int pkey;
int ret;
- /* No flags supported yet. */
- if (flags)
+ /* check for unsupported flags */
+ if (flags & ~PKEY_ALLOC_FLAGS)
return -EINVAL;
+
/* check for unsupported init values */
if (init_val & ~PKEY_ACCESS_MASK)
return -EINVAL;
This way, an application can specify the flag during key allocation, and
knows that if the allocation succeeds, the kernel implements access
rights inheritance in signal handlers. I think we need this so that
applications which are incompatible with the earlier x86 implementation
of memory protection keys do not use them.
With my second patch (not the first one implementing
PKEY_ALLOC_SETSIGNAL), no further changes to architecture=specific code
are needed, except for the definition of the flag in the header files.
I'm open to a different way towards conveying this information to
userspace. I don't want to probe for the behavior by sending a signal
because that is quite involved and would also be visible in debuggers,
confusing programmers.
Thanks,
Florian
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
On Sat, Dec 16, 2017 at 04:25:14PM +0100, Florian Weimer wrote:
quoted hunk
On 12/16/2017 04:09 PM, Ram Pai wrote:
quoted
quoted
It still restores the PKRU register value upon
regular exit from the signal handler, which I think is something we
should keep.
On x86, the pkru value is restored, on return from the signal handler,
to the value before the signal handler was called. right?
In other words, if 'x' was the value when signal handler was called, it
will be 'x' when return from the signal handler.
If correct, than it is consistent with the behavior on POWER.
That's good to know. I tended to implement the same semantics on x86.
quoted
quoted
I think we still should add a flag, so that applications can easily
determine if a kernel has this patch. Setting up a signal handler,
sending the signal, and thus checking for inheritance is a bit
involved, and we'd have to do this in the dynamic linker before we
can use pkeys to harden lazy binding. The flag could just be a
no-op, apart from the lack of an EINVAL failure if it is specified.
Sorry. I am little confused. What should I implement on POWER?
PKEY_ALLOC_SETSIGNAL semantics?
No, we would add a flag, with a different name, and this patch only:
@@ -523,14 +523,17 @@ static int do_mprotect_pkey(unsigned long
start, size_t l
return do_mprotect_pkey(start, len, prot, pkey);
}
+#define PKEY_ALLOC_FLAGS ((unsigned long) (PKEY_ALLOC_SETSIGNAL))
+
SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
{
int pkey;
int ret;
- /* No flags supported yet. */
- if (flags)
+ /* check for unsupported flags */
+ if (flags & ~PKEY_ALLOC_FLAGS)
return -EINVAL;
+
/* check for unsupported init values */
if (init_val & ~PKEY_ACCESS_MASK)
return -EINVAL;
This way, an application can specify the flag during key allocation,
and knows that if the allocation succeeds, the kernel implements
access rights inheritance in signal handlers. I think we need this
so that applications which are incompatible with the earlier x86
implementation of memory protection keys do not use them.
With my second patch (not the first one implementing
PKEY_ALLOC_SETSIGNAL), no further changes to architecture=specific
code are needed, except for the definition of the flag in the header
files.
Ok. Sounds like I do not have much to do. My patches in its current form
will continue to work and provide the semantics you envision.
I'm open to a different way towards conveying this information to
userspace. I don't want to probe for the behavior by sending a
signal because that is quite involved and would also be visible in
debuggers, confusing programmers.
Ok. Sounds like I do not have much to do. My patches in its current form
will continue to work and provide the semantics you envision.
Thanks for confirming.
quoted
I'm open to a different way towards conveying this information to
userspace. I don't want to probe for the behavior by sending a
signal because that is quite involved and would also be visible in
debuggers, confusing programmers.
I am fine with your proposal.
So how can we move this forward? Should I submit a single new patch
with the new flag with a more appropriate name (PKEY_ALLOC_SIGNALINHERIT
comes to my mind) and the signal inheritance change?
Dave, do you still want to wait for feedback from the x86 maintainer
regarding a general interface? Is this really feasible without detailed
knowledge of the XSAVE output structure? Otherwise, there probably
isn't a way around code which explicitly copies the bits we want to
preserve from the interrupted CPU context to the signal handler context.
Thanks,
Florian