Re: [PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2020-07-06 07:20:56
"Aneesh Kumar K.V" [off-list ref] writes:
quoted hunk ↗ jump to hunk
Use execute_pkey_disabled static key to check for execute key support instead of pkey_disabled. Signed-off-by: Aneesh Kumar K.V <redacted> --- arch/powerpc/include/asm/pkeys.h | 10 +--------- arch/powerpc/mm/book3s64/pkeys.c | 5 ++++- 2 files changed, 5 insertions(+), 10 deletions(-)diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h index 47c81d41ea9a..09fbaa409ac4 100644 --- a/arch/powerpc/include/asm/pkeys.h +++ b/arch/powerpc/include/asm/pkeys.h@@ -126,15 +126,7 @@ static inline int mm_pkey_free(struct mm_struct *mm, int pkey) * Try to dedicate one of the protection keys to be used as an * execute-only protection key. */ -extern int __execute_only_pkey(struct mm_struct *mm); -static inline int execute_only_pkey(struct mm_struct *mm) -{ - if (static_branch_likely(&pkey_disabled)) - return -1; - - return __execute_only_pkey(mm); -} - +extern int execute_only_pkey(struct mm_struct *mm); extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot, int pkey); static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c index bbba9c601e14..fed4f159011b 100644 --- a/arch/powerpc/mm/book3s64/pkeys.c +++ b/arch/powerpc/mm/book3s64/pkeys.c@@ -345,8 +345,11 @@ void thread_pkey_regs_init(struct thread_struct *thread) write_uamor(default_uamor); } -int __execute_only_pkey(struct mm_struct *mm) +int execute_only_pkey(struct mm_struct *mm) { + if (static_branch_likely(&execute_pkey_disabled)) + return -1; + return mm->context.execute_only_pkey; }
That adds the overhead of a function call, but then uses a static_key to
avoid an easy to predict branch, which seems like a bad tradeoff. And
it's not a performance critical path AFAICS.
Anyway this seems unnecessary:
pkey_early_init_devtree()
{
...
if (unlikely(max_pkey <= execute_only_key)) {
/*
* Insufficient number of keys to support
* execute only key. Mark it unavailable.
*/
execute_only_key = -1;
void pkey_mm_init(struct mm_struct *mm)
{
...
mm->context.execute_only_pkey = execute_only_key;
}
ie. Can't it just be:
static inline int execute_only_pkey(struct mm_struct *mm)
{
return mm->context.execute_only_pkey;
}
cheers