Ram Pai <linux...@us.ibm.com> writes:
> On Tue, Oct 24, 2017 at 11:55:41AM +0530, Aneesh Kumar K.V wrote:
>> Ram Pai <linux...@us.ibm.com> writes:
>> > +
>> > +static void pkey_status_change(int pkey, bool enable)
>> > +{
>> > + u64 old_uamor;
>> > +
>> > + /* reset the AMR and IAMR bits for this key */
>> > + init_amr(pkey, 0x0);
>> > + init_iamr(pkey, 0x0);
>> > +
>> > + /* enable/disable key */
>> > + old_uamor = read_uamor();
>> > + if (enable)
>> > + old_uamor |= (0x3ul << pkeyshift(pkey));
>> > + else
>> > + old_uamor &= ~(0x3ul << pkeyshift(pkey));
>> > + write_uamor(old_uamor);
>> > +}
>>
>> That one is confusing, we discussed this outside the list, but want to
>> bring the list to further discussion. So now the core kernel request
>> for a key via mm_pkey_alloc(). Why not do the below there
>>
>> static inline int mm_pkey_alloc(struct mm_struct *mm)
>> {
>> /*
>> * Note: this is the one and only place we make sure
>> * that the pkey is valid as far as the hardware is
>> * concerned. The rest of the kernel trusts that
>> * only good, valid pkeys come out of here.
>> */
>> u32 all_pkeys_mask = (u32)(~(0x0));
>> int ret;
>>
>> if (!pkey_inited)
>> return -1;
>> /*
>> * Are we out of pkeys? We must handle this specially
>> * because ffz() behavior is undefined if there are no
>> * zeros.
>> */
>> if (mm_pkey_allocation_map(mm) == all_pkeys_mask)
>> return -1;
>>
>> ret = ffz((u32)mm_pkey_allocation_map(mm));
>> mm_set_pkey_allocated(mm, ret);
>>
>> return ret;
>> }
>>
>> your mm_pkey_allocation_map() should have the keys specified in AMOR and
>> UAMOR marked as allocatied. It is in use by hypervisor and OS respectively.
>>
>>
>> There is no need of __arch_activate_key() etc. and by default if the OS
>> has not requested for a key for its internal use UAMOR should be
>> 0xFFFFFFFF and that AMOR value you derive from the device tree based of
>> what keys hypervisor has reserved.
>
>
> Ok. You are suggesting a programming model where
> a) keys that are reserved by hypervisor are enabled by default.
No he's not saying that.
> b) keys that are reserved by the OS are enabled by default.
Or that.
> c) keys that are not yet allocated to userspace is enabled by default.
But he is saying this.
> The problem with this model is that, the userspace can change the
> permissions of an unallocated key, and those permissions will go into
> effect immediately,
Correct, but an unallocated key should not be used anywhere, so it
should have no effect, unless there's a bug.
> because every key is enabled by default. If it
Not every key, every key that is not reserved by the hypervisor or OS.
> happens to be a key that is reserved by the OS or the hypervisor, bad
> things can happen.
So no nothing bad should be able to happen.
> the programming model that I have implemented; **which is the programming
> model expected by linux**, is
>
> a) keys that are reserved by hypervisor are left to the hypervisor to
> enable/disable whenever it needs to.
We agree on that.
> b) keys that are reserved by the OS are left to the OS to
> enable/disable whenever it needs to.
And that.
> c) keys that are not yet allocated to userspace are not yet enabled.
> Will be enabled when the key is allocated to userspace
> through sys_pkey_alloc() syscall.
This is the only part that is under discussion.
> In this programming model, userspace is expected to use only keys that
> are allocated. And in case it inadvertetly uses a key that is not
> allocated, nothing bad happens because that key is not activated unless
> it is allocated.
This sames like a good design to me.
The only downside I see is we have to switch an extra SPR, but that's
not much in the scheme of things.
cheers
