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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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 ?

> -			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