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

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

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