"Aneesh Kumar K.V" <[email protected]> writes:
> On 7/6/20 12:34 PM, Michael Ellerman wrote:
>> "Aneesh Kumar K.V" <[email protected]> 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
