[PATCH 1/3] CRISv32: don't attempt syscall restart on irq exit

[PATCH 1/3] CRISv32: don't attempt syscall restart on irq exit

[Rabin Vincent]

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 <rabin@rab.in>
---
 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

[PATCH 2/3] CRISv32: prevent bogus restarts on sigreturn

[Rabin Vincent]

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 <viro@ZenIV.linux.org.uk>
Link: http://lkml.kernel.org/r/20121208074429.GC4939@ZenIV.linux.org.uk
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 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

[PATCH 3/3] CRISv32: handle multiple signals

[Rabin Vincent]

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 <viro@ZenIV.linux.org.uk>
Link: http://lkml.kernel.org/r/20121208074429.GC4939@ZenIV.linux.org.uk
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 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

Re: [PATCH 3/3] CRISv32: handle multiple signals

[Guenter Roeck]

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 <viro@ZenIV.linux.org.uk>
> Link: http://lkml.kernel.org/r/20121208074429.GC4939@ZenIV.linux.org.uk
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Hi Rabin,

Works nicely, and, yes, it does fix the annoying traceback.

Tested-by: Guenter Roeck <linux@roeck-us.net>

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

Re: [PATCH 1/3] CRISv32: don't attempt syscall restart on irq exit

[Jesper Nilsson]

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 <rabin@rab.in>

Nice, added in the CRIS tree for 3.20.

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH 2/3] CRISv32: prevent bogus restarts on sigreturn

[Jesper Nilsson]

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 <viro@ZenIV.linux.org.uk>
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Nice, added in the CRIS tree for 3.20.

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH 3/3] CRISv32: handle multiple signals

[Jesper Nilsson]

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 <viro@ZenIV.linux.org.uk>
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Nice, added in the CRIS tree for 3.20.

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

Re: [PATCH 3/3] CRISv32: handle multiple signals

[Jesper Nilsson]

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 <viro@ZenIV.linux.org.uk>
> > Link: http://lkml.kernel.org/r/20121208074429.GC4939@ZenIV.linux.org.uk
> > Signed-off-by: Rabin Vincent <rabin@rab.in>
> 
> Hi Rabin,
> 
> Works nicely, and, yes, it does fix the annoying traceback.
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> 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 -- jesper.nilsson@axis.com

Re: [PATCH 3/3] CRISv32: handle multiple signals

[Guenter Roeck]

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 <viro@ZenIV.linux.org.uk>
>>> Link: http://lkml.kernel.org/r/20121208074429.GC4939@ZenIV.linux.org.uk
>>> Signed-off-by: Rabin Vincent <rabin@rab.in>
>>
>> Hi Rabin,
>>
>> Works nicely, and, yes, it does fix the annoying traceback.
>>
>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>
>> 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

Re: [PATCH 3/3] CRISv32: handle multiple signals

[Guenter Roeck]

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

Re: [PATCH 3/3] CRISv32: handle multiple signals

[Jesper Nilsson]

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 -- jesper.nilsson@axis.com

Re: [PATCH 3/3] CRISv32: handle multiple signals

[Guenter Roeck]

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