Re: [PATCH 8/8] powerpc/signal64: Rewrite rt_sigreturn() to minimise uaccess switches

["Christopher M. Riedl" <cmr@codefail.de>]

On Fri Oct 16, 2020 at 11:07 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.
> > 
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >   arch/powerpc/kernel/signal_64.c | 23 +++++++++++++++--------
> >   1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index 3b97e3681a8f..0f4ff7a5bfc1 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -779,18 +779,22 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >   	 */
> >   	regs->msr &= ~MSR_TS_MASK;
> >   
> > -	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> > +	if (!user_read_access_begin(uc, sizeof(*uc)))
> >   		goto badframe;
> > +
> > +	unsafe_get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR], badframe_block);
> > +
> >   	if (MSR_TM_ACTIVE(msr)) {
> >   		/* We recheckpoint on return. */
> >   		struct ucontext __user *uc_transact;
> >   
> >   		/* Trying to start TM on non TM system */
> >   		if (!cpu_has_feature(CPU_FTR_TM))
> > -			goto badframe;
> > +			goto badframe_block;
> > +
> > +		unsafe_get_user(uc_transact, &uc->uc_link, badframe_block);
> > +		user_read_access_end();
>
> user_access_end() only in the if branch ?
>
> >   
> > -		if (__get_user(uc_transact, &uc->uc_link))
> > -			goto badframe;
> >   		if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
> >   					   &uc_transact->uc_mcontext))
> >   			goto badframe;
> > @@ -810,12 +814,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >   		 * causing a TM bad thing.
> >   		 */
> >   		current->thread.regs->msr &= ~MSR_TS_MASK;
> > +
> > +#ifndef CONFIG_PPC_TRANSACTIONAL_MEM
> >   		if (!user_read_access_begin(uc, sizeof(*uc)))
>
> The matching user_read_access_end() is not in the same #ifndef ? That's
> dirty and hard to follow.
> Can you re-organise the code to avoid all those nesting ?

Yes, thanks for pointing this out. I really wanted to avoid changing too
much of the logic inside these functions. But I suppose I ended up
creating a mess - I will fix this in the next spin.

>
> > -			return -EFAULT;
> > -		if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
> > -			user_read_access_end();
> >   			goto badframe;
> > -		}
> > +#endif
> > +		unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext,
> > +					  badframe_block);
> >   		user_read_access_end();
> >   	}
> >   
> > @@ -825,6 +830,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >   	set_thread_flag(TIF_RESTOREALL);
> >   	return 0;
> >   
> > +badframe_block:
> > +	user_read_access_end();
> >   badframe:
> >   	signal_fault(current, regs, "rt_sigreturn", uc);
> >   
> > 
>
> Christophe