On Tue Feb 9, 2021 at 3:45 PM CST, Christophe Leroy wrote:
> "Christopher M. Riedl" <c...@codefail.de> a écrit :
>
> > 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.
> >
> > 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)
>
> Should be called __get_user_sigset() as it is a helper for __get_user()
Ok makes sense.
>
> > +{
> > + if (sizeof(sigset_t) <= 8)
>
> We should always use __get_user(), see below.
>
> > + return __get_user(dst->sig[0], &src->sig[0]);
>
> I think the above will not work on ppc32, it will only copy 4 bytes.
> You must cast the source to u64*
Well this is signal_64.c :) Looks like ppc32 needs the same thing so
I'll just move this into signal.h and use it for both.
The only exception would be the COMPAT case in signal_32.c which ends up
calling the common get_compat_sigset(). Updating that is probably
outside the scope of this series.
>
> > + else
> > + return __copy_from_user(dst, src, sizeof(sigset_t));
>
> I see no point in keeping this alternative. Today sigset_ t is fixed.
> If you fear one day someone might change it to something different
> than a u64, just add a BUILD_BUG_ON(sizeof(sigset_t) != sizeof(u64));
Ah yes that is much better - thanks for the suggestion.
>
> > +}
> > +
> > /*
> > * 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);
> > +
>
> This white space is not part of the change, keep patches to the
> minimum, avoid cosmetic
Just a (bad?) habit on my part that I missed - I'll remove this one and
the one further below.
>
> > 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;
> > +
>
> Same
>
> > set_current_blocked(&set);
> >
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > --
> > 2.26.1
