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 <[email protected]>
---
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:
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 <[email protected]>
> ---
> 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:
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 <[email protected]>
> ---
> 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 <[email protected]>
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
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.
> 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 <[email protected]>
>> ---
>> 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: