From: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
To: Nicholas Piggin <npiggin@gmail.com>,
linuxppc-dev@lists.ozlabs.org, mpe@ellerman.id.au
Cc: linuxram@us.ibm.com, bauerman@linux.ibm.com
Subject: Re: [PATCH v4 24/41] powerpc/book3s64/pkeys: Store/restore userspace AMR correctly on entry and exit from kernel
Date: Fri, 3 Jul 2020 15:00:13 +0530 [thread overview]
Message-ID: <83a38d37-099a-7b98-1262-c16b4f5a0cc4@linux.ibm.com> (raw)
In-Reply-To: <1593766495.dqos6wxr3o.astroid@bobo.none>
On 7/3/20 2:48 PM, Nicholas Piggin wrote:
> Excerpts from Aneesh Kumar K.V's message of June 15, 2020 4:14 pm:
>> This prepare kernel to operate with a different value than userspace AMR.
>> For this, AMR needs to be saved and restored on entry and return from the
>> kernel.
>>
>> With KUAP we modify kernel AMR when accessing user address from the kernel
>> via copy_to/from_user interfaces.
>>
>> If MMU_FTR_KEY is enabled we always use the key mechanism to implement KUAP
>> feature. If MMU_FTR_KEY is not supported and if we support MMU_FTR_KUAP
>> (radix translation on POWER9), we can skip restoring AMR on return
>> to userspace. Userspace won't be using AMR in that specific config.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> arch/powerpc/include/asm/book3s/64/kup.h | 141 ++++++++++++++++++-----
>> arch/powerpc/kernel/entry_64.S | 6 +-
>> arch/powerpc/kernel/exceptions-64s.S | 4 +-
>> arch/powerpc/kernel/syscall_64.c | 26 ++++-
>> 4 files changed, 144 insertions(+), 33 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
>> index e6ee1c34842f..6979cd1a0003 100644
>> --- a/arch/powerpc/include/asm/book3s/64/kup.h
>> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
>> @@ -13,18 +13,47 @@
>>
>> #ifdef __ASSEMBLY__
>>
>> -.macro kuap_restore_amr gpr1, gpr2
>> -#ifdef CONFIG_PPC_KUAP
>> +.macro kuap_restore_user_amr gpr1
>> +#if defined(CONFIG_PPC_MEM_KEYS)
>> BEGIN_MMU_FTR_SECTION_NESTED(67)
>> - mfspr \gpr1, SPRN_AMR
>> + /*
>> + * AMR is going to be different when
>> + * returning to userspace.
>> + */
>> + ld \gpr1, STACK_REGS_KUAP(r1)
>> + isync
>> + mtspr SPRN_AMR, \gpr1
>> +
>> + /* No isync required, see kuap_restore_user_amr() */
>> + END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_PKEY , 67)
>> +#endif
>> +.endm
>> +
>> +.macro kuap_restore_kernel_amr gpr1, gpr2
>> +#if defined(CONFIG_PPC_MEM_KEYS)
>> + BEGIN_MMU_FTR_SECTION_NESTED(67)
>> + b 99f // handle_pkey_restore_amr
>> + END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_PKEY , 67)
>> +
>> + BEGIN_MMU_FTR_SECTION_NESTED(68)
>> + b 99f // handle_kuap_restore_amr
>> + MMU_FTR_SECTION_ELSE_NESTED(68)
>> + b 100f // skip_restore_amr
>> + ALT_MMU_FTR_SECTION_END_NESTED_IFSET(MMU_FTR_KUAP, 68)
>> +
>> +99:
>> + /*
>> + * AMR is going to be mostly the same since we are
>> + * returning to the kernel. Compare and do a mtspr.
>> + */
>> ld \gpr2, STACK_REGS_KUAP(r1)
>> + mfspr \gpr1, SPRN_AMR
>> cmpd \gpr1, \gpr2
>> - beq 998f
>> + beq 100f
>> isync
>> mtspr SPRN_AMR, \gpr2
>> /* No isync required, see kuap_restore_amr() */
>> -998:
>> - END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_KUAP, 67)
>> +100: // skip_restore_amr
>
> Can't you code it like this? (_IFCLR requires none of the bits to be
> set)
>
> BEGIN_MMU_FTR_SECTION_NESTED(67)
> b 99f // nothing using AMR, no need to restore
> END_MMU_FTR_SECTION_NESTED_IFCLR(MMU_FTR_PKEY | MMU_FTR_KUAP, 67)
>
> That saves you a branch in the common case of using AMR. Similar
> for others.
Yes i could switch to that. The code is taking extra 200 cycles even
with KUAP/KUEP disabled and no keys being used on hash. I am yet to
analyze this closely. So will rework things based on that analysis.
>
>> @@ -69,22 +133,40 @@
>>
>> extern u64 default_uamor;
>>
>> -static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
>> +static inline void kuap_restore_user_amr(struct pt_regs *regs)
>> {
>> - if (mmu_has_feature(MMU_FTR_KUAP) && unlikely(regs->kuap != amr)) {
>> - isync();
>> - mtspr(SPRN_AMR, regs->kuap);
>> - /*
>> - * No isync required here because we are about to RFI back to
>> - * previous context before any user accesses would be made,
>> - * which is a CSI.
>> - */
>> + if (!mmu_has_feature(MMU_FTR_PKEY))
>> + return;
>
> If you have PKEY but not KUAP, do you still have to restore?
>
Yes, because user space pkey is now set on the exit path. This is needed
to handle things like exec/fork().
>> +
>> + isync();
>> + mtspr(SPRN_AMR, regs->kuap);
>> + /*
>> + * No isync required here because we are about to rfi
>> + * back to previous context before any user accesses
>> + * would be made, which is a CSI.
>> + */
>> +}
>> +
>> +static inline void kuap_restore_kernel_amr(struct pt_regs *regs,
>> + unsigned long amr)
>> +{
>> + if (mmu_has_feature(MMU_FTR_KUAP) || mmu_has_feature(MMU_FTR_PKEY)) {
>> +
>> + if (unlikely(regs->kuap != amr)) {
>> + isync();
>> + mtspr(SPRN_AMR, regs->kuap);
>> + /*
>> + * No isync required here because we are about to rfi
>> + * back to previous context before any user accesses
>> + * would be made, which is a CSI.
>> + */
>> + }
>> }
>> }
>>
>> static inline unsigned long kuap_get_and_check_amr(void)
>> {
>> - if (mmu_has_feature(MMU_FTR_KUAP)) {
>> + if (mmu_has_feature(MMU_FTR_KUAP) || mmu_has_feature(MMU_FTR_PKEY)) {
>> unsigned long amr = mfspr(SPRN_AMR);
>> if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG)) /* kuap_check_amr() */
>> WARN_ON_ONCE(amr != AMR_KUAP_BLOCKED);
>
> We could do a static key that's based on this condition, but that can
> wait for another day.
>
>>
>> static inline void kuap_check_amr(void)
>> {
>> - if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) && mmu_has_feature(MMU_FTR_KUAP))
>> + if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) &&
>> + (mmu_has_feature(MMU_FTR_KUAP) || mmu_has_feature(MMU_FTR_PKEY)))
>> WARN_ON_ONCE(mfspr(SPRN_AMR) != AMR_KUAP_BLOCKED);
>> }
>>
>> #else /* CONFIG_PPC_MEM_KEYS */
>>
>> -static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
>> +static inline void kuap_restore_user_amr(struct pt_regs *regs)
>> +{
>> +}
>> +
>> +static inline void kuap_restore_kernel_amr(struct pt_regs *regs, unsigned long amr)
>> {
>> }
>>
>> @@ -113,6 +200,7 @@ static inline unsigned long kuap_get_and_check_amr(void)
>> {
>> return 0;
>> }
>> +
>> #endif /* CONFIG_PPC_MEM_KEYS */
>>
>>
>> @@ -187,7 +275,6 @@ bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
>> "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
>> }
>> #endif /* CONFIG_PPC_KUAP */
>> -
>> #endif /* __ASSEMBLY__ */
>>
>> #endif /* _ASM_POWERPC_BOOK3S_64_KUP_H */
>
> Unnecessary hunks.
will remove
>
>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>> index 9d49338e0c85..a087cbe0b17d 100644
>> --- a/arch/powerpc/kernel/entry_64.S
>> +++ b/arch/powerpc/kernel/entry_64.S
>> @@ -481,8 +481,8 @@ _ASM_NOKPROBE_SYMBOL(fast_interrupt_return)
>> kuap_check_amr r3, r4
>> ld r5,_MSR(r1)
>> andi. r0,r5,MSR_PR
>> - bne .Lfast_user_interrupt_return
>> - kuap_restore_amr r3, r4
>> + bne .Lfast_user_interrupt_return_amr
>> + kuap_restore_kernel_amr r3, r4
>> andi. r0,r5,MSR_RI
>> li r3,0 /* 0 return value, no EMULATE_STACK_STORE */
>> bne+ .Lfast_kernel_interrupt_return
>> @@ -502,6 +502,8 @@ _ASM_NOKPROBE_SYMBOL(interrupt_return)
>> cmpdi r3,0
>> bne- .Lrestore_nvgprs
>>
>> +.Lfast_user_interrupt_return_amr:
>> + kuap_restore_user_amr r3
>> .Lfast_user_interrupt_return:
>> ld r11,_NIP(r1)
>> ld r12,_MSR(r1)
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index e70ebb5c318c..8226af444d77 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -971,7 +971,7 @@ EXC_COMMON_BEGIN(system_reset_common)
>> ld r10,SOFTE(r1)
>> stb r10,PACAIRQSOFTMASK(r13)
>>
>> - kuap_restore_amr r9, r10
>> + kuap_restore_kernel_amr r9, r10
>> EXCEPTION_RESTORE_REGS
>> RFI_TO_USER_OR_KERNEL
>>
>> @@ -2784,7 +2784,7 @@ EXC_COMMON_BEGIN(soft_nmi_common)
>> ld r10,SOFTE(r1)
>> stb r10,PACAIRQSOFTMASK(r13)
>>
>> - kuap_restore_amr r9, r10
>> + kuap_restore_kernel_amr r9, r10
>> EXCEPTION_RESTORE_REGS hsrr=0
>> RFI_TO_KERNEL
>>
>> diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
>> index 7e560a01afa4..fded67982fbe 100644
>> --- a/arch/powerpc/kernel/syscall_64.c
>> +++ b/arch/powerpc/kernel/syscall_64.c
>> @@ -35,7 +35,21 @@ notrace long system_call_exception(long r3, long r4, long r5,
>> BUG_ON(!FULL_REGS(regs));
>> BUG_ON(regs->softe != IRQS_ENABLED);
>>
>> - kuap_check_amr();
>> +#ifdef CONFIG_PPC_MEM_KEYS
>> + if (mmu_has_feature(MMU_FTR_PKEY)) {
>> + unsigned long amr;
>> + /*
>> + * When entering from userspace we mostly have the AMR
>> + * different from kernel default values. Hence don't compare.
>> + */
>> + amr = mfspr(SPRN_AMR);
>> + regs->kuap = amr;
>> + if (mmu_has_feature(MMU_FTR_KUAP))
>> + mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
>> + isync();
>
> isync should be inside the if(). Again do pkeys need to save this if
> KUAP is not being used? I haven't really looked at how all that works,
> but what's changing for the PKEY && !KUAP case?
>
There is no SPR switch in context switch now and all the AMR/IAMR
handling is now in the exit to userspace.
> This would be nice if it could all go into a wrapper function rather
> than ifdef.
>
will do
>> + } else
>> +#endif
>> + kuap_check_amr();
>>
>> account_cpu_user_entry();
>>
>> @@ -222,6 +236,10 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>
>> account_cpu_user_exit();
>>
>> + /*
>> + * We do this at the end so that we do context switch with KERNEL AMR
>> + */
>> + kuap_restore_user_amr(regs);
>> return ret;
>
> Comment doesn't make sense, newline required before return.
Ok the detail there was we need to make sure we restore AMR towrads the
end and make sure all the kernel code continue to run with KERNEL AMR
value. There is a schedule() call in there with _TIF_NEED_RESCHED. But
those details are not really relevant. That was me tracking down some
issues and writing comment around that part of the code. The only real
detail is switch to userspace AMR in the end.
>
>> }
>>
>> @@ -306,6 +324,10 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>>
>> account_cpu_user_exit();
>>
>> + /*
>> + * We do this at the end so that we do context switch with KERNEL AMR
>> + */
>> + kuap_restore_user_amr(regs);
>
> Duplicated comments I prefer to just have like this instead of trying to
> keep them in sync. Can complete the circular reference by having a
>
> * similarly in interrupt_exit_user_prepare
>
> in the main comment, but if they come close to one another in the same
> file it's not so important to keep them together.
>
> + kuap_restore_user_amr(regs); /* see syscall_exit_prepare */
>
>> return ret;
>> }
>>
>> @@ -376,7 +398,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
>> * which would cause Read-After-Write stalls. Hence, we take the AMR
>> * value from the check above.
>> */
>> - kuap_restore_amr(regs, amr);
>> + kuap_restore_kernel_amr(regs, amr);
>>
>> return ret;
>> }
>> --
>> 2.26.2
>>
>>
-aneesh
next prev parent reply other threads:[~2020-07-03 9:32 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-15 6:13 [PATCH v4 00/41] Kernel userspace access/execution prevention with hash translation Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 01/41] powerpc/book3s64/pkeys: Fixup bit numbering Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 02/41] powerpc/book3s64/pkeys: pkeys are supported only on hash on book3s Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 03/41] powerpc/book3s64/pkeys: Move pkey related bits in the linux page table Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 04/41] powerpc/book3s64/pkeys: Explain key 1 reservation details Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 05/41] powerpc/book3s64/pkeys: Simplify the key initialization Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 06/41] powerpc/book3s64/pkeys: Prevent key 1 modification from userspace Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 07/41] powerpc/book3s64/pkeys: kill cpu feature key CPU_FTR_PKEY Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 08/41] powerpc/book3s64/pkeys: Convert execute key support to static key Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 09/41] powerpc/book3s64/pkeys: Simplify pkey disable branch Aneesh Kumar K.V
2020-06-15 6:13 ` [PATCH v4 10/41] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 11/41] powerpc/book3s64/pkeys: Make initial_allocation_mask static Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 12/41] powerpc/book3s64/pkeys: Mark all the pkeys above max pkey as reserved Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 13/41] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 14/41] powerpc/book3s64/kuep: Add MMU_FTR_KUEP Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 15/41] powerpc/book3s64/pkeys: Use execute_pkey_disable static key Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 16/41] powerpc/book3s64/pkeys: Use MMU_FTR_PKEY instead of pkey_disabled " Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 17/41] powerpc/book3s64/kuap: Move KUAP related function outside radix Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 18/41] powerpc/book3s64/kuep: Move KUEP " Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 19/41] powerpc/book3s64/kuap: Rename MMU_FTR_RADIX_KUAP to MMU_FTR_KUAP Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 20/41] powerpc/book3s64/kuap/kuep: Make KUAP and KUEP a subfeature of PPC_MEM_KEYS Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 21/41] powerpc/book3s64/kuap: Move UAMOR setup to key init function Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 22/41] powerpc/book3s64/kuap: Use Key 3 for kernel mapping with hash translation Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 23/41] powerpc/exec: Set thread.regs early during exec Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 24/41] powerpc/book3s64/pkeys: Store/restore userspace AMR correctly on entry and exit from kernel Aneesh Kumar K.V
2020-07-03 9:18 ` Nicholas Piggin
2020-07-03 9:30 ` Aneesh Kumar K.V [this message]
2020-07-07 6:23 ` Nicholas Piggin
2020-06-15 6:14 ` [PATCH v4 25/41] powerpc/book3s64/kuep: Store/restore userspace IAMR " Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 26/41] powerpc/book3s64/pkeys: Inherit correctly on fork Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 27/41] powerpc/book3s64/pkeys: Reset userspace AMR correctly on exec Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 28/41] powerpc/ptrace-view: Use pt_regs values instead of thread_struct based one Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 29/41] powerpc/book3s64/pkeys: Don't update SPRN_AMR when in kernel mode Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 30/41] powerpc/book3s64/kuap: Restrict access to userspace based on userspace AMR Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 31/41] powerpc/book3s64/kuap: Improve error reporting with KUAP Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 32/41] powerpc/book3s64/kuap: Use Key 3 to implement KUAP with hash translation Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 33/41] powerpc/book3s64/kuep: Use Key 3 to implement KUEP " Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 34/41] powerpc/book3s64/hash/kuap: Enable kuap on hash Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 35/41] powerpc/book3s64/hash/kuep: Enable KUEP " Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 36/41] powerpc/book3s64/keys: Print information during boot Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 37/41] powerpc/selftest/ptrave-pkey: Rename variables to make it easier to follow code Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 38/41] powerpc/selftest/ptrace-pkey: Update the test to mark an invalid pkey correctly Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 39/41] powerpc/selftest/ptrace-pkey: IAMR and uamor cannot be updated by ptrace Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 40/41] powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec Aneesh Kumar K.V
2020-06-15 6:14 ` [PATCH v4 41/41] powerpc/book3s64/hash/kup: Don't hardcode kup key Aneesh Kumar K.V
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83a38d37-099a-7b98-1262-c16b4f5a0cc4@linux.ibm.com \
--to=aneesh.kumar@linux.ibm.com \
--cc=bauerman@linux.ibm.com \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=linuxram@us.ibm.com \
--cc=mpe@ellerman.id.au \
--cc=npiggin@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).