Skip to contentSkip to conversation

x86: treat pkey-0 special

PATCH In review Ram Pai 3 participants 7 messages Mar 14, 2018 – Mar 15, 2018

Subsystems (3)
lore
Share & tools
lore

Ram Pai

author
#0 lore raw

PATCH v3 1/1 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 <[email protected]>
cc Michael Ellermen <[email protected]>
cc Ingo Molnar <[email protected]>
---
 arch/x86/include/asm/pkeys.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
arch/x86/include/asm/pkeys.h +3 -2
index a0ba1ff..6ea7486 100644
@@ -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);
Signature · 1 line
1.8.3.1
#1 lore raw
Quoted 7 lines · Ram Pai

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

Quoted 8 lines

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 ...

Quoted 6 lines

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.

Quoted 23 lines
cc Dave Hansen <[email protected]>
cc Michael Ellermen <[email protected]>
cc Ingo Molnar <[email protected]>
---
 arch/x86/include/asm/pkeys.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
arch/x86/include/asm/pkeys.h +1 -1
index a0ba1ff..6ea7486 100644
@@ -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;

arch/x86/include/asm/pkeys.h int mm_pkey_alloc(struct mm_struct *mm) · line 95

Quoted 5 lines
arch/x86/include/asm/pkeys.h +1 -1
@@ -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 */

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

arch/x86/include/asm/pkeys.h int mm_pkey_alloc(struct mm_struct *mm) · line 96

Quoted 1 line
arch/x86/include/asm/pkeys.h +1 -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

#2 lore raw
Quoted 3 lines · Thomas Gleixner

arch/x86/include/asm/pkeys.h int mm_pkey_alloc(struct mm_struct *mm) · line 96

Quoted 1 line
arch/x86/include/asm/pkeys.h +1 -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.

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.

#3 lore raw
Quoted 9 lines · Dave Hansen
Quoted 3 lines · Thomas Gleixner

arch/x86/include/asm/pkeys.h int mm_pkey_alloc(struct mm_struct *mm) · line 96

Quoted 1 line
arch/x86/include/asm/pkeys.h +1 -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.

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.

author
#4 lore raw
In reply to Dave Hansen
Quoted 9 lines · Dave Hansen
Quoted 3 lines · Thomas Gleixner

arch/x86/include/asm/pkeys.h int mm_pkey_alloc(struct mm_struct *mm) · line 96

Quoted 1 line
arch/x86/include/asm/pkeys.h +1 -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.

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

#5 lore raw
Quoted 5 lines · Ram Pai
Quoted 4 lines · Dave Hansen
Quoted 3 lines · Thomas Gleixner

arch/x86/include/asm/pkeys.h int mm_pkey_alloc(struct mm_struct *mm) · line 96

Quoted 1 line
arch/x86/include/asm/pkeys.h +1 -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.

Quoted 7 lines
Quoted 4 lines

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?

author
#6 lore raw
Quoted 10 lines · Dave Hansen
Quoted 9 lines · Ram Pai
Quoted 8 lines · Dave Hansen
Quoted 3 lines · Thomas Gleixner

arch/x86/include/asm/pkeys.h int mm_pkey_alloc(struct mm_struct *mm) · line 96

Quoted 1 line
arch/x86/include/asm/pkeys.h +1 -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.

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.

Quoted 8 lines
Quoted 3 lines

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!

Signature · 1 line
Ram Pai