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

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

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.

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