Re: [PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key

On 7/6/20 12:50 PM, Michael Ellerman wrote:

"Aneesh Kumar K.V" [off-list ref] writes:

quoted

Use execute_pkey_disabled static key to check for execute key support instead
of pkey_disabled.

Signed-off-by: Aneesh Kumar K.V <redacted>
---
  arch/powerpc/include/asm/pkeys.h | 10 +---------
  arch/powerpc/mm/book3s64/pkeys.c |  5 ++++-
  2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 47c81d41ea9a..09fbaa409ac4 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h

@@ -126,15 +126,7 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
   * Try to dedicate one of the protection keys to be used as an
   * execute-only protection key.
   */
-extern int __execute_only_pkey(struct mm_struct *mm);
-static inline int execute_only_pkey(struct mm_struct *mm)
-{
-	if (static_branch_likely(&pkey_disabled))
-		return -1;
-
-	return __execute_only_pkey(mm);
-}
-
+extern int execute_only_pkey(struct mm_struct *mm);
  extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma,
  					 int prot, int pkey);
  static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,

diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
index bbba9c601e14..fed4f159011b 100644
--- a/arch/powerpc/mm/book3s64/pkeys.c
+++ b/arch/powerpc/mm/book3s64/pkeys.c

@@ -345,8 +345,11 @@ void thread_pkey_regs_init(struct thread_struct *thread)
  	write_uamor(default_uamor);
  }
  
-int __execute_only_pkey(struct mm_struct *mm)
+int execute_only_pkey(struct mm_struct *mm)
  {
+	if (static_branch_likely(&execute_pkey_disabled))
+		return -1;
+
  	return mm->context.execute_only_pkey;
  }

That adds the overhead of a function call, but then uses a static_key to
avoid an easy to predict branch, which seems like a bad tradeoff. And
it's not a performance critical path AFAICS.

Anyway this seems unnecessary:

pkey_early_init_devtree()
{
	...
	if (unlikely(max_pkey <= execute_only_key)) {
		/*
		 * Insufficient number of keys to support
		 * execute only key. Mark it unavailable.
		 */
		execute_only_key = -1;

void pkey_mm_init(struct mm_struct *mm)
{
	...
	mm->context.execute_only_pkey = execute_only_key;
}


ie. Can't it just be:

static inline int execute_only_pkey(struct mm_struct *mm)
{
	return mm->context.execute_only_pkey;
}

ok updated with

modified   arch/powerpc/mm/book3s64/pkeys.c

@@ -151,7 +151,7 @@ void __init pkey_early_init_devtree(void)
  	max_pkey = pkeys_total;
  #endif

-	if (unlikely(max_pkey <= execute_only_key)) {
+	if (unlikely(max_pkey <= execute_only_key) || 

!pkey_execute_disable_supported) {
  		/*
  		 * Insufficient number of keys to support
  		 * execute only key. Mark it unavailable.

@@ -368,9 +368,6 @@ int __arch_set_user_pkey_access(struct task_struct 

*tsk, int pkey,

  int execute_only_pkey(struct mm_struct *mm)
  {
-	if (static_branch_likely(&execute_pkey_disabled))
-		return -1;
-
  	return mm->context.execute_only_pkey;
  }

-aneesh