[PATCH 0/5] Pkey User clean up patches

From: Ira Weiny <redacted>

I'm looking for acks that this is acceptable for official submission to the
maintainers.  I believe the code to be better than RFC quality but I realize
that the type changes may be more churn than is desired.

The following patches contain pkey cleanups and an attempt to standardize on
the type used for pkeys.

The PKS code is standardizing on u8 for the pkey type and for some of the call
paths in the user space code this should work as well.

Ira Weiny (5):
x86/pkeys: Clean up arch_set_user_pkey_access() declaration
x86/pkeys: Remove __arch_set_user_pkey_access() declaration
powerpc/pkeys: Properly type pkey in init_{i}amr()
mm/pkeys: Make pkey unsigned in arch_set_user_pkey_access()
x86/pkeys: Standardize on u8 for pkey type

arch/powerpc/include/asm/pkeys.h | 4 ++--
arch/powerpc/mm/book3s64/pkeys.c | 6 +++---
arch/x86/include/asm/pgtable.h | 4 ++--
arch/x86/include/asm/pkeys.h | 12 ++----------
arch/x86/include/asm/pkru.h | 4 ++--
arch/x86/kernel/fpu/xstate.c | 2 +-
include/linux/pkeys.h | 2 +-
7 files changed, 13 insertions(+), 21 deletions(-)

--
2.35.1

From: Ira Weiny <redacted>

arch_set_user_pkey_access() was declared two times in the header.

Remove the 2nd declaration.

Suggested-by: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Signed-off-by: Ira Weiny <redacted>
---
 arch/x86/include/asm/pkeys.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 1d5f14aff5f6..9c530530b9a7 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h

@@ -118,8 +118,6 @@ int mm_pkey_free(struct mm_struct *mm, int pkey)
 	return 0;
 }
 
-extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
-		unsigned long init_val);
 extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		unsigned long init_val);
 

-- 
2.35.1

From: Ira Weiny <redacted>

In the x86 code __arch_set_user_pkey_access() is not used and is not
defined.

Remove the dead declaration.

To: Dave Hansen <redacted>
Signed-off-by: Ira Weiny <redacted>

---
Changes from V1:
	Make this part of a series of pkey clean ups
---
 arch/x86/include/asm/pkeys.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 9c530530b9a7..2e6c04d8a45b 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h

@@ -41,9 +41,6 @@ static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
 	return __arch_override_mprotect_pkey(vma, prot, pkey);
 }
 
-extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
-		unsigned long init_val);
-
 #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | VM_PKEY_BIT3)
 
 #define mm_pkey_allocation_map(mm)	(mm->context.pkey_allocation_map)

@@ -118,9 +115,6 @@ int mm_pkey_free(struct mm_struct *mm, int pkey)
 	return 0;
 }
 
-extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
-		unsigned long init_val);
-
 static inline int vma_pkey(struct vm_area_struct *vma)
 {
 	unsigned long vma_pkey_mask = VM_PKEY_BIT0 | VM_PKEY_BIT1 |

-- 
2.35.1

From: Ira Weiny <redacted>

The WARN_ON check in arch_set_user_pkey_access() in the x86 architecture
fails to check for an invalid negative value.

A simple check for less than 0 would fix this issue however, in the call
stack below arch_set_user_pkey_access() the pkey should never be
negative on any architecture.  x86 only supports 16 keys while ppc
supports 32, u8 is therefore large enough for all current architectures
and likely those in the future.

Change the type of the pkey passed to arch_set_user_pkey_access() to u8.

To: Dave Hansen <dave.hansen@linux.intel.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Ira Weiny <redacted>

---
Changes from V1:
	Make this part of a generic pkey clean up series.
---
 arch/powerpc/include/asm/pkeys.h | 4 ++--
 arch/powerpc/mm/book3s64/pkeys.c | 2 +-
 arch/x86/include/asm/pkeys.h     | 4 ++--
 arch/x86/kernel/fpu/xstate.c     | 2 +-
 include/linux/pkeys.h            | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 59a2c7dbc78f..e70615a1da9b 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h

@@ -143,9 +143,9 @@ static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
 	return __arch_override_mprotect_pkey(vma, prot, pkey);
 }
 
-extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+extern int __arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
 				       unsigned long init_val);
-static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+static inline int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
 					    unsigned long init_val)
 {
 	if (!mmu_has_feature(MMU_FTR_PKEY))

diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index d6456f8846de..310feb9efd57 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c

@@ -333,7 +333,7 @@ static inline void init_iamr(u8 pkey, u8 init_bits)
  * Set the access rights in AMR IAMR and UAMOR registers for @pkey to that
  * specified in @init_val.
  */
-int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+int __arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
 				unsigned long init_val)
 {
 	u64 new_amr_bits = 0x0ul;

diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 2e6c04d8a45b..3f5c236e34cd 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h

@@ -9,8 +9,8 @@
  */
 #define arch_max_pkey() (cpu_feature_enabled(X86_FEATURE_OSPKE) ? 16 : 1)
 
-extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
-		unsigned long init_val);
+extern int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
+				     unsigned long init_val);
 
 static inline bool arch_pkeys_enabled(void)
 {

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 7c7824ae7862..db511bec57e5 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c

@@ -1068,7 +1068,7 @@ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
  * This will go out and modify PKRU register to set the access
  * rights for @pkey to @init_val.
  */
-int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
 			      unsigned long init_val)
 {
 	u32 old_pkru, new_pkru_bits = 0;

diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 86be8bf27b41..aa40ed2fb0fc 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h

@@ -35,7 +35,7 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
 	return -EINVAL;
 }
 
-static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
+static inline int arch_set_user_pkey_access(struct task_struct *tsk, u8 pkey,
 			unsigned long init_val)
 {
 	return 0;

-- 
2.35.1

From: Ira Weiny <redacted>

Negative values passed to pkeyshift() will cause an overflow of the amr
and imar values.  Pkey should not be negative in this call path and u8
is large enough for the 32 pkeys available on powerpc.

Change pkey to u8 in init_amr() and init_iamr().

To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Aneesh Kumar K.V <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ira Weiny <redacted>
---
 arch/powerpc/mm/book3s64/pkeys.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index 753e62ba67af..d6456f8846de 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c

@@ -310,7 +310,7 @@ void pkey_mm_init(struct mm_struct *mm)
 	mm->context.execute_only_pkey = execute_only_key;
 }
 
-static inline void init_amr(int pkey, u8 init_bits)
+static inline void init_amr(u8 pkey, u8 init_bits)
 {
 	u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
 	u64 old_amr = current_thread_amr() & ~((u64)(0x3ul) << pkeyshift(pkey));

@@ -318,7 +318,7 @@ static inline void init_amr(int pkey, u8 init_bits)
 	current->thread.regs->amr = old_amr | new_amr_bits;
 }
 
-static inline void init_iamr(int pkey, u8 init_bits)
+static inline void init_iamr(u8 pkey, u8 init_bits)
 {
 	u64 new_iamr_bits = (((u64)init_bits & 0x1UL) << pkeyshift(pkey));
 	u64 old_iamr = current_thread_iamr() & ~((u64)(0x1ul) << pkeyshift(pkey));

-- 
2.35.1

From: Ira Weiny <redacted>

The number of pkeys supported on x86 and powerpc are much smaller than a
u16 value can hold.  It is desirable to standardize on the type for
pkeys.  powerpc currently supports the most pkeys at 32.  u8 is plenty
large for that.

Standardize on the pkey types by changing u16 to u8.

To: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Ira Weiny <redacted>
---
 arch/x86/include/asm/pgtable.h | 4 ++--
 arch/x86/include/asm/pkru.h    | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 8a9432fb3802..cb89f1224d8a 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h

@@ -1357,7 +1357,7 @@ static inline pmd_t pmd_swp_clear_uffd_wp(pmd_t pmd)
 }
 #endif /* CONFIG_HAVE_ARCH_USERFAULTFD_WP */
 
-static inline u16 pte_flags_pkey(unsigned long pte_flags)
+static inline u8 pte_flags_pkey(unsigned long pte_flags)
 {
 #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
 	/* ifdef to avoid doing 59-bit shift on 32-bit values */

@@ -1367,7 +1367,7 @@ static inline u16 pte_flags_pkey(unsigned long pte_flags)
 #endif
 }
 
-static inline bool __pkru_allows_pkey(u16 pkey, bool write)
+static inline bool __pkru_allows_pkey(u8 pkey, bool write)
 {
 	u32 pkru = read_pkru();
 

diff --git a/arch/x86/include/asm/pkru.h b/arch/x86/include/asm/pkru.h
index 74f0a2d34ffd..06d088f06229 100644
--- a/arch/x86/include/asm/pkru.h
+++ b/arch/x86/include/asm/pkru.h

@@ -16,13 +16,13 @@ extern u32 init_pkru_value;
 #define pkru_get_init_value()	0
 #endif
 
-static inline bool __pkru_allows_read(u32 pkru, u16 pkey)
+static inline bool __pkru_allows_read(u32 pkru, u8 pkey)
 {
 	int pkru_pkey_bits = pkey * PKRU_BITS_PER_PKEY;
 	return !(pkru & (PKRU_AD_BIT << pkru_pkey_bits));
 }
 
-static inline bool __pkru_allows_write(u32 pkru, u16 pkey)
+static inline bool __pkru_allows_write(u32 pkru, u8 pkey)
 {
 	int pkru_pkey_bits = pkey * PKRU_BITS_PER_PKEY;
 	/*

-- 
2.35.1

On 3/10/22 16:57, ira.weiny@intel.com wrote:

From: Ira Weiny <redacted>

The number of pkeys supported on x86 and powerpc are much smaller than a
u16 value can hold.  It is desirable to standardize on the type for
pkeys.  powerpc currently supports the most pkeys at 32.  u8 is plenty
large for that.

Standardize on the pkey types by changing u16 to u8.

How widely was this intended to "standardize" things?  Looks like it may
have missed a few spots.

Also if we're worried about the type needing to change or with the wrong
type being used, I guess we could just to a pkey_t typedef.

On Mon, Mar 14, 2022 at 04:49:12PM -0700, Dave Hansen wrote:

On 3/10/22 16:57, ira.weiny@intel.com wrote:

quoted

From: Ira Weiny <redacted>

The number of pkeys supported on x86 and powerpc are much smaller than a
u16 value can hold.  It is desirable to standardize on the type for
pkeys.  powerpc currently supports the most pkeys at 32.  u8 is plenty
large for that.

Standardize on the pkey types by changing u16 to u8.

How widely was this intended to "standardize" things?  Looks like it may
have missed a few spots.

Sorry I think the commit message is misleading you.  The justification of u8 as
the proper type is that no arch has a need for more than 255 pkeys.

This specific patch was intended to only change x86.  Per that goal I don't see
any other places in x86 which uses u16 after this patch.

$ git grep u16 arch/x86 | grep key
arch/x86/events/intel/uncore_discovery.c:	const u16 *type_id = key;
arch/x86/include/asm/intel_pconfig.h:	u16 keyid;
arch/x86/include/asm/mmu.h:	u16 pkey_allocation_map;
arch/x86/include/asm/pkeys.h:	u16 all_pkeys_mask = ((1U << arch_max_pkey()) - 1);

Also if we're worried about the type needing to change or with the wrong
type being used, I guess we could just to a pkey_t typedef.

I'm not 'worried' about it.  But I do think it makes the code cleaner and more
self documenting.

Ira

On 3/15/22 08:53, Ira Weiny wrote:

On Mon, Mar 14, 2022 at 04:49:12PM -0700, Dave Hansen wrote:

quoted

On 3/10/22 16:57, ira.weiny@intel.com wrote:

quoted

From: Ira Weiny <redacted>

The number of pkeys supported on x86 and powerpc are much smaller than a
u16 value can hold.  It is desirable to standardize on the type for
pkeys.  powerpc currently supports the most pkeys at 32.  u8 is plenty
large for that.

Standardize on the pkey types by changing u16 to u8.

How widely was this intended to "standardize" things?  Looks like it may
have missed a few spots.

Sorry I think the commit message is misleading you.  The justification of u8 as
the proper type is that no arch has a need for more than 255 pkeys.

This specific patch was intended to only change x86.  Per that goal I don't see
any other places in x86 which uses u16 after this patch.

$ git grep u16 arch/x86 | grep key
arch/x86/events/intel/uncore_discovery.c:	const u16 *type_id = key;
arch/x86/include/asm/intel_pconfig.h:	u16 keyid;
arch/x86/include/asm/mmu.h:	u16 pkey_allocation_map;
arch/x86/include/asm/pkeys.h:	u16 all_pkeys_mask = ((1U << arch_max_pkey()) - 1);

I was also looking at the generic mm code.

quoted

Also if we're worried about the type needing to changY or with the wrong
type being used, I guess we could just to a pkey_t typedef.

I'm not 'worried' about it.  But I do think it makes the code cleaner and more
self documenting.

Yeah, consistency is good.  Do you mind taking a look at how a pkey_t
would look, and also seeing how much core mm code should use it?

On Tue, Mar 15, 2022 at 09:03:26AM -0700, Dave Hansen wrote:

On 3/15/22 08:53, Ira Weiny wrote:

quoted

On Mon, Mar 14, 2022 at 04:49:12PM -0700, Dave Hansen wrote:

quoted

On 3/10/22 16:57, ira.weiny@intel.com wrote:

quoted

From: Ira Weiny <redacted>

The number of pkeys supported on x86 and powerpc are much smaller than a
u16 value can hold.  It is desirable to standardize on the type for
pkeys.  powerpc currently supports the most pkeys at 32.  u8 is plenty
large for that.

Standardize on the pkey types by changing u16 to u8.

How widely was this intended to "standardize" things?  Looks like it may
have missed a few spots.

Sorry I think the commit message is misleading you.  The justification of u8 as
the proper type is that no arch has a need for more than 255 pkeys.

This specific patch was intended to only change x86.  Per that goal I don't see
any other places in x86 which uses u16 after this patch.

$ git grep u16 arch/x86 | grep key
arch/x86/events/intel/uncore_discovery.c:	const u16 *type_id = key;
arch/x86/include/asm/intel_pconfig.h:	u16 keyid;
arch/x86/include/asm/mmu.h:	u16 pkey_allocation_map;
arch/x86/include/asm/pkeys.h:	u16 all_pkeys_mask = ((1U << arch_max_pkey()) - 1);

I was also looking at the generic mm code.

Ah yea that needs to be sorted out too I think.

quoted

quoted

Also if we're worried about the type needing to changY or with the wrong
type being used, I guess we could just to a pkey_t typedef.

I'm not 'worried' about it.  But I do think it makes the code cleaner and more
self documenting.

Yeah, consistency is good.  Do you mind taking a look at how a pkey_t
would look, and also seeing how much core mm code should use it?

I don't mind at all.

Ira