| Message ID | 20201015150159.28933-8-cmr@codefail.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve signal performance on PPC64 with KUAP | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (118be7377c97e35c33819bcb3bbbae5a42a4ac43) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (a2d0230b91f7e23ceb5d8fb6a9799f30517ec33a) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (3e4fb4346c781068610d03c12b16c0cfb0fd24a3) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (0460534b532e5518c657c7d6492b9337d975eaa3) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (03d992bd2de6c7f2c9bbd4260ff104c0926ce3ff) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit : > From: Daniel Axtens <dja@axtens.net> > > Add uaccess blocks and use the 'unsafe' versions of functions doing user > access where possible to reduce the number of times uaccess has to be > opened/closed. > > There is no 'unsafe' version of copy_siginfo_to_user, so move it > slightly to allow for a "longer" uaccess block. > > Signed-off-by: Daniel Axtens <dja@axtens.net> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > --- > arch/powerpc/kernel/signal_64.c | 54 ++++++++++++++++----------------- > 1 file changed, 27 insertions(+), 27 deletions(-) > > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > index 6d4f7a5c4fbf..3b97e3681a8f 100644 > --- a/arch/powerpc/kernel/signal_64.c > +++ b/arch/powerpc/kernel/signal_64.c > @@ -843,46 +843,42 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > /* Save the thread's msr before get_tm_stackpointer() changes it */ > unsigned long msr = regs->msr; > #endif > - > frame = get_sigframe(ksig, tsk, sizeof(*frame), 0); > - if (!access_ok(frame, sizeof(*frame))) > + if (!user_write_access_begin(frame, sizeof(*frame))) > goto badframe; > > - err |= __put_user(&frame->info, &frame->pinfo); > - err |= __put_user(&frame->uc, &frame->puc); > - err |= copy_siginfo_to_user(&frame->info, &ksig->info); > - if (err) > - goto badframe; > + unsafe_put_user(&frame->info, &frame->pinfo, badframe_block); > + unsafe_put_user(&frame->uc, &frame->puc, badframe_block); > > /* Create the ucontext. */ > - err |= __put_user(0, &frame->uc.uc_flags); > - err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]); > + unsafe_put_user(0, &frame->uc.uc_flags, badframe_block); > + unsafe_save_altstack(&frame->uc.uc_stack, regs->gpr[1], badframe_block); > + > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > if (MSR_TM_ACTIVE(msr)) { > /* The ucontext_t passed to userland points to the second > * ucontext_t (for transactional state) with its uc_link ptr. > */ > - err |= __put_user(&frame->uc_transact, &frame->uc.uc_link); > + unsafe_put_user(&frame->uc_transact, &frame->uc.uc_link, badframe_block); > + user_write_access_end(); Whaou. Doing this inside an #ifdef sequence is dirty. Can you reorganise code to avoid that and to avoid nesting #ifdef/#endif and the if/else as I did in signal32 ? > err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext, > &frame->uc_transact.uc_mcontext, > tsk, ksig->sig, NULL, > (unsigned long)ksig->ka.sa.sa_handler, > msr); > + if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) > + goto badframe; > + > } else > #endif > { > - err |= __put_user(0, &frame->uc.uc_link); > - > - if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) > - return -EFAULT; > - err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk, > - ksig->sig, NULL, > - (unsigned long)ksig->ka.sa.sa_handler, 1); > - user_write_access_end(); > + unsafe_put_user(0, &frame->uc.uc_link, badframe_block); > + unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig, > + NULL, (unsigned long)ksig->ka.sa.sa_handler, > + 1, badframe_block); > } > - err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set)); > - if (err) > - goto badframe; > + > + unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block); > > /* Make sure signal handler doesn't get spurious FP exceptions */ > tsk->thread.fp_state.fpscr = 0; > @@ -891,15 +887,17 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) { > regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp; > } else { > - if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) > - return -EFAULT; > - err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); > - user_write_access_end(); > - if (err) > - goto badframe; > + unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0], > + badframe_block); > regs->nip = (unsigned long) &frame->tramp[0]; > } > > + user_write_access_end(); > + > + /* Save the siginfo outside of the unsafe block. */ > + if (copy_siginfo_to_user(&frame->info, &ksig->info)) > + goto badframe; > + > /* Allocate a dummy caller frame for the signal handler. */ > newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE; > err |= put_user(regs->gpr[1], (unsigned long __user *)newsp); > @@ -939,6 +937,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > > return 0; > > +badframe_block: > + user_write_access_end(); > badframe: > signal_fault(current, regs, "handle_rt_signal64", frame); > > Christophe
On Fri Oct 16, 2020 at 11:00 AM CDT, Christophe Leroy wrote: > > > Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit : > > From: Daniel Axtens <dja@axtens.net> > > > > Add uaccess blocks and use the 'unsafe' versions of functions doing user > > access where possible to reduce the number of times uaccess has to be > > opened/closed. > > > > There is no 'unsafe' version of copy_siginfo_to_user, so move it > > slightly to allow for a "longer" uaccess block. > > > > Signed-off-by: Daniel Axtens <dja@axtens.net> > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > > --- > > arch/powerpc/kernel/signal_64.c | 54 ++++++++++++++++----------------- > > 1 file changed, 27 insertions(+), 27 deletions(-) > > > > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > > index 6d4f7a5c4fbf..3b97e3681a8f 100644 > > --- a/arch/powerpc/kernel/signal_64.c > > +++ b/arch/powerpc/kernel/signal_64.c > > @@ -843,46 +843,42 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > > /* Save the thread's msr before get_tm_stackpointer() changes it */ > > unsigned long msr = regs->msr; > > #endif > > - > > frame = get_sigframe(ksig, tsk, sizeof(*frame), 0); > > - if (!access_ok(frame, sizeof(*frame))) > > + if (!user_write_access_begin(frame, sizeof(*frame))) > > goto badframe; > > > > - err |= __put_user(&frame->info, &frame->pinfo); > > - err |= __put_user(&frame->uc, &frame->puc); > > - err |= copy_siginfo_to_user(&frame->info, &ksig->info); > > - if (err) > > - goto badframe; > > + unsafe_put_user(&frame->info, &frame->pinfo, badframe_block); > > + unsafe_put_user(&frame->uc, &frame->puc, badframe_block); > > > > /* Create the ucontext. */ > > - err |= __put_user(0, &frame->uc.uc_flags); > > - err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]); > > + unsafe_put_user(0, &frame->uc.uc_flags, badframe_block); > > + unsafe_save_altstack(&frame->uc.uc_stack, regs->gpr[1], badframe_block); > > + > > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > > if (MSR_TM_ACTIVE(msr)) { > > /* The ucontext_t passed to userland points to the second > > * ucontext_t (for transactional state) with its uc_link ptr. > > */ > > - err |= __put_user(&frame->uc_transact, &frame->uc.uc_link); > > + unsafe_put_user(&frame->uc_transact, &frame->uc.uc_link, badframe_block); > > + user_write_access_end(); > > Whaou. Doing this inside an #ifdef sequence is dirty. > Can you reorganise code to avoid that and to avoid nesting #ifdef/#endif > and the if/else as I did in > signal32 ? Hopefully yes - next spin! > > > err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext, > > &frame->uc_transact.uc_mcontext, > > tsk, ksig->sig, NULL, > > (unsigned long)ksig->ka.sa.sa_handler, > > msr); > > + if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) > > + goto badframe; > > + > > } else > > #endif > > { > > - err |= __put_user(0, &frame->uc.uc_link); > > - > > - if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) > > - return -EFAULT; > > - err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk, > > - ksig->sig, NULL, > > - (unsigned long)ksig->ka.sa.sa_handler, 1); > > - user_write_access_end(); > > + unsafe_put_user(0, &frame->uc.uc_link, badframe_block); > > + unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig, > > + NULL, (unsigned long)ksig->ka.sa.sa_handler, > > + 1, badframe_block); > > } > > - err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set)); > > - if (err) > > - goto badframe; > > + > > + unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block); > > > > /* Make sure signal handler doesn't get spurious FP exceptions */ > > tsk->thread.fp_state.fpscr = 0; > > @@ -891,15 +887,17 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > > if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) { > > regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp; > > } else { > > - if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) > > - return -EFAULT; > > - err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); > > - user_write_access_end(); > > - if (err) > > - goto badframe; > > + unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0], > > + badframe_block); > > regs->nip = (unsigned long) &frame->tramp[0]; > > } > > > > + user_write_access_end(); > > + > > + /* Save the siginfo outside of the unsafe block. */ > > + if (copy_siginfo_to_user(&frame->info, &ksig->info)) > > + goto badframe; > > + > > /* Allocate a dummy caller frame for the signal handler. */ > > newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE; > > err |= put_user(regs->gpr[1], (unsigned long __user *)newsp); > > @@ -939,6 +937,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > > > > return 0; > > > > +badframe_block: > > + user_write_access_end(); > > badframe: > > signal_fault(current, regs, "handle_rt_signal64", frame); > > > > > > > Christophe
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c index 6d4f7a5c4fbf..3b97e3681a8f 100644 --- a/arch/powerpc/kernel/signal_64.c +++ b/arch/powerpc/kernel/signal_64.c @@ -843,46 +843,42 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, /* Save the thread's msr before get_tm_stackpointer() changes it */ unsigned long msr = regs->msr; #endif - frame = get_sigframe(ksig, tsk, sizeof(*frame), 0); - if (!access_ok(frame, sizeof(*frame))) + if (!user_write_access_begin(frame, sizeof(*frame))) goto badframe; - err |= __put_user(&frame->info, &frame->pinfo); - err |= __put_user(&frame->uc, &frame->puc); - err |= copy_siginfo_to_user(&frame->info, &ksig->info); - if (err) - goto badframe; + unsafe_put_user(&frame->info, &frame->pinfo, badframe_block); + unsafe_put_user(&frame->uc, &frame->puc, badframe_block); /* Create the ucontext. */ - err |= __put_user(0, &frame->uc.uc_flags); - err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]); + unsafe_put_user(0, &frame->uc.uc_flags, badframe_block); + unsafe_save_altstack(&frame->uc.uc_stack, regs->gpr[1], badframe_block); + #ifdef CONFIG_PPC_TRANSACTIONAL_MEM if (MSR_TM_ACTIVE(msr)) { /* The ucontext_t passed to userland points to the second * ucontext_t (for transactional state) with its uc_link ptr. */ - err |= __put_user(&frame->uc_transact, &frame->uc.uc_link); + unsafe_put_user(&frame->uc_transact, &frame->uc.uc_link, badframe_block); + user_write_access_end(); err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext, &frame->uc_transact.uc_mcontext, tsk, ksig->sig, NULL, (unsigned long)ksig->ka.sa.sa_handler, msr); + if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) + goto badframe; + } else #endif { - err |= __put_user(0, &frame->uc.uc_link); - - if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) - return -EFAULT; - err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk, - ksig->sig, NULL, - (unsigned long)ksig->ka.sa.sa_handler, 1); - user_write_access_end(); + unsafe_put_user(0, &frame->uc.uc_link, badframe_block); + unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig, + NULL, (unsigned long)ksig->ka.sa.sa_handler, + 1, badframe_block); } - err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set)); - if (err) - goto badframe; + + unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block); /* Make sure signal handler doesn't get spurious FP exceptions */ tsk->thread.fp_state.fpscr = 0; @@ -891,15 +887,17 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) { regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp; } else { - if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) - return -EFAULT; - err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); - user_write_access_end(); - if (err) - goto badframe; + unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0], + badframe_block); regs->nip = (unsigned long) &frame->tramp[0]; } + user_write_access_end(); + + /* Save the siginfo outside of the unsafe block. */ + if (copy_siginfo_to_user(&frame->info, &ksig->info)) + goto badframe; + /* Allocate a dummy caller frame for the signal handler. */ newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE; err |= put_user(regs->gpr[1], (unsigned long __user *)newsp); @@ -939,6 +937,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, return 0; +badframe_block: + user_write_access_end(); badframe: signal_fault(current, regs, "handle_rt_signal64", frame);