Re: [PATCH 6/8] powerpc/signal64: Replace setup_trampoline() w/ unsafe_setup_trampoline()

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

Le 20/10/2020 à 04:42, Christopher M. Riedl a écrit :
> On Fri Oct 16, 2020 at 10:56 AM CDT, Christophe Leroy wrote:
>>
>>
>> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
>>> From: Daniel Axtens <dja@axtens.net>
>>>
>>> Previously setup_trampoline() performed a costly KUAP switch on every
>>> uaccess operation. These repeated uaccess switches cause a significant
>>> drop in signal handling performance.
>>>
>>> Rewrite setup_trampoline() to assume that a userspace write access
>>> window is open. Replace all uaccess functions with their 'unsafe'
>>> versions to avoid the repeated uaccess switches.
>>>
>>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>>> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
>>> ---
>>>    arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++-------------
>>>    1 file changed, 19 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>>> index bd92064e5576..6d4f7a5c4fbf 100644
>>> --- a/arch/powerpc/kernel/signal_64.c
>>> +++ b/arch/powerpc/kernel/signal_64.c
>>> @@ -600,30 +600,33 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
>>>    /*
>>>     * Setup the trampoline code on the stack
>>>     */
>>> -static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp)
>>> +#define unsafe_setup_trampoline(syscall, tramp, e) \
>>> +	unsafe_op_wrap(__unsafe_setup_trampoline(syscall, tramp), e)
>>> +static long notrace __unsafe_setup_trampoline(unsigned int syscall,
>>> +					unsigned int __user *tramp)
>>>    {
>>>    	int i;
>>> -	long err = 0;
>>>    
>>>    	/* bctrl # call the handler */
>>> -	err |= __put_user(PPC_INST_BCTRL, &tramp[0]);
>>> +	unsafe_put_user(PPC_INST_BCTRL, &tramp[0], err);
>>>    	/* addi r1, r1, __SIGNAL_FRAMESIZE  # Pop the dummy stackframe */
>>> -	err |= __put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
>>> -			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1]);
>>> +	unsafe_put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
>>> +			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1], err);
>>>    	/* li r0, __NR_[rt_]sigreturn| */
>>> -	err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2]);
>>> +	unsafe_put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2], err);
>>>    	/* sc */
>>> -	err |= __put_user(PPC_INST_SC, &tramp[3]);
>>> +	unsafe_put_user(PPC_INST_SC, &tramp[3], err);
>>>    
>>>    	/* Minimal traceback info */
>>>    	for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++)
>>> -		err |= __put_user(0, &tramp[i]);
>>> +		unsafe_put_user(0, &tramp[i], err);
>>>    
>>> -	if (!err)
>>> -		flush_icache_range((unsigned long) &tramp[0],
>>> -			   (unsigned long) &tramp[TRAMP_SIZE]);
>>> +	flush_icache_range((unsigned long)&tramp[0],
>>> +			   (unsigned long)&tramp[TRAMP_SIZE]);
>>
>> This flush should be done outside the user_write_access block.
>>
> 
> Hmm, I suppose that means setup_trampoline() cannot be completely
> "unsafe". I'll see if I can re-arrange the code which calls this
> function to avoid an additional uaccess block instead and push the
> start()/end() into setup_trampoline() directly.

I think we shouldn't put too much effort on setup_trampoline(). Nowadays 99.999% of applications use 
the VDSO. Using the trampoline on stack requires to unmap the VDSO and remap the STACK RW. That's 
really a corner case, I think it would be good enough to call it outside the main access begin/end 
block, and let it do its own access_begin/end.

This corner functionnality can be tested using the sigreturn_vdso selftest in selftests/powerpc/signal/

Christophe

> 
>>>    
>>> -	return err;
>>> +	return 0;
>>> +err:
>>> +	return 1;
>>>    }
>>>    
>>>    /*
>>> @@ -888,7 +891,10 @@ 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 {
>>> -		err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
>>> +		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;
>>>    		regs->nip = (unsigned long) &frame->tramp[0];
>>>
>>
>> Christophe
>