Re: [PATCH] powerpc/signal64: Don't read sigaction arguments back from user memory

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

Le 14/06/2021 à 13:49, Christophe Leroy a écrit :
> 
> 
> Le 14/06/2021 à 07:49, Nicholas Piggin a écrit :
>> 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 <npiggin@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.
>>
> 
> Finally not so easy. We have a quite efficient clear_user() which uses 'dcbz'. When replacing that 
> by a simplistic unsafe_clear_user() on the same model as unsafe_copy_to_user(), performance are 
> degradated on 32s. Need to implement it more efficiently.
> 

Don't know what I did yesterday. Performance is _not_ degraded, it is improved by 5%. I'll send out 
a series soon.