[PATCH 7/8] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches

[Christopher M. Riedl]

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 at 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 at axtens.net>
> > Signed-off-by: Christopher M. Riedl <cmr at 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