lkml.org 
[lkml]   [2017]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFC v6 21/62] powerpc: introduce execute-only pkey
    Date

    Ram Pai <linuxram@us.ibm.com> writes:

    > On Fri, Jul 28, 2017 at 07:17:13PM -0300, Thiago Jung Bauermann wrote:
    >>
    >> Ram Pai <linuxram@us.ibm.com> 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

    \
     
     \ /
      Last update: 2017-07-31 18:20    [W:2.603 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site