From: Daniel Axtens <dja@axtens•net>
To: "Christopher M. Riedl" <cmr@codefail•de>, linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH v5 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext()
Date: Fri, 12 Feb 2021 16:41:31 +1100 [thread overview]
Message-ID: <871rdletbo.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <20210203184323.20792-7-cmr@codefail.de>
Hi Chris,
> Previously setup_sigcontext() performed a costly KUAP switch on every
> uaccess operation. These repeated uaccess switches cause a significant
> drop in signal handling performance.
>
> Rewrite setup_sigcontext() to assume that a userspace write access window
> is open. Replace all uaccess functions with their 'unsafe' versions
> which avoid the repeated uaccess switches.
Just to clarify the commit message a bit: you're also changing the
callers of the old safe versions to first open the window, then call the
unsafe variants, then close the window again.
> Signed-off-by: Christopher M. Riedl <cmr@codefail•de>
> ---
> arch/powerpc/kernel/signal_64.c | 70 ++++++++++++++++++++-------------
> 1 file changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 8e1d804ce552..4248e4489ff1 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -101,9 +101,13 @@ static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_re
> * Set up the sigcontext for the signal frame.
> */
>
> -static long setup_sigcontext(struct sigcontext __user *sc,
> - struct task_struct *tsk, int signr, sigset_t *set,
> - unsigned long handler, int ctx_has_vsx_region)
> +#define unsafe_setup_sigcontext(sc, tsk, signr, set, handler, \
> + ctx_has_vsx_region, e) \
> + unsafe_op_wrap(__unsafe_setup_sigcontext(sc, tsk, signr, set, \
> + handler, ctx_has_vsx_region), e)
> +static long notrace __unsafe_setup_sigcontext(struct sigcontext __user *sc,
> + struct task_struct *tsk, int signr, sigset_t *set,
> + unsigned long handler, int ctx_has_vsx_region)
> {
> /* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
> * process never used altivec yet (MSR_VEC is zero in pt_regs of
> @@ -118,20 +122,19 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> #endif
> struct pt_regs *regs = tsk->thread.regs;
> unsigned long msr = regs->msr;
> - long err = 0;
> /* Force usr to alway see softe as 1 (interrupts enabled) */
> unsigned long softe = 0x1;
>
> BUG_ON(tsk != current);
>
> #ifdef CONFIG_ALTIVEC
> - err |= __put_user(v_regs, &sc->v_regs);
> + unsafe_put_user(v_regs, &sc->v_regs, efault_out);
>
> /* save altivec registers */
> if (tsk->thread.used_vr) {
> /* Copy 33 vec registers (vr0..31 and vscr) to the stack */
> - err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
> - 33 * sizeof(vector128));
> + unsafe_copy_to_user(v_regs, &tsk->thread.vr_state,
> + 33 * sizeof(vector128), efault_out);
> /* set MSR_VEC in the MSR value in the frame to indicate that sc->v_reg)
> * contains valid data.
> */
> @@ -140,12 +143,12 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> /* We always copy to/from vrsave, it's 0 if we don't have or don't
> * use altivec.
> */
> - err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> + unsafe_put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33], efault_out);
> #else /* CONFIG_ALTIVEC */
> - err |= __put_user(0, &sc->v_regs);
> + unsafe_put_user(0, &sc->v_regs, efault_out);
> #endif /* CONFIG_ALTIVEC */
> /* copy fpr regs and fpscr */
> - err |= copy_fpr_to_user(&sc->fp_regs, tsk);
> + unsafe_copy_fpr_to_user(&sc->fp_regs, tsk, efault_out);
>
> /*
> * Clear the MSR VSX bit to indicate there is no valid state attached
> @@ -160,24 +163,27 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> */
> if (tsk->thread.used_vsr && ctx_has_vsx_region) {
> v_regs += ELF_NVRREG;
> - err |= copy_vsx_to_user(v_regs, tsk);
> + unsafe_copy_vsx_to_user(v_regs, tsk, efault_out);
> /* set MSR_VSX in the MSR value in the frame to
> * indicate that sc->vs_reg) contains valid data.
> */
> msr |= MSR_VSX;
> }
> #endif /* CONFIG_VSX */
> - err |= __put_user(&sc->gp_regs, &sc->regs);
> + unsafe_put_user(&sc->gp_regs, &sc->regs, efault_out);
> WARN_ON(!FULL_REGS(regs));
> - err |= __copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE);
> - err |= __put_user(msr, &sc->gp_regs[PT_MSR]);
> - err |= __put_user(softe, &sc->gp_regs[PT_SOFTE]);
> - err |= __put_user(signr, &sc->signal);
> - err |= __put_user(handler, &sc->handler);
> + unsafe_copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE, efault_out);
> + unsafe_put_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> + unsafe_put_user(softe, &sc->gp_regs[PT_SOFTE], efault_out);
> + unsafe_put_user(signr, &sc->signal, efault_out);
> + unsafe_put_user(handler, &sc->handler, efault_out);
> if (set != NULL)
> - err |= __put_user(set->sig[0], &sc->oldmask);
> + unsafe_put_user(set->sig[0], &sc->oldmask, efault_out);
>
> - return err;
> + return 0;
> +
> +efault_out:
> + return -EFAULT;
> }
>
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> @@ -664,12 +670,15 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
>
> if (old_ctx != NULL) {
> prepare_setup_sigcontext(current, ctx_has_vsx_region);
> - if (!access_ok(old_ctx, ctx_size)
> - || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
> - ctx_has_vsx_region)
> - || __copy_to_user(&old_ctx->uc_sigmask,
> - ¤t->blocked, sizeof(sigset_t)))
> + if (!user_write_access_begin(old_ctx, ctx_size))
> return -EFAULT;
I notice we get rid of access_ok, but that's fine because
user_write_access_begin includes access_ok since Linus decided that was
a good idea.
> +
> + unsafe_setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL,
> + 0, ctx_has_vsx_region, efault_out);
> + unsafe_copy_to_user(&old_ctx->uc_sigmask, ¤t->blocked,
> + sizeof(sigset_t), efault_out);
> +
> + user_write_access_end();
> }
> if (new_ctx == NULL)
> return 0;
> @@ -698,6 +707,10 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> /* This returns like rt_sigreturn */
> set_thread_flag(TIF_RESTOREALL);
> return 0;
> +
> +efault_out:
> + user_write_access_end();
> + return -EFAULT;
> }
>
>
> @@ -850,9 +863,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> } else {
> err |= __put_user(0, &frame->uc.uc_link);
> prepare_setup_sigcontext(tsk, 1);
> - err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
> - NULL, (unsigned long)ksig->ka.sa.sa_handler,
> - 1);
> + if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
> + return -EFAULT;
Here you're opening a window for all of `frame`...
> + err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk,
... but here you're only passing in frame->uc.uc_mcontext for writing.
ISTR that the underlying AMR switch is fully on / fully off so I don't
think it really matters, but in this case should you be calling
user_write_access_begin() with &frame->uc.uc_mcontext and the size of
that?
> + ksig->sig, NULL,
> + (unsigned long)ksig->ka.sa.sa_handler, 1);
> + user_write_access_end();
> }
> err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
> if (err)
Apart from the size thing, everything looks good to me. I checked that
all the things you've changed from safe to unsafe pass the same
parameters, and they all looked good to me.
With those caveats,
Reviewed-by: Daniel Axtens <dja@axtens•net>
Kind regards,
Daniel
next prev parent reply other threads:[~2021-02-12 5:43 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 [this message]
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
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=871rdletbo.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