From: Daniel Axtens <dja@axtens•net>
To: "Christopher M. Riedl" <cmr@codefail•de>, linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH v5 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext()
Date: Sat, 13 Feb 2021 08:17:54 +1100 [thread overview]
Message-ID: <87y2ftc7el.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <20210203184323.20792-8-cmr@codefail.de>
Hi Chris,
> Previously restore_sigcontext() performed a costly KUAP switch on every
> uaccess operation. These repeated uaccess switches cause a significant
> drop in signal handling performance.
>
> Rewrite restore_sigcontext() to assume that a userspace read access
> window is open. Replace all uaccess functions with their 'unsafe'
> versions which avoid the repeated uaccess switches.
>
Much of the same comments apply here as to the last patch:
- the commit message might be improved by adding that you are also
changing the calling functions to open the uaccess window before
calling into the new unsafe functions
- I have checked that the safe to unsafe conversions look right.
- I think you're opening too wide a window in user_read_access_begin,
it seems to me that it could be reduced to just the
uc_mcontext. (Again, not that it makes a difference with the current
HW.)
Kind regards,
Daniel
> Signed-off-by: Christopher M. Riedl <cmr@codefail•de>
> ---
> arch/powerpc/kernel/signal_64.c | 68 ++++++++++++++++++++-------------
> 1 file changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 4248e4489ff1..d668f8af18fe 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -326,14 +326,14 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
> /*
> * Restore the sigcontext from the signal frame.
> */
> -
> -static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> - struct sigcontext __user *sc)
> +#define unsafe_restore_sigcontext(tsk, set, sig, sc, e) \
> + unsafe_op_wrap(__unsafe_restore_sigcontext(tsk, set, sig, sc), e)
> +static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_t *set,
> + int sig, struct sigcontext __user *sc)
> {
> #ifdef CONFIG_ALTIVEC
> elf_vrreg_t __user *v_regs;
> #endif
> - unsigned long err = 0;
> unsigned long save_r13 = 0;
> unsigned long msr;
> struct pt_regs *regs = tsk->thread.regs;
> @@ -348,27 +348,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> save_r13 = regs->gpr[13];
>
> /* copy the GPRs */
> - err |= __copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr));
> - err |= __get_user(regs->nip, &sc->gp_regs[PT_NIP]);
> + unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr),
> + efault_out);
> + unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out);
> /* get MSR separately, transfer the LE bit if doing signal return */
> - err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
> + unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> if (sig)
> regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
> - err |= __get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3]);
> - err |= __get_user(regs->ctr, &sc->gp_regs[PT_CTR]);
> - err |= __get_user(regs->link, &sc->gp_regs[PT_LNK]);
> - err |= __get_user(regs->xer, &sc->gp_regs[PT_XER]);
> - err |= __get_user(regs->ccr, &sc->gp_regs[PT_CCR]);
> + unsafe_get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3], efault_out);
> + unsafe_get_user(regs->ctr, &sc->gp_regs[PT_CTR], efault_out);
> + unsafe_get_user(regs->link, &sc->gp_regs[PT_LNK], efault_out);
> + unsafe_get_user(regs->xer, &sc->gp_regs[PT_XER], efault_out);
> + unsafe_get_user(regs->ccr, &sc->gp_regs[PT_CCR], efault_out);
> /* Don't allow userspace to set SOFTE */
> set_trap_norestart(regs);
> - err |= __get_user(regs->dar, &sc->gp_regs[PT_DAR]);
> - err |= __get_user(regs->dsisr, &sc->gp_regs[PT_DSISR]);
> - err |= __get_user(regs->result, &sc->gp_regs[PT_RESULT]);
> + unsafe_get_user(regs->dar, &sc->gp_regs[PT_DAR], efault_out);
> + unsafe_get_user(regs->dsisr, &sc->gp_regs[PT_DSISR], efault_out);
> + unsafe_get_user(regs->result, &sc->gp_regs[PT_RESULT], efault_out);
>
> if (!sig)
> regs->gpr[13] = save_r13;
> if (set != NULL)
> - err |= __get_user(set->sig[0], &sc->oldmask);
> + unsafe_get_user(set->sig[0], &sc->oldmask, efault_out);
>
> /*
> * Force reload of FP/VEC.
> @@ -378,29 +379,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> regs->msr &= ~(MSR_FP | MSR_FE0 | MSR_FE1 | MSR_VEC | MSR_VSX);
>
> #ifdef CONFIG_ALTIVEC
> - err |= __get_user(v_regs, &sc->v_regs);
> - if (err)
> - return err;
> + unsafe_get_user(v_regs, &sc->v_regs, efault_out);
> if (v_regs && !access_ok(v_regs, 34 * sizeof(vector128)))
> return -EFAULT;
> /* Copy 33 vec registers (vr0..31 and vscr) from the stack */
> if (v_regs != NULL && (msr & MSR_VEC) != 0) {
> - err |= __copy_from_user(&tsk->thread.vr_state, v_regs,
> - 33 * sizeof(vector128));
> + unsafe_copy_from_user(&tsk->thread.vr_state, v_regs,
> + 33 * sizeof(vector128), efault_out);
> tsk->thread.used_vr = true;
> } else if (tsk->thread.used_vr) {
> memset(&tsk->thread.vr_state, 0, 33 * sizeof(vector128));
> }
> /* Always get VRSAVE back */
> if (v_regs != NULL)
> - err |= __get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> + unsafe_get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33],
> + efault_out);
> else
> tsk->thread.vrsave = 0;
> if (cpu_has_feature(CPU_FTR_ALTIVEC))
> mtspr(SPRN_VRSAVE, tsk->thread.vrsave);
> #endif /* CONFIG_ALTIVEC */
> /* restore floating point */
> - err |= copy_fpr_from_user(tsk, &sc->fp_regs);
> + unsafe_copy_fpr_from_user(tsk, &sc->fp_regs, efault_out);
> #ifdef CONFIG_VSX
> /*
> * Get additional VSX data. Update v_regs to point after the
> @@ -409,14 +409,17 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> */
> v_regs += ELF_NVRREG;
> if ((msr & MSR_VSX) != 0) {
> - err |= copy_vsx_from_user(tsk, v_regs);
> + unsafe_copy_vsx_from_user(tsk, v_regs, efault_out);
> tsk->thread.used_vsr = true;
> } else {
> for (i = 0; i < 32 ; i++)
> tsk->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
> }
> #endif
> - return err;
> + return 0;
> +
> +efault_out:
> + return -EFAULT;
> }
>
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> @@ -701,8 +704,14 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
> do_exit(SIGSEGV);
> set_current_blocked(&set);
> - if (restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext))
> +
> + if (!user_read_access_begin(new_ctx, ctx_size))
> + return -EFAULT;
> + if (__unsafe_restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext)) {
> + user_read_access_end();
> do_exit(SIGSEGV);
> + }
> + user_read_access_end();
>
> /* This returns like rt_sigreturn */
> set_thread_flag(TIF_RESTOREALL);
> @@ -807,8 +816,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
> * causing a TM bad thing.
> */
> current->thread.regs->msr &= ~MSR_TS_MASK;
> - if (restore_sigcontext(current, NULL, 1, &uc->uc_mcontext))
> + if (!user_read_access_begin(uc, sizeof(*uc)))
> + return -EFAULT;
> + if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
> + user_read_access_end();
> goto badframe;
> + }
> + user_read_access_end();
> }
>
> if (restore_altstack(&uc->uc_stack))
> --
> 2.26.1
next prev parent reply other threads:[~2021-02-12 21:20 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-03 18:43 [PATCH v5 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 01/10] powerpc/uaccess: Add unsafe_copy_from_user Christopher M. Riedl
2021-02-05 4:47 ` Daniel Axtens
2021-02-03 18:43 ` [PATCH v5 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user() Christopher M. Riedl
2021-02-05 5:17 ` Daniel Axtens
2021-02-03 18:43 ` [PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext() Christopher M. Riedl
2021-02-08 4:44 ` Daniel Axtens
2021-02-10 4:37 ` Christopher M. Riedl
2021-02-10 21:06 ` Daniel Axtens
2021-02-10 23:51 ` Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 04/10] powerpc: Reference param in MSR_TM_ACTIVE() macro Christopher M. Riedl
2021-02-12 4:52 ` Daniel Axtens
2021-02-03 18:43 ` [PATCH v5 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block Christopher M. Riedl
2021-02-12 5:21 ` Daniel Axtens
2021-02-17 19:27 ` Christopher M. Riedl
2021-02-18 10:47 ` Michael Ellerman
2021-02-03 18:43 ` [PATCH v5 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext() Christopher M. Riedl
2021-02-12 5:41 ` Daniel Axtens
2021-02-17 19:31 ` Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext() Christopher M. Riedl
2021-02-12 21:17 ` Daniel Axtens [this message]
2021-02-17 19:32 ` Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 08/10] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 09/10] powerpc/signal64: Rewrite rt_sigreturn() " Christopher M. Riedl
2021-02-03 18:43 ` [PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t Christopher M. Riedl
2021-02-05 4:40 ` Christopher M. Riedl
2021-02-09 21:45 ` Christophe Leroy
2021-02-10 4:16 ` Christopher M. Riedl
2021-02-12 21:21 ` Daniel Axtens
2021-02-17 19:53 ` Christopher M. Riedl
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=87y2ftc7el.fsf@dja-thinkpad.axtens.net \
--to=dja@axtens$(echo .)net \
--cc=cmr@codefail$(echo .)de \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
/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