[PATCH] riscv: fix a nasty sigreturn bug...

[PATCH] riscv: fix a nasty sigreturn bug...

[Al Viro]

riscv has an equivalent of arm bug fixed by 653d48b22166; if signal
gets caught by an interrupt that hits when we have the right value
in a0 (-513), *and* another signal gets delivered upon sigreturn()
(e.g. included into the blocked mask for the first signal and posted
while the handler had been running), the syscall restart logics will
see regs->cause equal to EXC_SYSCALL (we are in a syscall, after all)
and a0 already restored to its original value (-513, which happens to
be -ERESTARTNOINTR) and assume that we need to apply the usual
syscall restart logics.
    
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
index c2d5ecbe55264..f8fb85dc94b7a 100644
--- a/arch/riscv/kernel/signal.c
+++ b/arch/riscv/kernel/signal.c
@@ -121,6 +121,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	if (restore_altstack(&frame->uc.uc_stack))
 		goto badframe;
 
+	regs->cause = -1UL;
+
 	return regs->a0;
 
 badframe:

[PATCH] riscv: fix a nasty sigreturn bug...

[Al Viro]

riscv has an equivalent of arm bug fixed by 653d48b22166; if signal
gets caught by an interrupt that hits when we have the right value
in a0 (-513), *and* another signal gets delivered upon sigreturn()
(e.g. included into the blocked mask for the first signal and posted
while the handler had been running), the syscall restart logics will
see regs->cause equal to EXC_SYSCALL (we are in a syscall, after all)
and a0 already restored to its original value (-513, which happens to
be -ERESTARTNOINTR) and assume that we need to apply the usual
syscall restart logics.
    
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
index c2d5ecbe55264..f8fb85dc94b7a 100644
--- a/arch/riscv/kernel/signal.c
+++ b/arch/riscv/kernel/signal.c
@@ -121,6 +121,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	if (restore_altstack(&frame->uc.uc_stack))
 		goto badframe;
 
+	regs->cause = -1UL;
+
 	return regs->a0;
 
 badframe:

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: fix a nasty sigreturn bug...

[Al Viro]

Ping?  Does anybody have objections?  AFAICS, the bug is still
there...

On Fri, Sep 24, 2021 at 01:55:27AM +0000, Al Viro wrote:
> riscv has an equivalent of arm bug fixed by 653d48b22166; if signal
> gets caught by an interrupt that hits when we have the right value
> in a0 (-513), *and* another signal gets delivered upon sigreturn()
> (e.g. included into the blocked mask for the first signal and posted
> while the handler had been running), the syscall restart logics will
> see regs->cause equal to EXC_SYSCALL (we are in a syscall, after all)
> and a0 already restored to its original value (-513, which happens to
> be -ERESTARTNOINTR) and assume that we need to apply the usual
> syscall restart logics.
>     
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> index c2d5ecbe55264..f8fb85dc94b7a 100644
> --- a/arch/riscv/kernel/signal.c
> +++ b/arch/riscv/kernel/signal.c
> @@ -121,6 +121,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  	if (restore_altstack(&frame->uc.uc_stack))
>  		goto badframe;
>  
> +	regs->cause = -1UL;
> +
>  	return regs->a0;
>  
>  badframe:

Re: [PATCH] riscv: fix a nasty sigreturn bug...

[Al Viro]

Ping?  Does anybody have objections?  AFAICS, the bug is still
there...

On Fri, Sep 24, 2021 at 01:55:27AM +0000, Al Viro wrote:
> riscv has an equivalent of arm bug fixed by 653d48b22166; if signal
> gets caught by an interrupt that hits when we have the right value
> in a0 (-513), *and* another signal gets delivered upon sigreturn()
> (e.g. included into the blocked mask for the first signal and posted
> while the handler had been running), the syscall restart logics will
> see regs->cause equal to EXC_SYSCALL (we are in a syscall, after all)
> and a0 already restored to its original value (-513, which happens to
> be -ERESTARTNOINTR) and assume that we need to apply the usual
> syscall restart logics.
>     
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> index c2d5ecbe55264..f8fb85dc94b7a 100644
> --- a/arch/riscv/kernel/signal.c
> +++ b/arch/riscv/kernel/signal.c
> @@ -121,6 +121,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  	if (restore_altstack(&frame->uc.uc_stack))
>  		goto badframe;
>  
> +	regs->cause = -1UL;
> +
>  	return regs->a0;
>  
>  badframe:

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: fix a nasty sigreturn bug...

[Andrew Jones]

On Fri, Sep 24, 2021 at 01:55:27AM +0000, Al Viro wrote:
> riscv has an equivalent of arm bug fixed by 653d48b22166; if signal
> gets caught by an interrupt that hits when we have the right value
> in a0 (-513), *and* another signal gets delivered upon sigreturn()
> (e.g. included into the blocked mask for the first signal and posted
> while the handler had been running), the syscall restart logics will
> see regs->cause equal to EXC_SYSCALL (we are in a syscall, after all)
> and a0 already restored to its original value (-513, which happens to
> be -ERESTARTNOINTR) and assume that we need to apply the usual
> syscall restart logics.
>     
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> index c2d5ecbe55264..f8fb85dc94b7a 100644
> --- a/arch/riscv/kernel/signal.c
> +++ b/arch/riscv/kernel/signal.c
> @@ -121,6 +121,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  	if (restore_altstack(&frame->uc.uc_stack))
>  		goto badframe;
>  
> +	regs->cause = -1UL;
> +
>  	return regs->a0;
>  
>  badframe:
>

This looks good to me based on what other architectures do.

For example, arm64 does

  rt_sigreturn
    restore_sigframe
      forget_syscall
        regs->syscallno = NO_SYSCALL

  which results in do_signal avoiding syscall restarting

And x86 does

  rt_sigreturn
    restore_sigcontext
      regs->orig_ax = -1

  where its handle_signal only restarts syscalls when regs->orig_ax != -1

So, for riscv, where in do_signal and handle_signal syscall restarting
is avoided when regs->cause != EXC_SYSCALL and it's common to set cause
to -1 to avoid it, then it makes sense to set regs->cause != EXEC_SYSCALL
in rt_sigreturn or in restore_sigcontext, which rt_sigreturn calls, as
well.

So the only question I have is whether or not the cause assignment
is better in restore_sigcontext() like other architectures? At least,
since rt_sigreturn is the only caller of restore_sigcontext, it can't
break anything putting it there atm...

Anyway,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

BTW, I ran the testcase from 653d48b22166 with the asm modified for
riscv for a while over QEMU. It didn't reproduce, but I suppose that
doesn't mean much.

Thanks,
drew

Re: [PATCH] riscv: fix a nasty sigreturn bug...

[Andrew Jones]

On Fri, Sep 24, 2021 at 01:55:27AM +0000, Al Viro wrote:
> riscv has an equivalent of arm bug fixed by 653d48b22166; if signal
> gets caught by an interrupt that hits when we have the right value
> in a0 (-513), *and* another signal gets delivered upon sigreturn()
> (e.g. included into the blocked mask for the first signal and posted
> while the handler had been running), the syscall restart logics will
> see regs->cause equal to EXC_SYSCALL (we are in a syscall, after all)
> and a0 already restored to its original value (-513, which happens to
> be -ERESTARTNOINTR) and assume that we need to apply the usual
> syscall restart logics.
>     
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> index c2d5ecbe55264..f8fb85dc94b7a 100644
> --- a/arch/riscv/kernel/signal.c
> +++ b/arch/riscv/kernel/signal.c
> @@ -121,6 +121,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  	if (restore_altstack(&frame->uc.uc_stack))
>  		goto badframe;
>  
> +	regs->cause = -1UL;
> +
>  	return regs->a0;
>  
>  badframe:
>

This looks good to me based on what other architectures do.

For example, arm64 does

  rt_sigreturn
    restore_sigframe
      forget_syscall
        regs->syscallno = NO_SYSCALL

  which results in do_signal avoiding syscall restarting

And x86 does

  rt_sigreturn
    restore_sigcontext
      regs->orig_ax = -1

  where its handle_signal only restarts syscalls when regs->orig_ax != -1

So, for riscv, where in do_signal and handle_signal syscall restarting
is avoided when regs->cause != EXC_SYSCALL and it's common to set cause
to -1 to avoid it, then it makes sense to set regs->cause != EXEC_SYSCALL
in rt_sigreturn or in restore_sigcontext, which rt_sigreturn calls, as
well.

So the only question I have is whether or not the cause assignment
is better in restore_sigcontext() like other architectures? At least,
since rt_sigreturn is the only caller of restore_sigcontext, it can't
break anything putting it there atm...

Anyway,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

BTW, I ran the testcase from 653d48b22166 with the asm modified for
riscv for a while over QEMU. It didn't reproduce, but I suppose that
doesn't mean much.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: fix a nasty sigreturn bug...

[Al Viro]

On Fri, Sep 02, 2022 at 11:22:45AM +0200, Andrew Jones wrote:

> So, for riscv, where in do_signal and handle_signal syscall restarting
> is avoided when regs->cause != EXC_SYSCALL and it's common to set cause
> to -1 to avoid it, then it makes sense to set regs->cause != EXEC_SYSCALL
> in rt_sigreturn or in restore_sigcontext, which rt_sigreturn calls, as
> well.
> 
> So the only question I have is whether or not the cause assignment
> is better in restore_sigcontext() like other architectures? At least,
> since rt_sigreturn is the only caller of restore_sigcontext, it can't
> break anything putting it there atm...

	The only reason for doing that in restore_sigcontext() is that for
old architectures there'd been separate sigreturn(2) and rt_sigreturn(2).
Doing that in the helper shared by both avoided duplication; since
there's no such thing on riscv...

	Matter of taste, really - I have a slight preference for doing that
closer to the syscall surface, but it's no more than that.

Re: [PATCH] riscv: fix a nasty sigreturn bug...

[Al Viro]

On Fri, Sep 02, 2022 at 11:22:45AM +0200, Andrew Jones wrote:

> So, for riscv, where in do_signal and handle_signal syscall restarting
> is avoided when regs->cause != EXC_SYSCALL and it's common to set cause
> to -1 to avoid it, then it makes sense to set regs->cause != EXEC_SYSCALL
> in rt_sigreturn or in restore_sigcontext, which rt_sigreturn calls, as
> well.
> 
> So the only question I have is whether or not the cause assignment
> is better in restore_sigcontext() like other architectures? At least,
> since rt_sigreturn is the only caller of restore_sigcontext, it can't
> break anything putting it there atm...

	The only reason for doing that in restore_sigcontext() is that for
old architectures there'd been separate sigreturn(2) and rt_sigreturn(2).
Doing that in the helper shared by both avoided duplication; since
there's no such thing on riscv...

	Matter of taste, really - I have a slight preference for doing that
closer to the syscall surface, but it's no more than that.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: fix a nasty sigreturn bug...

[Palmer Dabbelt]

> Ping?  Does anybody have objections?  AFAICS, the bug is still
> there...

Sorry, something's gone off the rails with email and this thread doesn't 
show up in my inbox (not even any of the replies).  I tried to patch 
together this reply manually so hopefully it works.

This is on fixes, thanks -- trying to debug this one would have been a 
nightmare.

> On Fri, Sep 24, 2021 at 01:55:27AM +0000, Al Viro wrote:
>> riscv has an equivalent of arm bug fixed by 653d48b22166; if signal
>> gets caught by an interrupt that hits when we have the right value
>> in a0 (-513), *and* another signal gets delivered upon sigreturn()
>> (e.g. included into the blocked mask for the first signal and posted
>> while the handler had been running), the syscall restart logics will
>> see regs->cause equal to EXC_SYSCALL (we are in a syscall, after all)
>> and a0 already restored to its original value (-513, which happens to
>> be -ERESTARTNOINTR) and assume that we need to apply the usual
>> syscall restart logics.
>>     
>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>> ---
>> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
>> index c2d5ecbe55264..f8fb85dc94b7a 100644
>> --- a/arch/riscv/kernel/signal.c
>> +++ b/arch/riscv/kernel/signal.c
>> @@ -121,6 +121,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>>  	if (restore_altstack(&frame->uc.uc_stack))
>>  		goto badframe;
>>  
>> +	regs->cause = -1UL;
>> +
>>  	return regs->a0;
>>  
>>  badframe:

Re: [PATCH] riscv: fix a nasty sigreturn bug...

[Palmer Dabbelt]

> Ping?  Does anybody have objections?  AFAICS, the bug is still
> there...

Sorry, something's gone off the rails with email and this thread doesn't 
show up in my inbox (not even any of the replies).  I tried to patch 
together this reply manually so hopefully it works.

This is on fixes, thanks -- trying to debug this one would have been a 
nightmare.

> On Fri, Sep 24, 2021 at 01:55:27AM +0000, Al Viro wrote:
>> riscv has an equivalent of arm bug fixed by 653d48b22166; if signal
>> gets caught by an interrupt that hits when we have the right value
>> in a0 (-513), *and* another signal gets delivered upon sigreturn()
>> (e.g. included into the blocked mask for the first signal and posted
>> while the handler had been running), the syscall restart logics will
>> see regs->cause equal to EXC_SYSCALL (we are in a syscall, after all)
>> and a0 already restored to its original value (-513, which happens to
>> be -ERESTARTNOINTR) and assume that we need to apply the usual
>> syscall restart logics.
>>     
>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>> ---
>> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
>> index c2d5ecbe55264..f8fb85dc94b7a 100644
>> --- a/arch/riscv/kernel/signal.c
>> +++ b/arch/riscv/kernel/signal.c
>> @@ -121,6 +121,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>>  	if (restore_altstack(&frame->uc.uc_stack))
>>  		goto badframe;
>>  
>> +	regs->cause = -1UL;
>> +
>>  	return regs->a0;
>>  
>>  badframe:

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv