r9 is used to determine whether syscall restarting must be performed or
not. Unfortunately, r9 is never set to zero in the non-syscall path,
and r9 is on top of that a callee-saved register which can be set to
non-zero by the C functions that are called during IRQ handling.
This means that if r10 (used for the syscall return value) is one of the
-ERESTART* values when a hardware interrupt occurs which leads to a
signal being delivered to the process, the kernel will "restart" a
syscall which never occurred. This will lead to the PC being moved back
by 2 on return to user space.
Fix the problem by setting r9 to zero in the interrupt path.
Test case (should loop forever but ends up executing the break 8 trap
instruction):
#include <signal.h>
#include <stdlib.h>
#include <sys/time.h>
void f(int n)
{
register int r9 asm ("r9") = 1;
register int r10 asm ("r10") = n;
__asm__ __volatile__(
"ba 1f \n"
"nop \n"
"break 8 \n"
"1: ba . \n"
"nop \n"
:
: "r" (r9), "r" (r10)
: "memory");
}
void handler1(int sig) { }
int main(int argc, char *argv[])
{
struct itimerval t1 = { .it_value = {1} };
signal(SIGALRM, handler1);
setitimer(ITIMER_REAL, &t1, NULL);
f(-513); /* -ERESTARTNOINTR */
return 0;
}
Signed-off-by: Rabin Vincent <[email protected]>
---
arch/cris/arch-v32/kernel/entry.S | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/cris/arch-v32/kernel/entry.S b/arch/cris/arch-v32/kernel/entry.S
index 2f19ac6..d4c088b 100644
--- a/arch/cris/arch-v32/kernel/entry.S
+++ b/arch/cris/arch-v32/kernel/entry.S
@@ -99,6 +99,8 @@ ret_from_kernel_thread:
.type ret_from_intr,@function
ret_from_intr:
+ moveq 0, $r9 ; not a syscall
+
;; Check for resched if preemptive kernel, or if we're going back to
;; user-mode. This test matches the user_regs(regs) macro. Don't simply
;; test CCS since that doesn't necessarily reflect what mode we'll
--
2.1.4
Al Viro noted that CRIS is vulnerable to bogus restarts on sigreturn.
The fixes CRISv32 by using regs->exs as an additional indicator to
whether we should attempt to restart the syscall or not. EXS is only
used in the sigtrap handling, and in that path we already have r9 (the
other indicator, which indicates if we're in a syscall or not) cleared.
Test case, a port of Al's ARM version from 653d48b22166db2d8 ("arm: fix
really nasty sigreturn bug"):
#include <unistd.h>
#include <signal.h>
#include <stdlib.h>
#include <sys/time.h>
#include <errno.h>
void f(int n)
{
register int r10 asm ("r10") = n;
__asm__ __volatile__(
"ba 1f \n"
"nop \n"
"break 8 \n"
"1: ba . \n"
"nop \n"
:
: "r" (r10)
: "memory");
}
void handler1(int sig) { }
void handler2(int sig) { raise(1); }
void handler3(int sig) { exit(0); }
int main(int argc, char *argv[])
{
struct sigaction s = {.sa_handler = handler2};
struct itimerval t1 = { .it_value = {1} };
struct itimerval t2 = { .it_value = {2} };
signal(1, handler1);
sigemptyset(&s.sa_mask);
sigaddset(&s.sa_mask, 1);
sigaction(SIGALRM, &s, NULL);
signal(SIGVTALRM, handler3);
setitimer(ITIMER_REAL, &t1, NULL);
setitimer(ITIMER_VIRTUAL, &t2, NULL);
f(-513); /* -ERESTARTNOINTR */
return 0;
}
Reported-by: Al Viro <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Rabin Vincent <[email protected]>
---
arch/cris/arch-v32/kernel/entry.S | 5 +++--
arch/cris/arch-v32/kernel/signal.c | 5 +++++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/cris/arch-v32/kernel/entry.S b/arch/cris/arch-v32/kernel/entry.S
index d4c088b..1ea29b7 100644
--- a/arch/cris/arch-v32/kernel/entry.S
+++ b/arch/cris/arch-v32/kernel/entry.S
@@ -147,7 +147,7 @@ system_call:
;; Stack-frame similar to the irq heads, which is reversed in
;; ret_from_sys_call.
- sub.d 92, $sp ; Skip EXS and EDA.
+ sub.d 92, $sp ; Skip EDA.
movem $r13, [$sp]
move.d $sp, $r8
addq 14*4, $r8
@@ -158,8 +158,9 @@ system_call:
move $ccs, $r4
move $srp, $r5
move $erp, $r6
+ move.d $r9, $r7 ; Store syscall number in EXS
subq 4, $sp
- movem $r6, [$r8]
+ movem $r7, [$r8]
ei ; Enable interrupts while processing syscalls.
move.d $r10, [$sp]
diff --git a/arch/cris/arch-v32/kernel/signal.c b/arch/cris/arch-v32/kernel/signal.c
index 78ce3b1..06f48f7 100644
--- a/arch/cris/arch-v32/kernel/signal.c
+++ b/arch/cris/arch-v32/kernel/signal.c
@@ -72,6 +72,9 @@ restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc)
/* Make that the user-mode flag is set. */
regs->ccs |= (1 << (U_CCS_BITNR + CCS_SHIFT));
+ /* Don't perform syscall restarting */
+ regs->exs = -1;
+
/* Restore the old USP. */
err |= __get_user(old_usp, &sc->usp);
wrusp(old_usp);
@@ -427,6 +430,8 @@ do_signal(int canrestart, struct pt_regs *regs)
{
struct ksignal ksig;
+ canrestart = canrestart && ((int)regs->exs >= 0);
+
/*
* The common case should go fast, which is why this point is
* reached from kernel-mode. If that's the case, just return
--
2.1.4
Al Viro noted that CRIS fails to handle multiple signals.
This fixes the problem for CRISv32 by making it use a C work_pending
handling loop similar to the ARM implementation in 0a267fa6a15d41c
("ARM: 7472/1: pull all work_pending logics into C function").
This also happens to fixes the warnings which currently trigger on
CRISv32 due to do_signal() being called with interrupts disabled.
Test case (should die of the SIGSEGV which gets raised when setting up
the stack for SIGALRM, but instead reaches and executes the _exit(1)):
#include <unistd.h>
#include <signal.h>
#include <sys/time.h>
#include <err.h>
static void handler(int sig) { }
int main(int argc, char *argv[])
{
int ret;
struct itimerval t1 = { .it_value = {1} };
stack_t ss = {
.ss_sp = NULL,
.ss_size = SIGSTKSZ,
};
struct sigaction action = {
.sa_handler = handler,
.sa_flags = SA_ONSTACK,
};
ret = sigaltstack(&ss, NULL);
if (ret < 0)
err(1, "sigaltstack");
sigaction(SIGALRM, &action, NULL);
setitimer(ITIMER_REAL, &t1, NULL);
pause();
_exit(1);
return 0;
}
Reported-by: Al Viro <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Rabin Vincent <[email protected]>
---
arch/cris/arch-v32/kernel/entry.S | 35 +++--------------------------------
arch/cris/kernel/ptrace.c | 23 +++++++++++++++++++++++
2 files changed, 26 insertions(+), 32 deletions(-)
diff --git a/arch/cris/arch-v32/kernel/entry.S b/arch/cris/arch-v32/kernel/entry.S
index 1ea29b7..026a0b2 100644
--- a/arch/cris/arch-v32/kernel/entry.S
+++ b/arch/cris/arch-v32/kernel/entry.S
@@ -281,43 +281,14 @@ _syscall_exit_work:
.type _work_pending,@function
_work_pending:
addoq +TI_flags, $r0, $acr
- move.d [$acr], $r10
- btstq TIF_NEED_RESCHED, $r10 ; Need resched?
- bpl _work_notifysig ; No, must be signal/notify.
- nop
- .size _work_pending, . - _work_pending
-
- .type _work_resched,@function
-_work_resched:
- move.d $r9, $r1 ; Preserve R9.
- jsr schedule
- nop
- move.d $r1, $r9
- di
-
- addoq +TI_flags, $r0, $acr
- move.d [$acr], $r1
- and.d _TIF_WORK_MASK, $r1 ; Ignore sycall trace counter.
- beq _Rexit
- nop
- btstq TIF_NEED_RESCHED, $r1
- bmi _work_resched ; current->work.need_resched.
- nop
- .size _work_resched, . - _work_resched
-
- .type _work_notifysig,@function
-_work_notifysig:
- ;; Deal with pending signals and notify-resume requests.
-
- addoq +TI_flags, $r0, $acr
move.d [$acr], $r12 ; The thread_info_flags parameter.
move.d $sp, $r11 ; The regs param.
- jsr do_notify_resume
- move.d $r9, $r10 ; do_notify_resume syscall/irq param.
+ jsr do_work_pending
+ move.d $r9, $r10 ; The syscall/irq param.
ba _Rexit
nop
- .size _work_notifysig, . - _work_notifysig
+ .size _work_pending, . - _work_pending
;; We get here as a sidetrack when we've entered a syscall with the
;; trace-bit set. We need to call do_syscall_trace and then continue
diff --git a/arch/cris/kernel/ptrace.c b/arch/cris/kernel/ptrace.c
index 58d44ee..fd3427e 100644
--- a/arch/cris/kernel/ptrace.c
+++ b/arch/cris/kernel/ptrace.c
@@ -42,3 +42,26 @@ void do_notify_resume(int canrestart, struct pt_regs *regs,
tracehook_notify_resume(regs);
}
}
+
+void do_work_pending(int syscall, struct pt_regs *regs,
+ unsigned int thread_flags)
+{
+ do {
+ if (likely(thread_flags & _TIF_NEED_RESCHED)) {
+ schedule();
+ } else {
+ if (unlikely(!user_mode(regs)))
+ return;
+ local_irq_enable();
+ if (thread_flags & _TIF_SIGPENDING) {
+ do_signal(syscall, regs);
+ syscall = 0;
+ } else {
+ clear_thread_flag(TIF_NOTIFY_RESUME);
+ tracehook_notify_resume(regs);
+ }
+ }
+ local_irq_disable();
+ thread_flags = current_thread_info()->flags;
+ } while (thread_flags & _TIF_WORK_MASK);
+}
--
2.1.4
On Sun, Feb 08, 2015 at 09:45:04PM +0100, Rabin Vincent wrote:
> Al Viro noted that CRIS fails to handle multiple signals.
>
> This fixes the problem for CRISv32 by making it use a C work_pending
> handling loop similar to the ARM implementation in 0a267fa6a15d41c
> ("ARM: 7472/1: pull all work_pending logics into C function").
>
> This also happens to fixes the warnings which currently trigger on
> CRISv32 due to do_signal() being called with interrupts disabled.
>
> Test case (should die of the SIGSEGV which gets raised when setting up
> the stack for SIGALRM, but instead reaches and executes the _exit(1)):
>
> #include <unistd.h>
> #include <signal.h>
> #include <sys/time.h>
> #include <err.h>
>
> static void handler(int sig) { }
>
> int main(int argc, char *argv[])
> {
> int ret;
> struct itimerval t1 = { .it_value = {1} };
> stack_t ss = {
> .ss_sp = NULL,
> .ss_size = SIGSTKSZ,
> };
> struct sigaction action = {
> .sa_handler = handler,
> .sa_flags = SA_ONSTACK,
> };
>
> ret = sigaltstack(&ss, NULL);
> if (ret < 0)
> err(1, "sigaltstack");
>
> sigaction(SIGALRM, &action, NULL);
> setitimer(ITIMER_REAL, &t1, NULL);
>
> pause();
>
> _exit(1);
>
> return 0;
> }
>
> Reported-by: Al Viro <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: Rabin Vincent <[email protected]>
Hi Rabin,
Works nicely, and, yes, it does fix the annoying traceback.
Tested-by: Guenter Roeck <[email protected]>
Wondering - what serial driver do you use with crisv32, if any ?
So far I always patch in a cut-down version of the driver
from 2.6.26/33 which was never submitted upstream.
Thanks,
Guenter
On Sun, Feb 08, 2015 at 09:45:02PM +0100, Rabin Vincent wrote:
> r9 is used to determine whether syscall restarting must be performed or
> not. Unfortunately, r9 is never set to zero in the non-syscall path,
> and r9 is on top of that a callee-saved register which can be set to
> non-zero by the C functions that are called during IRQ handling.
>
> This means that if r10 (used for the syscall return value) is one of the
> -ERESTART* values when a hardware interrupt occurs which leads to a
> signal being delivered to the process, the kernel will "restart" a
> syscall which never occurred. This will lead to the PC being moved back
> by 2 on return to user space.
>
> Fix the problem by setting r9 to zero in the interrupt path.
>
> Test case (should loop forever but ends up executing the break 8 trap
> instruction):
>
> #include <signal.h>
> #include <stdlib.h>
> #include <sys/time.h>
>
> void f(int n)
> {
> register int r9 asm ("r9") = 1;
> register int r10 asm ("r10") = n;
>
> __asm__ __volatile__(
> "ba 1f \n"
> "nop \n"
> "break 8 \n"
> "1: ba . \n"
> "nop \n"
> :
> : "r" (r9), "r" (r10)
> : "memory");
> }
>
> void handler1(int sig) { }
>
> int main(int argc, char *argv[])
> {
> struct itimerval t1 = { .it_value = {1} };
>
> signal(SIGALRM, handler1);
> setitimer(ITIMER_REAL, &t1, NULL);
>
> f(-513); /* -ERESTARTNOINTR */
>
> return 0;
> }
>
> Signed-off-by: Rabin Vincent <[email protected]>
Nice, added in the CRIS tree for 3.20.
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
On Sun, Feb 08, 2015 at 09:45:03PM +0100, Rabin Vincent wrote:
> Al Viro noted that CRIS is vulnerable to bogus restarts on sigreturn.
> Reported-by: Al Viro <[email protected]>
> Signed-off-by: Rabin Vincent <[email protected]>
Nice, added in the CRIS tree for 3.20.
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
On Sun, Feb 08, 2015 at 09:45:04PM +0100, Rabin Vincent wrote:
> Al Viro noted that CRIS fails to handle multiple signals.
>
> This fixes the problem for CRISv32 by making it use a C work_pending
> handling loop similar to the ARM implementation in 0a267fa6a15d41c
> ("ARM: 7472/1: pull all work_pending logics into C function").
>
> This also happens to fixes the warnings which currently trigger on
> CRISv32 due to do_signal() being called with interrupts disabled.
> Reported-by: Al Viro <[email protected]>
> Signed-off-by: Rabin Vincent <[email protected]>
Nice, added in the CRIS tree for 3.20.
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
On Sun, Feb 08, 2015 at 06:03:49PM -0800, Guenter Roeck wrote:
> On Sun, Feb 08, 2015 at 09:45:04PM +0100, Rabin Vincent wrote:
> > Al Viro noted that CRIS fails to handle multiple signals.
[snip]
> > Reported-by: Al Viro <[email protected]>
> > Link: http://lkml.kernel.org/r/[email protected]
> > Signed-off-by: Rabin Vincent <[email protected]>
>
> Hi Rabin,
>
> Works nicely, and, yes, it does fix the annoying traceback.
>
> Tested-by: Guenter Roeck <[email protected]>
>
> Wondering - what serial driver do you use with crisv32, if any ?
> So far I always patch in a cut-down version of the driver
> from 2.6.26/33 which was never submitted upstream.
Niclas Cassel has submitted a reworked driver for crisv32,
which is in Gregs tree now, unfortunately it wasn't sent to LKML,
only the linux-serial list, but it should be headed to Linus soon.
http://www.spinics.net/lists/linux-serial/msg15619.html
> Thanks,
> Guenter
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
On 02/09/2015 01:57 AM, Jesper Nilsson wrote:
> On Sun, Feb 08, 2015 at 06:03:49PM -0800, Guenter Roeck wrote:
>> On Sun, Feb 08, 2015 at 09:45:04PM +0100, Rabin Vincent wrote:
>>> Al Viro noted that CRIS fails to handle multiple signals.
> [snip]
>>> Reported-by: Al Viro <[email protected]>
>>> Link: http://lkml.kernel.org/r/[email protected]
>>> Signed-off-by: Rabin Vincent <[email protected]>
>>
>> Hi Rabin,
>>
>> Works nicely, and, yes, it does fix the annoying traceback.
>>
>> Tested-by: Guenter Roeck <[email protected]>
>>
>> Wondering - what serial driver do you use with crisv32, if any ?
>> So far I always patch in a cut-down version of the driver
>> from 2.6.26/33 which was never submitted upstream.
>
> Niclas Cassel has submitted a reworked driver for crisv32,
> which is in Gregs tree now, unfortunately it wasn't sent to LKML,
> only the linux-serial list, but it should be headed to Linus soon.
>
It is in Greg's tty-testing tree, not in tty-next. Guess that is
why I missed it.
Looks pretty good. I'll give it a try.
Thanks!
Guenter
On Mon, Feb 09, 2015 at 10:57:27AM +0100, Jesper Nilsson wrote:
>
> Niclas Cassel has submitted a reworked driver for crisv32,
> which is in Gregs tree now, unfortunately it wasn't sent to LKML,
> only the linux-serial list, but it should be headed to Linus soon.
>
> http://www.spinics.net/lists/linux-serial/msg15619.html
>
I fetched it from Greg's tree and gave it a test. Works nicely
when combined with the other pending patches for crisv32.
Different question - do you by any chance have a working configuration
for crisv10 in combination with qemu ?
Thanks,
Guenter
On Mon, Feb 09, 2015 at 05:57:03PM +0100, Guenter Roeck wrote:
> On Mon, Feb 09, 2015 at 10:57:27AM +0100, Jesper Nilsson wrote:
> >
> > Niclas Cassel has submitted a reworked driver for crisv32,
> > which is in Gregs tree now, unfortunately it wasn't sent to LKML,
> > only the linux-serial list, but it should be headed to Linus soon.
> >
> > http://www.spinics.net/lists/linux-serial/msg15619.html
> >
> I fetched it from Greg's tree and gave it a test. Works nicely
> when combined with the other pending patches for crisv32.
>
> Different question - do you by any chance have a working configuration
> for crisv10 in combination with qemu ?
I'm afraid not, there are large similarities between the
two, but enough to make them separate architectures.
I know that Edgar Iglesias said that it would not be very hard,
perhaps he has some ideas? (added CC)
> Thanks,
> Guenter
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
On 02/10/2015 03:04 AM, Edgar E. Iglesias wrote:
> Hi,
>
> Sorry for top posting, on my phone.
>
> Qemu has support for the crisv10 CPU but lacks the v10 MMU setup. The MMU models are basically available as the v10 mmu is very similar to the v32 mmu IIRC.
>
> It's a matter of putting in the effort to wire everything up. The v10 setup interacts with the mmu via mmio while the v32 has special regs/insns for accessing the mmu. There might be other difficulties I haven't thought of though.
>
Too bad. And even if someone would spend the time it looks like it is becoming
very difficult to get changes accepted to qemu nowadays.
One step at a time ... I would be happy if we can get crisv32 to the point
where I can run my automated qemu tests on it with unpatched kernels.
Thanks,
Guenter