On Fri Feb 12, 2021 at 3:17 PM CST, Daniel Axtens wrote:
> 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.)
Ok, I'll fix these in the next version as well.
>
> Kind regards,
> Daniel
>
> > Signed-off-by: Christopher M. Riedl <c...@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
