Re: [PATCH v2 25/25] powerpc/signal32: Transform save_user_regs() and save_tm_user_regs() in 'unsafe' version

[Christophe Leroy]

Le 29/09/2020 à 04:55, Christopher M. Riedl a écrit :
> On Tue Aug 18, 2020 at 12:19 PM CDT, Christophe Leroy wrote:
> > Change those two functions to be used within a user access block.
> >
> > For that, change save_general_regs() to and unsafe_save_general_regs(),
> > then replace all user accesses by unsafe_ versions.
> >
> > This series leads to a reduction from 2.55s to 1.73s of
> > the system CPU time with the following microbench app
> > on an mpc832x with KUAP (approx 32%)
> >
> > Without KUAP, the difference is in the noise.
> >
> > void sigusr1(int sig) { }
> >
> > int main(int argc, char **argv)
> > {
> > int i = 100000;
> >
> > signal(SIGUSR1, sigusr1);
> > for (;i--;)
> > raise(SIGUSR1);
> > exit(0);
> > }
> >
> > An additional 0.10s reduction is achieved by removing
> > CONFIG_PPC_FPU, as the mpc832x has no FPU.
> >
> > A bit less spectacular on an 8xx as KUAP is less heavy, prior to
> > the series (with KUAP) it ran in 8.10 ms. Once applies the removal
> > of FPU regs handling, we get 7.05s. With the full series, we get 6.9s.
> > If artificially re-activating FPU regs handling with the full series,
> > we get 7.6s.
> >
> > So for the 8xx, the removal of the FPU regs copy is what makes the
> > difference, but the rework of handle_signal also have a benefit.
> >
> > Same as above, without KUAP the difference is in the noise.
> >
> > Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> > ---
> > arch/powerpc/kernel/signal_32.c | 224 ++++++++++++++++----------------
> > 1 file changed, 111 insertions(+), 113 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_32.c
> > b/arch/powerpc/kernel/signal_32.c
> > index 86539a4e0514..f795fe0240a1 100644
> > --- a/arch/powerpc/kernel/signal_32.c
> > +++ b/arch/powerpc/kernel/signal_32.c
> > @@ -93,8 +93,8 @@ static inline int get_sigset_t(sigset_t *set,
> > #define to_user_ptr(p) ptr_to_compat(p)
> > #define from_user_ptr(p) compat_ptr(p)
> >   
> > -static inline int save_general_regs(struct pt_regs *regs,
> > - struct mcontext __user *frame)
> > +static __always_inline int
> > +save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user
> > *frame)
> > {
> > elf_greg_t64 *gregs = (elf_greg_t64 *)regs;
> > int val, i;
> > @@ -108,10 +108,12 @@ static inline int save_general_regs(struct pt_regs
> > *regs,
> > else
> > val = gregs[i];
> >   
> > - if (__put_user(val, &frame->mc_gregs[i]))
> > - return -EFAULT;
> > + unsafe_put_user(val, &frame->mc_gregs[i], failed);
> > }
> > return 0;
> > +
> > +failed:
> > + return 1;
> > }
> >   
> > static inline int restore_general_regs(struct pt_regs *regs,
> > @@ -148,11 +150,15 @@ static inline int get_sigset_t(sigset_t *set,
> > const sigset_t __user *uset)
> > #define to_user_ptr(p) ((unsigned long)(p))
> > #define from_user_ptr(p) ((void __user *)(p))
> >   
> > -static inline int save_general_regs(struct pt_regs *regs,
> > - struct mcontext __user *frame)
> > +static __always_inline int
> > +save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user
> > *frame)
> > {
> > WARN_ON(!FULL_REGS(regs));
> > - return __copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE);
> > + unsafe_copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE, failed);
> > + return 0;
> > +
> > +failed:
> > + return 1;
> > }
> >   
> > static inline int restore_general_regs(struct pt_regs *regs,
> > @@ -170,6 +176,11 @@ static inline int restore_general_regs(struct
> > pt_regs *regs,
> > }
> > #endif
> >   
> > +#define unsafe_save_general_regs(regs, frame, label) do { \
> > + if (save_general_regs_unsafe(regs, frame)) \
>
> Minor nitpick (sorry); this naming seems a bit strange to me, in x86 it
> is "__unsafe_" as a prefix instead of "_unsafe" as a suffix. That sounds
> a bit better to me, what do you think? Unless there is some convention I
> am not aware of here apart from "unsafe_" using a goto label for errors.

You are probably right, I have never been good at naming stuff.

Christophe