On Fri Feb 12, 2021 at 3:21 PM CST, Daniel Axtens wrote: > "Christopher M. Riedl" <c...@codefail.de> writes: > > > Usually sigset_t is exactly 8B which is a "trivial" size and does not > > warrant using __copy_from_user(). Use __get_user() directly in > > anticipation of future work to remove the trivial size optimizations > > from __copy_from_user(). Calling __get_user() also results in a small > > boost to signal handling throughput here. > > Modulo the comments from Christophe, this looks good to me. It seems to > do what it says on the tin. > > Reviewed-by: Daniel Axtens <d...@axtens.net> > > Do you know if this patch is responsible for the slightly increase in > radix performance you observed in your cover letter? The rest of the > series shouldn't really make things faster than the no-KUAP case...
No, this patch just results in a really small improvement overall. I looked over this again and I think the reason for the speedup is that my implementation of unsafe_copy_from_user() in this series calls __copy_tofrom_user() directly avoiding a barrier_nospec(). This speeds up all the unsafe_get_from_user() calls in this version. Skipping the barrier_nospec() like that is incorrect, but Christophe recently sent a patch which moves barrier_nospec() into the uaccess allowance helpers. It looks like mpe has already accepted that patch: https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-February/223959.html That means that my implementation of unsafe_copy_from_user() is _now_ correct _and_ faster. We do not need to call barrier_nospec() since the precondition for the 'unsafe' routine is that we called one of the helpers to open up a uaccess window earlier. > > Kind regards, > Daniel > > > > > Signed-off-by: Christopher M. Riedl <c...@codefail.de> > > --- > > arch/powerpc/kernel/signal_64.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/arch/powerpc/kernel/signal_64.c > > b/arch/powerpc/kernel/signal_64.c > > index 817b64e1e409..42fdc4a7ff72 100644 > > --- a/arch/powerpc/kernel/signal_64.c > > +++ b/arch/powerpc/kernel/signal_64.c > > @@ -97,6 +97,14 @@ static void prepare_setup_sigcontext(struct task_struct > > *tsk, int ctx_has_vsx_re > > #endif /* CONFIG_VSX */ > > } > > > > +static inline int get_user_sigset(sigset_t *dst, const sigset_t *src) > > +{ > > + if (sizeof(sigset_t) <= 8) > > + return __get_user(dst->sig[0], &src->sig[0]); > > + else > > + return __copy_from_user(dst, src, sizeof(sigset_t)); > > +} > > + > > /* > > * Set up the sigcontext for the signal frame. > > */ > > @@ -701,8 +709,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, > > old_ctx, > > * We kill the task with a SIGSEGV in this situation. > > */ > > > > - if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set))) > > + if (get_user_sigset(&set, &new_ctx->uc_sigmask)) > > do_exit(SIGSEGV); > > + > > set_current_blocked(&set); > > > > if (!user_read_access_begin(new_ctx, ctx_size)) > > @@ -740,8 +749,9 @@ SYSCALL_DEFINE0(rt_sigreturn) > > if (!access_ok(uc, sizeof(*uc))) > > goto badframe; > > > > - if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set))) > > + if (get_user_sigset(&set, &uc->uc_sigmask)) > > goto badframe; > > + > > set_current_blocked(&set); > > > > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > -- > > 2.26.1
