Re: [PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey

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

On 7/6/20 12:34 PM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> max_pkey now represents max key value that userspace can allocate.
>>

I guess commit message is confusing.

>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/pkeys.h |  7 +++++--
>>   arch/powerpc/mm/book3s64/pkeys.c | 14 +++++++-------
>>   2 files changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
>> index 75d2a2c19c04..652bad7334f3 100644
>> --- a/arch/powerpc/include/asm/pkeys.h
>> +++ b/arch/powerpc/include/asm/pkeys.h
>> @@ -12,7 +12,7 @@
>>   #include <asm/firmware.h>
>>   
>>   DECLARE_STATIC_KEY_FALSE(pkey_disabled);
>> -extern int pkeys_total; /* total pkeys as per device tree */
>> +extern int max_pkey;
>>   extern u32 initial_allocation_mask; /*  bits set for the initially allocated keys */
>>   extern u32 reserved_allocation_mask; /* bits set for reserved keys */
>>   
>> @@ -44,7 +44,10 @@ static inline int vma_pkey(struct vm_area_struct *vma)
>>   	return (vma->vm_flags & ARCH_VM_PKEY_FLAGS) >> VM_PKEY_SHIFT;
>>   }
>>   
>> -#define arch_max_pkey() pkeys_total
>> +static inline int arch_max_pkey(void)
>> +{
>> +	return max_pkey;
>> +}
> 
> If pkeys_total = 32 then max_pkey = 31.

we have

#ifdef CONFIG_PPC_4K_PAGES
	/*
	 * The OS can manage only 8 pkeys due to its inability to represent them
	 * in the Linux 4K PTE. Mark all other keys reserved.
	 */
	max_pkey = min(8, pkeys_total);
#else
	max_pkey = pkeys_total;
#endif

so it is 32.

> 
> So we can't just substitute one for the other. ie. arch_max_pkey() must
> have been wrong, or it is wrong now.
> 
>> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
>> index 87d882a9aaf2..a4d7287082a8 100644
>> --- a/arch/powerpc/mm/book3s64/pkeys.c
>> +++ b/arch/powerpc/mm/book3s64/pkeys.c
>> @@ -14,7 +14,7 @@
>>   
>>   DEFINE_STATIC_KEY_FALSE(pkey_disabled);
>>   DEFINE_STATIC_KEY_FALSE(execute_pkey_disabled);
>> -int  pkeys_total;		/* Total pkeys as per device tree */
>> +int  max_pkey;			/* Maximum key value supported */
>>   u32  initial_allocation_mask;   /* Bits set for the initially allocated keys */
>>   /*
>>    *  Keys marked in the reservation list cannot be allocated by  userspace
>> @@ -84,7 +84,7 @@ static int scan_pkey_feature(void)
>>   
>>   static int pkey_initialize(void)
>>   {
>> -	int os_reserved, i;
>> +	int pkeys_total, i;
>>   
>>   	/*
>>   	 * We define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
>> @@ -122,12 +122,12 @@ static int pkey_initialize(void)
>>   	 * The OS can manage only 8 pkeys due to its inability to represent them
>>   	 * in the Linux 4K PTE. Mark all other keys reserved.
>>   	 */
>> -	os_reserved = pkeys_total - 8;
>> +	max_pkey = min(8, pkeys_total);
> 
> Shouldn't it be 7 ?
> 
>>   #else
>> -	os_reserved = 0;
>> +	max_pkey = pkeys_total;
>>   #endif
>>   
>> -	if (unlikely((pkeys_total - os_reserved) <= execute_only_key)) {
>> +	if (unlikely(max_pkey <= execute_only_key)) {
> 
> Isn't that an off-by-one now?
> 
> This is one-off boot time code, there's no need to clutter it with
> unlikely.
> 
>>   		/*
>>   		 * Insufficient number of keys to support
>>   		 * execute only key. Mark it unavailable.
>> @@ -174,10 +174,10 @@ static int pkey_initialize(void)
>>   	default_uamor &= ~(0x3ul << pkeyshift(1));
>>   
>>   	/*
>> -	 * Prevent the usage of OS reserved the keys. Update UAMOR
>> +	 * Prevent the usage of OS reserved keys. Update UAMOR
>>   	 * for those keys.
>>   	 */
>> -	for (i = (pkeys_total - os_reserved); i < pkeys_total; i++) {
>> +	for (i = max_pkey; i < pkeys_total; i++) {
> 
> Another off-by-one? Shouldn't we start from max_pkey + 1 ?
> 
>>   		reserved_allocation_mask |= (0x1 << i);
>>   		default_uamor &= ~(0x3ul << pkeyshift(i));
>>   	}
> 

It is the commit message. It is max pkey such that userspace can 
allocate 0 - maxp_pkey -1.

-aneesh