[PATCH v3] x86: treat pkey-0 special

Applications need the ability to associate an address-range with some
key and latter revert to its initial default key. Pkey-0 comes close to
providing this function but falls short, because the current
implementation disallows applications to explicitly associate pkey-0 to
the address range.

This patch clarifies the semantics of pkey-0 and provides the
corresponding implementation on powerpc.

Pkey-0 is special with the following semantics.
(a) it is implicitly allocated and can never be freed. It always exists.
(b) it is the default key assigned to any address-range.
(c) it can be explicitly associated with any address-range.

Tested on x86_64.

History:
    v3 : added clarification of the semantics of pkey0.
    		-- suggested by Dave Hansen
    v2 : split the patch into two, one for x86 and one for powerpc
    		-- suggested by Michael Ellermen

cc: Dave Hansen <redacted>
cc: Michael Ellermen <mpe@ellerman.id.au>
cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Ram Pai <redacted>
---
 arch/x86/include/asm/pkeys.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index a0ba1ff..6ea7486 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h

@@ -52,7 +52,7 @@ bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
 	 * from pkey_alloc().  pkey 0 is special, and never
 	 * returned from pkey_alloc().
 	 */
-	if (pkey <= 0)
+	if (pkey < 0)
 		return false;
 	if (pkey >= arch_max_pkey())
 		return false;

@@ -92,7 +92,8 @@ int mm_pkey_alloc(struct mm_struct *mm)
 static inline
 int mm_pkey_free(struct mm_struct *mm, int pkey)
 {
-	if (!mm_pkey_is_allocated(mm, pkey))
+	/* pkey 0 is special and can never be freed */
+	if (!pkey || !mm_pkey_is_allocated(mm, pkey))
 		return -EINVAL;
 
 	mm_set_pkey_free(mm, pkey);

-- 
1.8.3.1

On Wed, 14 Mar 2018, Ram Pai wrote:

Applications need the ability to associate an address-range with some
key and latter revert to its initial default key. Pkey-0 comes close to
providing this function but falls short, because the current
implementation disallows applications to explicitly associate pkey-0 to
the address range.

This patch clarifies the semantics of pkey-0 and provides the

grep 'This patch' Documentation/process

corresponding implementation on powerpc.

Pkey-0 is special with the following semantics.
(a) it is implicitly allocated and can never be freed. It always exists.
(b) it is the default key assigned to any address-range.
(c) it can be explicitly associated with any address-range.

Tested on x86_64.

I'm curious how the corresponding implementation on powerpc can be tested
on x86_64. Copy and paste is not enough ...

History:
    v3 : added clarification of the semantics of pkey0.
    		-- suggested by Dave Hansen
    v2 : split the patch into two, one for x86 and one for powerpc
    		-- suggested by Michael Ellermen

Please put the history below the --- seperator. It's not part of the
changelog. That way the tools can discard it when picking up the patch.

This comment is pretty useless. How should anyone figure out whats special
about pkey 0?

+	if (!pkey || !mm_pkey_is_allocated(mm, pkey))

Why this extra check? mm_pkey_is_allocated(mm, 0) should not return true
ever. If it does, then this wants to be fixed.

Thanks,

	tglx

On 03/15/2018 02:46 AM, Thomas Gleixner wrote:

quoted

+	if (!pkey || !mm_pkey_is_allocated(mm, pkey))

Why this extra check? mm_pkey_is_allocated(mm, 0) should not return true
ever. If it does, then this wants to be fixed.

I was thinking that we _do_ actually want it to seem allocated.  It just
get "allocated" implicitly when an mm is created.  I think that will
simplify the code if we avoid treating it specially in as many places as
possible.

On Thu, 15 Mar 2018, Dave Hansen wrote:

On 03/15/2018 02:46 AM, Thomas Gleixner wrote:

quoted

quoted

+	if (!pkey || !mm_pkey_is_allocated(mm, pkey))

Why this extra check? mm_pkey_is_allocated(mm, 0) should not return true
ever. If it does, then this wants to be fixed.

I was thinking that we _do_ actually want it to seem allocated.  It just
get "allocated" implicitly when an mm is created.  I think that will
simplify the code if we avoid treating it specially in as many places as
possible.

That works as well.

On Thu, Mar 15, 2018 at 08:55:31AM -0700, Dave Hansen wrote:

On 03/15/2018 02:46 AM, Thomas Gleixner wrote:

quoted

quoted

+	if (!pkey || !mm_pkey_is_allocated(mm, pkey))

Why this extra check? mm_pkey_is_allocated(mm, 0) should not return true
ever. If it does, then this wants to be fixed.

I was thinking that we _do_ actually want it to seem allocated.  It just
get "allocated" implicitly when an mm is created.  I think that will
simplify the code if we avoid treating it specially in as many places as
possible.

I think, the logic that makes pkey-0 special must to go
in arch-neutral code.   How about checking for pkey-0 in sys_pkey_free()
itself?

RP

On 03/15/2018 10:21 AM, Ram Pai wrote:

On Thu, Mar 15, 2018 at 08:55:31AM -0700, Dave Hansen wrote:

quoted

On 03/15/2018 02:46 AM, Thomas Gleixner wrote:

quoted

quoted

+	if (!pkey || !mm_pkey_is_allocated(mm, pkey))

Why this extra check? mm_pkey_is_allocated(mm, 0) should not return true
ever. If it does, then this wants to be fixed.

I was thinking that we _do_ actually want it to seem allocated.  It just
get "allocated" implicitly when an mm is created.  I think that will
simplify the code if we avoid treating it specially in as many places as
possible.

I think, the logic that makes pkey-0 special must to go
in arch-neutral code.   How about checking for pkey-0 in sys_pkey_free()
itself?

This is for protection against shooting yourself in the foot?  Yes, that
can go in sys_pkey_free().

Does this need manpage and/or selftests updates?

On Thu, Mar 15, 2018 at 10:31:51AM -0700, Dave Hansen wrote:

On 03/15/2018 10:21 AM, Ram Pai wrote:

quoted

On Thu, Mar 15, 2018 at 08:55:31AM -0700, Dave Hansen wrote:

quoted

On 03/15/2018 02:46 AM, Thomas Gleixner wrote:

quoted

quoted

+	if (!pkey || !mm_pkey_is_allocated(mm, pkey))

Why this extra check? mm_pkey_is_allocated(mm, 0) should not return true
ever. If it does, then this wants to be fixed.

I was thinking that we _do_ actually want it to seem allocated.  It just
get "allocated" implicitly when an mm is created.  I think that will
simplify the code if we avoid treating it specially in as many places as
possible.

I think, the logic that makes pkey-0 special must to go
in arch-neutral code.   How about checking for pkey-0 in sys_pkey_free()
itself?

This is for protection against shooting yourself in the foot?  Yes, that
can go in sys_pkey_free().

Does this need manpage and/or selftests updates?

Yes. it needs selftest, manpage and documentation updates too.

Unfortunately I am not getting enough reviewed-by for my selftests
and documentation changes. :-(  Need help!


-- 
Ram Pai