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

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

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

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Use execute_pkey_disabled static key to check for execute key support instead
> of pkey_disabled.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  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;
}

cheers

On 7/6/20 12:50 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> Use execute_pkey_disabled static key to check for execute key support instead
>> of pkey_disabled.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   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