From: Michael Ellerman <mpe@ellerman•id.au>
To: "Aneesh Kumar K.V" <aneesh.kumar@linux•ibm.com>,
linuxppc-dev@lists•ozlabs.org
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux•ibm.com>,
linuxram@us•ibm.com, bauerman@linux•ibm.com
Subject: Re: [PATCH v5 23/26] powerpc/book3s64/kuap: Move UAMOR setup to key init function
Date: Tue, 07 Jul 2020 16:05:15 +1000 [thread overview]
Message-ID: <87fta35084.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <20200619135850.47155-24-aneesh.kumar@linux.ibm.com>
"Aneesh Kumar K.V" <aneesh.kumar@linux•ibm.com> writes:
> UAMOR values are not application-specific.
It used to be, that's worth mentioning.
> The kernel initializes its value based on different reserved keys.
> Remove the thread-specific UAMOR value and don't switch the UAMOR on
> context switch.
>
> Move UAMOR initialization to key initialization code. Now that
> KUAP/KUEP feature depends on PPC_MEM_KEYS, we can start to consolidate
> all register initialization to keys init.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux•ibm.com>
> ---
> arch/powerpc/include/asm/book3s/64/kup.h | 2 ++
> arch/powerpc/include/asm/processor.h | 1 -
> arch/powerpc/kernel/ptrace/ptrace-view.c | 17 ++++++++----
> arch/powerpc/kernel/smp.c | 5 ++++
> arch/powerpc/mm/book3s64/pkeys.c | 35 ++++++++++++++----------
> 5 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
> index 3a0e138d2735..942594745dfa 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
> @@ -67,6 +67,8 @@
> #include <asm/mmu.h>
> #include <asm/ptrace.h>
>
> +extern u64 default_uamor;
> +
> static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
> {
> if (mmu_has_feature(MMU_FTR_KUAP) && unlikely(regs->kuap != amr)) {
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index 52a67835057a..6ac12168f1fe 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -237,7 +237,6 @@ struct thread_struct {
> #ifdef CONFIG_PPC_MEM_KEYS
> unsigned long amr;
> unsigned long iamr;
> - unsigned long uamor;
> #endif
> #ifdef CONFIG_KVM_BOOK3S_32_HANDLER
> void* kvm_shadow_vcpu; /* KVM internal data */
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
> index caeb5822a8f4..689711eb018a 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-view.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
> @@ -488,14 +488,22 @@ static int pkey_active(struct task_struct *target, const struct user_regset *reg
> static int pkey_get(struct task_struct *target, const struct user_regset *regset,
> unsigned int pos, unsigned int count, void *kbuf, void __user *ubuf)
> {
> + int ret;
> +
> BUILD_BUG_ON(TSO(amr) + sizeof(unsigned long) != TSO(iamr));
> - BUILD_BUG_ON(TSO(iamr) + sizeof(unsigned long) != TSO(uamor));
>
> if (!arch_pkeys_enabled())
> return -ENODEV;
>
> - return user_regset_copyout(&pos, &count, &kbuf, &ubuf, &target->thread.amr,
> - 0, ELF_NPKEY * sizeof(unsigned long));
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &target->thread.amr,
> + 0, 2 * sizeof(unsigned long));
> + if (ret)
> + goto err_out;
Why not just return?
> +
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &default_uamor,
> + 2 * sizeof(unsigned long), 3 * sizeof(unsigned long));
> +err_out:
> + return ret;
> }
>
> static int pkey_set(struct task_struct *target, const struct user_regset *regset,
> @@ -518,8 +526,7 @@ static int pkey_set(struct task_struct *target, const struct user_regset *regset
> return ret;
>
> /* UAMOR determines which bits of the AMR can be set from userspace. */
> - target->thread.amr = (new_amr & target->thread.uamor) |
> - (target->thread.amr & ~target->thread.uamor);
> + target->thread.amr = (new_amr & default_uamor) | (target->thread.amr & ~default_uamor);
That comment could explain better why we are bothering to mask with ~default_uamor.
> return 0;
> }
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index c820c95162ff..eec40082599f 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -59,6 +59,7 @@
> #include <asm/asm-prototypes.h>
> #include <asm/cpu_has_feature.h>
> #include <asm/ftrace.h>
> +#include <asm/kup.h>
>
> #ifdef DEBUG
> #include <asm/udbg.h>
> @@ -1256,6 +1257,10 @@ void start_secondary(void *unused)
> mmgrab(&init_mm);
> current->active_mm = &init_mm;
>
> +#ifdef CONFIG_PPC_MEM_KEYS
> + mtspr(SPRN_UAMOR, default_uamor);
> +#endif
That's 1) not very pretty and 2) risks blowing up on other CPUs.
It should at least go in early_init_mmu_secondary().
> smp_store_cpu_info(cpu);
> set_dec(tb_ticks_per_jiffy);
> preempt_disable();
> diff --git a/arch/powerpc/mm/book3s64/pkeys.c b/arch/powerpc/mm/book3s64/pkeys.c
> index aeecc8b8e11c..3f3593f85358 100644
> --- a/arch/powerpc/mm/book3s64/pkeys.c
> +++ b/arch/powerpc/mm/book3s64/pkeys.c
> @@ -24,7 +24,7 @@ static u32 initial_allocation_mask; /* Bits set for the initially allocated k
> static u64 default_amr;
> static u64 default_iamr;
> /* Allow all keys to be modified by default */
> -static u64 default_uamor = ~0x0UL;
> +u64 default_uamor = ~0x0UL;
__ro_after_init?
> /*
> * Key used to implement PROT_EXEC mmap. Denies READ/WRITE
> * We pick key 2 because 0 is special key and 1 is reserved as per ISA.
> @@ -113,8 +113,16 @@ void __init pkey_early_init_devtree(void)
> /* scan the device tree for pkey feature */
> pkeys_total = scan_pkey_feature();
> if (!pkeys_total) {
> - /* No support for pkey. Mark it disabled */
> - return;
> + /*
> + * No key support but on radix we can use key 0
> + * to implement kuap.
> + */
> + if (early_radix_enabled())
> + /*
> + * Make sure userspace can't change the AMR
> + */
> + default_uamor = 0;
> + goto err_out;
Would be cleaner if you inverted that. ie. initialise to 0 and then set
to ~0x0UL when you detect pkeys.
> }
>
> cur_cpu_spec->mmu_features |= MMU_FTR_PKEY;
> @@ -197,6 +205,12 @@ void __init pkey_early_init_devtree(void)
> initial_allocation_mask |= reserved_allocation_mask;
>
> pr_info("Enabling Memory keys with max key count %d", max_pkey);
> +err_out:
It's not "err" out if the OK path goes via here. That's just "out".
> + /*
> + * Setup uamor on boot cpu
> + */
> + mtspr(SPRN_UAMOR, default_uamor);
> +
> return;
> }
>
> @@ -232,8 +246,9 @@ void __init setup_kuap(bool disabled)
> cur_cpu_spec->mmu_features |= MMU_FTR_KUAP;
> }
>
> - /* Make sure userspace can't change the AMR */
> - mtspr(SPRN_UAMOR, 0);
Why not just leave it there. It's extra insurance and it's good
documentation.
> + /*
> + * Set the default kernel AMR values on all cpus.
> + */
> mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);
> isync();
> }
> @@ -278,11 +293,6 @@ static inline u64 read_uamor(void)
> return mfspr(SPRN_UAMOR);
> }
>
> -static inline void write_uamor(u64 value)
> -{
> - mtspr(SPRN_UAMOR, value);
> -}
> -
> static bool is_pkey_enabled(int pkey)
> {
> u64 uamor = read_uamor();
> @@ -353,7 +363,6 @@ void thread_pkey_regs_save(struct thread_struct *thread)
> */
> thread->amr = read_amr();
> thread->iamr = read_iamr();
> - thread->uamor = read_uamor();
> }
>
> void thread_pkey_regs_restore(struct thread_struct *new_thread,
> @@ -366,8 +375,6 @@ void thread_pkey_regs_restore(struct thread_struct *new_thread,
> write_amr(new_thread->amr);
> if (old_thread->iamr != new_thread->iamr)
> write_iamr(new_thread->iamr);
> - if (old_thread->uamor != new_thread->uamor)
> - write_uamor(new_thread->uamor);
> }
>
> void thread_pkey_regs_init(struct thread_struct *thread)
> @@ -377,11 +384,9 @@ void thread_pkey_regs_init(struct thread_struct *thread)
>
> thread->amr = default_amr;
> thread->iamr = default_iamr;
> - thread->uamor = default_uamor;
>
> write_amr(default_amr);
> write_iamr(default_iamr);
> - write_uamor(default_uamor);
> }
>
> int execute_only_pkey(struct mm_struct *mm)
cheers
next prev parent reply other threads:[~2020-07-07 6:04 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-19 13:58 [PATCH v5 00/26] powerpc/book3s/64/pkeys: Simplify the code Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 01/26] powerpc/book3s64/pkeys: Fixup bit numbering Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 02/26] powerpc/book3s64/pkeys: pkeys are supported only on hash on book3s Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 03/26] powerpc/book3s64/pkeys: Move pkey related bits in the linux page table Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 04/26] powerpc/book3s64/pkeys: Explain key 1 reservation details Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 05/26] powerpc/book3s64/pkeys: Simplify the key initialization Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 06/26] powerpc/book3s64/pkeys: Prevent key 1 modification from userspace Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 07/26] powerpc/book3s64/pkeys: kill cpu feature key CPU_FTR_PKEY Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 08/26] powerpc/book3s64/pkeys: Convert execute key support to static key Aneesh Kumar K.V
2020-07-06 7:19 ` Michael Ellerman
2020-07-06 8:47 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 09/26] powerpc/book3s64/pkeys: Simplify pkey disable branch Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 10/26] powerpc/book3s64/pkeys: Convert pkey_total to max_pkey Aneesh Kumar K.V
2020-07-06 7:04 ` Michael Ellerman
2020-07-06 7:20 ` Aneesh Kumar K.V
2020-07-07 1:26 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 11/26] powerpc/book3s64/pkeys: Make initial_allocation_mask static Aneesh Kumar K.V
2020-07-06 7:04 ` Michael Ellerman
2020-07-06 8:48 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 12/26] powerpc/book3s64/pkeys: Mark all the pkeys above max pkey as reserved Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 13/26] powerpc/book3s64/pkeys: Enable MMU_FTR_PKEY Aneesh Kumar K.V
2020-07-06 13:10 ` Michael Ellerman
2020-07-06 14:09 ` Aneesh Kumar K.V
2020-07-06 17:17 ` Aneesh Kumar K.V
2020-07-07 1:02 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 14/26] powerpc/book3s64/kuep: Add MMU_FTR_KUEP Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 15/26] powerpc/book3s64/pkeys: Use execute_pkey_disable static key Aneesh Kumar K.V
2020-07-06 7:20 ` Michael Ellerman
2020-07-06 8:49 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 16/26] powerpc/book3s64/pkeys: Use MMU_FTR_PKEY instead of pkey_disabled " Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 17/26] powerpc/book3s64/keys: Print information during boot Aneesh Kumar K.V
2020-07-06 7:52 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 18/26] powerpc/book3s64/keys/kuap: Reset AMR/IAMR values on kexec Aneesh Kumar K.V
2020-07-06 12:29 ` Michael Ellerman
2020-07-06 14:39 ` Aneesh Kumar K.V
2020-07-07 1:07 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 19/26] powerpc/book3s64/kuap: Move KUAP related function outside radix Aneesh Kumar K.V
2020-07-06 12:41 ` Michael Ellerman
2020-07-06 14:41 ` Aneesh Kumar K.V
2020-07-07 1:22 ` Michael Ellerman
2020-06-19 13:58 ` [PATCH v5 20/26] powerpc/book3s64/kuep: Move KUEP " Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 21/26] powerpc/book3s64/kuap: Rename MMU_FTR_RADIX_KUAP to MMU_FTR_KUAP Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 22/26] powerpc/book3s64/kuap/kuep: Make KUAP and KUEP a subfeature of PPC_MEM_KEYS Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 23/26] powerpc/book3s64/kuap: Move UAMOR setup to key init function Aneesh Kumar K.V
2020-07-07 6:05 ` Michael Ellerman [this message]
2020-07-07 11:25 ` Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 24/26] powerpc/selftest/ptrave-pkey: Rename variables to make it easier to follow code Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 25/26] powerpc/selftest/ptrace-pkey: Update the test to mark an invalid pkey correctly Aneesh Kumar K.V
2020-06-19 13:58 ` [PATCH v5 26/26] powerpc/selftest/ptrace-pkey: IAMR and uamor cannot be updated by ptrace 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=87fta35084.fsf@mpe.ellerman.id.au \
--to=mpe@ellerman$(echo .)id.au \
--cc=aneesh.kumar@linux$(echo .)ibm.com \
--cc=bauerman@linux$(echo .)ibm.com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=linuxram@us$(echo .)ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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