* [PATCH] riscv: fix a nasty sigreturn bug...
@ 2021-09-24 1:55 ` Al Viro
0 siblings, 0 replies; 10+ messages in thread
From: Al Viro @ 2021-09-24 1:55 UTC (permalink / raw)
To: linux-riscv; +Cc: Linus Torvalds, linux-kernel
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:
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH] riscv: fix a nasty sigreturn bug... @ 2021-09-24 1:55 ` Al Viro 0 siblings, 0 replies; 10+ messages in thread From: Al Viro @ 2021-09-24 1:55 UTC (permalink / raw) To: linux-riscv; +Cc: Linus Torvalds, linux-kernel 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] riscv: fix a nasty sigreturn bug... 2021-09-24 1:55 ` Al Viro @ 2022-09-02 0:13 ` Al Viro -1 siblings, 0 replies; 10+ messages in thread From: Al Viro @ 2022-09-02 0:13 UTC (permalink / raw) To: linux-riscv; +Cc: Linus Torvalds, linux-kernel 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: ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] riscv: fix a nasty sigreturn bug... @ 2022-09-02 0:13 ` Al Viro 0 siblings, 0 replies; 10+ messages in thread From: Al Viro @ 2022-09-02 0:13 UTC (permalink / raw) To: linux-riscv; +Cc: Linus Torvalds, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] riscv: fix a nasty sigreturn bug... 2021-09-24 1:55 ` Al Viro @ 2022-09-02 9:22 ` Andrew Jones -1 siblings, 0 replies; 10+ messages in thread From: Andrew Jones @ 2022-09-02 9:22 UTC (permalink / raw) To: Al Viro; +Cc: linux-riscv, Linus Torvalds, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] riscv: fix a nasty sigreturn bug... @ 2022-09-02 9:22 ` Andrew Jones 0 siblings, 0 replies; 10+ messages in thread From: Andrew Jones @ 2022-09-02 9:22 UTC (permalink / raw) To: Al Viro; +Cc: linux-riscv, Linus Torvalds, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] riscv: fix a nasty sigreturn bug... 2022-09-02 9:22 ` Andrew Jones @ 2022-09-02 17:59 ` Al Viro -1 siblings, 0 replies; 10+ messages in thread From: Al Viro @ 2022-09-02 17:59 UTC (permalink / raw) To: Andrew Jones; +Cc: linux-riscv, Linus Torvalds, linux-kernel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] riscv: fix a nasty sigreturn bug... @ 2022-09-02 17:59 ` Al Viro 0 siblings, 0 replies; 10+ messages in thread From: Al Viro @ 2022-09-02 17:59 UTC (permalink / raw) To: Andrew Jones; +Cc: linux-riscv, Linus Torvalds, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] riscv: fix a nasty sigreturn bug... 2022-09-02 17:59 ` Al Viro @ 2022-09-15 18:48 ` Palmer Dabbelt -1 siblings, 0 replies; 10+ messages in thread From: Palmer Dabbelt @ 2022-09-15 18:48 UTC (permalink / raw) To: viro, ajones; +Cc: Linus Torvalds, linux-kernel, linux-riscv > 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: ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] riscv: fix a nasty sigreturn bug... @ 2022-09-15 18:48 ` Palmer Dabbelt 0 siblings, 0 replies; 10+ messages in thread From: Palmer Dabbelt @ 2022-09-15 18:48 UTC (permalink / raw) To: viro, ajones; +Cc: Linus Torvalds, linux-kernel, linux-riscv > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-09-15 18:48 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-24 1:55 [PATCH] riscv: fix a nasty sigreturn bug Al Viro 2021-09-24 1:55 ` Al Viro 2022-09-02 0:13 ` Al Viro 2022-09-02 0:13 ` Al Viro 2022-09-02 9:22 ` Andrew Jones 2022-09-02 9:22 ` Andrew Jones 2022-09-02 17:59 ` Al Viro 2022-09-02 17:59 ` Al Viro 2022-09-15 18:48 ` Palmer Dabbelt 2022-09-15 18:48 ` Palmer Dabbelt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.