Re: [PATCH 7/8] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches
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
> >
> > 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
> > Signed-off-by: Christopher M. Riedl
> > ---
> > 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);
> >
Re: [PATCH 7/8] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches
Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
From: Daniel Axtens
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
Signed-off-by: Christopher M. Riedl
---
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
[PATCH 7/8] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches
From: Daniel Axtens
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
Signed-off-by: Christopher M. Riedl
---
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();
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 k
