Excerpts from Michael Ellerman's message of June 10, 2021 5:29 pm: > When delivering a signal to a sigaction style handler (SA_SIGINFO), we > pass pointers to the siginfo and ucontext via r4 and r5. > > Currently we populate the values in those registers by reading the > pointers out of the sigframe in user memory, even though the values in > user memory were written by the kernel just prior: > > unsafe_put_user(&frame->info, &frame->pinfo, badframe_block); > unsafe_put_user(&frame->uc, &frame->puc, badframe_block); > ... > if (ksig->ka.sa.sa_flags & SA_SIGINFO) { > err |= get_user(regs->gpr[4], (unsigned long __user *)&frame->pinfo); > err |= get_user(regs->gpr[5], (unsigned long __user *)&frame->puc); > > ie. we write &frame->info into frame->pinfo, and then read frame->pinfo > back into r4, and similarly for &frame->uc. > > The code has always been like this, since linux-fullhistory commit > d4f2d95eca2c ("Forward port of 2.4 ppc64 signal changes."). > > There's no reason for us to read the values back from user memory, > rather than just setting the value in the gpr[4/5] directly. In fact > reading the value back from user memory opens up the possibility of > another user thread changing the values before we read them back. > Although any process doing that would be racing against the kernel > delivering the signal, and would risk corrupting the stack, so that > would be a userspace bug. > > Note that this is 64-bit only code, so there's no subtlety with the size > of pointers differing between kernel and user. Also the frame variable > is not modified to point elsewhere during the function. > > In the past reading the values back from user memory was not costly, but > now that we have KUAP on some CPUs it is, so we'd rather avoid it for > that reason too. > > So change the code to just set the values directly, using the same > values we have written to the sigframe previously in the function. > > Note also that this matches what our 32-bit signal code does. > > Using a version of will-it-scale's signal1_threads that sets SA_SIGINFO, > this results in a ~4% increase in signals per second on a Power9, from > 229,777 to 239,766.
Good find, nice improvement. Will make it possible to make the error handling much nicer too I think. Reviewed-by: Nicholas Piggin <npig...@gmail.com> You've moved copy_siginfo_to_user right up to the user access unlock, could save 2 more KUAP lock/unlocks if we had an unsafe_clear_user. If we can move the other user access stuff up as well, the stack frame put_user could use unsafe_put_user as well, saving 1 more. Another few percent? > > Signed-off-by: Michael Ellerman <m...@ellerman.id.au> > --- > arch/powerpc/kernel/signal_64.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > index dca66481d0c2..f58e7a98d0df 100644 > --- a/arch/powerpc/kernel/signal_64.c > +++ b/arch/powerpc/kernel/signal_64.c > @@ -948,8 +948,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t > *set, > regs->gpr[3] = ksig->sig; > regs->result = 0; > if (ksig->ka.sa.sa_flags & SA_SIGINFO) { > - err |= get_user(regs->gpr[4], (unsigned long __user > *)&frame->pinfo); > - err |= get_user(regs->gpr[5], (unsigned long __user > *)&frame->puc); > + regs->gpr[4] = (unsigned long)&frame->info; > + regs->gpr[5] = (unsigned long)&frame->uc; > regs->gpr[6] = (unsigned long) frame; > } else { > regs->gpr[4] = (unsigned long)&frame->uc.uc_mcontext; > -- > 2.25.1 > >