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

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

[Michael Ellerman]

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.

Signed-off-by: Michael Ellerman <mpe@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

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

[Nicholas Piggin]

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?


> 
> Signed-off-by: Michael Ellerman <mpe@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
> 
> 

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

[Christophe Leroy]

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.

> 
> 
>>
>> Signed-off-by: Michael Ellerman <mpe@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
>>
>>

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

[Nicholas Piggin]

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.

Thanks,
Nick

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

[Christophe Leroy]

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.

Christophe

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

[Christophe Leroy]

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.

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

[Michael Ellerman]

On Thu, 10 Jun 2021 17:29:49 +1000, Michael Ellerman wrote:
> 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:
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/signal64: Don't read sigaction arguments back from user memory
      https://git.kernel.org/powerpc/c/a3309226454a7e76d76251579c1183787694f303

cheers