Ram Pai <linuxram@xxxxxxxxxx> writes:
> On Fri, Jul 28, 2017 at 07:17:13PM -0300, Thiago Jung Bauermann wrote:
>>
>> Ram Pai <linuxram@xxxxxxxxxx> writes:
>> > --- a/arch/powerpc/mm/pkeys.c
>> > +++ b/arch/powerpc/mm/pkeys.c
>> > @@ -97,3 +97,60 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
>> > init_iamr(pkey, new_iamr_bits);
>> > return 0;
>> > }
>> > +
>> > +static inline bool pkey_allows_readwrite(int pkey)
>> > +{
>> > + int pkey_shift = pkeyshift(pkey);
>> > +
>> > + if (!(read_uamor() & (0x3UL << pkey_shift)))
>> > + return true;
>> > +
>> > + return !(read_amr() & ((AMR_RD_BIT|AMR_WR_BIT) << pkey_shift));
>> > +}
>> > +
>> > +int __execute_only_pkey(struct mm_struct *mm)
>> > +{
>> > + bool need_to_set_mm_pkey = false;
>> > + int execute_only_pkey = mm->context.execute_only_pkey;
>> > + int ret;
>> > +
>> > + /* Do we need to assign a pkey for mm's execute-only maps? */
>> > + if (execute_only_pkey == -1) {
>> > + /* Go allocate one to use, which might fail */
>> > + execute_only_pkey = mm_pkey_alloc(mm);
>> > + if (execute_only_pkey < 0)
>> > + return -1;
>> > + need_to_set_mm_pkey = true;
>> > + }
>> > +
>> > + /*
>> > + * We do not want to go through the relatively costly
>> > + * dance to set AMR if we do not need to. Check it
>> > + * first and assume that if the execute-only pkey is
>> > + * readwrite-disabled than we do not have to set it
>> > + * ourselves.
>> > + */
>> > + if (!need_to_set_mm_pkey &&
>> > + !pkey_allows_readwrite(execute_only_pkey))
> ^^^^^
> Here uamor and amr is read once each.
You are right. What confused me was that the call to mm_pkey_alloc above
also reads uamor and amr (and also iamr, and writes to all of those) but
if that function is called, then need_to_set_mm_pkey is true and
pkey_allows_readwrite won't be called.
>> > + return execute_only_pkey;
>> > +
>> > + /*
>> > + * Set up AMR so that it denies access for everything
>> > + * other than execution.
>> > + */
>> > + ret = __arch_set_user_pkey_access(current, execute_only_pkey,
>> > + (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
> ^^^^^^^
> here amr and iamr are written once each if the
> the function returns successfully.
__arch_set_user_pkey_access also reads uamor for the second time in its
call to is_pkey_enabled, and reads amr for the second time as well in
its calls to init_amr. The first reads are in either
pkey_allows_readwrite or pkey_status_change (called from
__arch_activate_pkey).
If need_to_set_mm_pkey is true, then the iamr read in init_iamr is the
2nd one during __execute_only_pkey's execution. In this case the writes
to amr and iamr will be the 2nd ones as well. The first reads and writes
are in pkey_status_change.
>> > + /*
>> > + * If the AMR-set operation failed somehow, just return
>> > + * 0 and effectively disable execute-only support.
>> > + */
>> > + if (ret) {
>> > + mm_set_pkey_free(mm, execute_only_pkey);
> ^^^
> here only if __arch_set_user_pkey_access() fails
> amr and iamr and uamor will be written once each.
I assume the error case isn't perfomance sensitive and didn't account
for mm_set_pkey_free in my analysis.
>> > + return -1;
>> > + }
>> > +
>> > + /* We got one, store it and use it from here on out */
>> > + if (need_to_set_mm_pkey)
>> > + mm->context.execute_only_pkey = execute_only_pkey;
>> > + return execute_only_pkey;
>> > +}
>>
>> If you follow the code flow in __execute_only_pkey, the AMR and UAMOR
>> are read 3 times in total, and AMR is written twice. IAMR is read and
>> written twice. Since they are SPRs and access to them is slow (or isn't
>> it?), is it worth it to read them once in __execute_only_pkey and pass
>> down their values to the callees, and then write them once at the end of
>> the function?
>
> If my calculations are right:
> uamor may be read once and may be written once.
> amr may be read once and is written once.
> iamr is written once.
> So not that bad, i think.
If I'm following the code correctly:
if need_to_set_mm_pkey = true:
uamor is read twice and written once.
amr is read twice and written twice.
iamr is read twice and written twice.
if need_to_set_mm_pkey = false:
uamor is read twice.
amr is read once or twice (depending on the value of uamor) and written once.
iamr is read once and written once.
--
Thiago Jung Bauermann
IBM Linux Technology Center
