Excerpts from Christophe Leroy's message of June 14, 2021 3:30 pm: > > > Le 14/06/2021 à 03:32, Nicholas Piggin a écrit : >> 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? > > I'm looking at making an 'unsafe' version of copy_siginfo_to_user(). > That's straight forward for 'native' signals, but for compat signals that's > more tricky.
Ah nice. Native is most important at the moment. Thanks, Nick