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

[Michael Ellerman]

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

And the name.

If it's called max_pkey then it should be the maximum allowed pkey
value.

cheers


>>> 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