[PATCH] Retry {save,restore}_fp_context if failed in atomic context.

[PATCH] Retry {save,restore}_fp_context if failed in atomic context.

[Atsushi Nemoto]

The save_fp_context()/restore_fp_context() might sleep on accessing
user stack and therefore might lose FPU ownership in middle of them.

If these function failed due to "in_atomic" test in do_page_fault,
touch the sigcontext area in non-atomic context and retry these
save/restore operation.

This is a replacement of a (broken) fix which was titled "Allow CpU
exception in kernel partially".

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
This patch depends on:
> Subject: Re: [PATCH] Disallow CpU exception in kernel again.
> Message-Id: <20070414.023726.128617751.anemo@mba.ocn.ne.jp>

 arch/mips/kernel/signal-common.h |    9 ++++++
 arch/mips/kernel/signal.c        |   52 +++++++++++++++++++++++++++++++------
 arch/mips/kernel/signal32.c      |   52 +++++++++++++++++++++++++++++++------
 include/asm-mips/fpu.h           |    9 +++++-
 4 files changed, 102 insertions(+), 20 deletions(-)

diff --git a/arch/mips/kernel/signal-common.h b/arch/mips/kernel/signal-common.h
index 297dfcb..c0faabd 100644
--- a/arch/mips/kernel/signal-common.h
+++ b/arch/mips/kernel/signal-common.h
@@ -34,4 +34,13 @@ extern int install_sigtramp(unsigned int
 /* Check and clear pending FPU exceptions in saved CSR */
 extern int fpcsr_pending(unsigned int __user *fpcsr);
 
+/* Make sure we will not lose FPU ownership */
+#ifdef CONFIG_PREEMPT
+#define lock_fpu_owner()	preempt_disable()
+#define unlock_fpu_owner()	preempt_enable()
+#else
+#define lock_fpu_owner()	pagefault_disable()
+#define unlock_fpu_owner()	pagefault_enable()
+#endif
+
 #endif	/* __SIGNAL_COMMON_H */
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index fa58119..07d6730 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -20,6 +20,7 @@
 #include <linux/ptrace.h>
 #include <linux/unistd.h>
 #include <linux/compiler.h>
+#include <linux/uaccess.h>
 
 #include <asm/abi.h>
 #include <asm/asm.h>
@@ -27,7 +28,6 @@
 #include <asm/cacheflush.h>
 #include <asm/fpu.h>
 #include <asm/sim.h>
-#include <asm/uaccess.h>
 #include <asm/ucontext.h>
 #include <asm/cpu-features.h>
 #include <asm/war.h>
@@ -78,6 +78,46 @@ struct rt_sigframe {
 /*
  * Helper routines
  */
+static int protected_save_fp_context(struct sigcontext __user *sc)
+{
+	int err;
+	while (1) {
+		lock_fpu_owner();
+		own_fpu_inatomic(1);
+		err = save_fp_context(sc); /* this might fail */
+		unlock_fpu_owner();
+		if (likely(!err))
+			break;
+		/* touch the sigcontext and try again */
+		err = __put_user(0, &sc->sc_fpregs[0]) |
+			__put_user(0, &sc->sc_fpregs[31]) |
+			__put_user(0, &sc->sc_fpc_csr);
+		if (err)
+			break;	/* really bad sigcontext */
+	}
+	return err;
+}
+
+static int protected_restore_fp_context(struct sigcontext __user *sc)
+{
+	int err, tmp;
+	while (1) {
+		lock_fpu_owner();
+		own_fpu_inatomic(0);
+		err = restore_fp_context(sc); /* this might fail */
+		unlock_fpu_owner();
+		if (likely(!err))
+			break;
+		/* touch the sigcontext and try again */
+		err = __get_user(tmp, &sc->sc_fpregs[0]) |
+			__get_user(tmp, &sc->sc_fpregs[31]) |
+			__get_user(tmp, &sc->sc_fpc_csr);
+		if (err)
+			break;	/* really bad sigcontext */
+	}
+	return err;
+}
+
 int setup_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc)
 {
 	int err = 0;
@@ -113,10 +153,7 @@ int setup_sigcontext(struct pt_regs *reg
 		 * Save FPU state to signal context. Signal handler
 		 * will "inherit" current FPU state.
 		 */
-		preempt_disable();
-		own_fpu(1);
-		err |= save_fp_context(sc);
-		preempt_enable();
+		err |= protected_save_fp_context(sc);
 	}
 	return err;
 }
@@ -148,10 +185,7 @@ check_and_restore_fp_context(struct sigc
 	err = sig = fpcsr_pending(&sc->sc_fpc_csr);
 	if (err > 0)
 		err = 0;
-	preempt_disable();
-	own_fpu(0);
-	err |= restore_fp_context(sc);
-	preempt_enable();
+	err |= protected_restore_fp_context(sc);
 	return err ?: sig;
 }
 
diff --git a/arch/mips/kernel/signal32.c b/arch/mips/kernel/signal32.c
index 53a337c..b9a0144 100644
--- a/arch/mips/kernel/signal32.c
+++ b/arch/mips/kernel/signal32.c
@@ -22,6 +22,7 @@
 #include <linux/compat.h>
 #include <linux/suspend.h>
 #include <linux/compiler.h>
+#include <linux/uaccess.h>
 
 #include <asm/abi.h>
 #include <asm/asm.h>
@@ -29,7 +30,6 @@
 #include <linux/bitops.h>
 #include <asm/cacheflush.h>
 #include <asm/sim.h>
-#include <asm/uaccess.h>
 #include <asm/ucontext.h>
 #include <asm/system.h>
 #include <asm/fpu.h>
@@ -176,6 +176,46 @@ struct rt_sigframe32 {
 /*
  * sigcontext handlers
  */
+static int protected_save_fp_context32(struct sigcontext32 __user *sc)
+{
+	int err;
+	while (1) {
+		lock_fpu_owner();
+		own_fpu_inatomic(1);
+		err = save_fp_context32(sc); /* this might fail */
+		unlock_fpu_owner();
+		if (likely(!err))
+			break;
+		/* touch the sigcontext and try again */
+		err = __put_user(0, &sc->sc_fpregs[0]) |
+			__put_user(0, &sc->sc_fpregs[31]) |
+			__put_user(0, &sc->sc_fpc_csr);
+		if (err)
+			break;	/* really bad sigcontext */
+	}
+	return err;
+}
+
+static int protected_restore_fp_context32(struct sigcontext32 __user *sc)
+{
+	int err, tmp;
+	while (1) {
+		lock_fpu_owner();
+		own_fpu_inatomic(0);
+		err = restore_fp_context32(sc); /* this might fail */
+		unlock_fpu_owner();
+		if (likely(!err))
+			break;
+		/* touch the sigcontext and try again */
+		err = __get_user(tmp, &sc->sc_fpregs[0]) |
+			__get_user(tmp, &sc->sc_fpregs[31]) |
+			__get_user(tmp, &sc->sc_fpc_csr);
+		if (err)
+			break;	/* really bad sigcontext */
+	}
+	return err;
+}
+
 static int setup_sigcontext32(struct pt_regs *regs,
 			      struct sigcontext32 __user *sc)
 {
@@ -209,10 +249,7 @@ static int setup_sigcontext32(struct pt_
 		 * Save FPU state to signal context.  Signal handler
 		 * will "inherit" current FPU state.
 		 */
-		preempt_disable();
-		own_fpu(1);
-		err |= save_fp_context32(sc);
-		preempt_enable();
+		err |= protected_save_fp_context32(sc);
 	}
 	return err;
 }
@@ -225,10 +262,7 @@ check_and_restore_fp_context32(struct si
 	err = sig = fpcsr_pending(&sc->sc_fpc_csr);
 	if (err > 0)
 		err = 0;
-	preempt_disable();
-	own_fpu(0);
-	err |= restore_fp_context32(sc);
-	preempt_enable();
+	err |= protected_restore_fp_context32(sc);
 	return err ?: sig;
 }
 
diff --git a/include/asm-mips/fpu.h b/include/asm-mips/fpu.h
index 71436f9..b414a7d 100644
--- a/include/asm-mips/fpu.h
+++ b/include/asm-mips/fpu.h
@@ -100,14 +100,19 @@ static inline void __own_fpu(void)
 	set_thread_flag(TIF_USEDFPU);
 }
 
-static inline void own_fpu(int restore)
+static inline void own_fpu_inatomic(int restore)
 {
-	preempt_disable();
 	if (cpu_has_fpu && !__is_fpu_owner()) {
 		__own_fpu();
 		if (restore)
 			_restore_fp(current);
 	}
+}
+
+static inline void own_fpu(int restore)
+{
+	preempt_disable();
+	own_fpu_inatomic(restore);
 	preempt_enable();
 }
 

Re: [PATCH] Retry {save,restore}_fp_context if failed in atomic context.

[Atsushi Nemoto]

On Mon, 16 Apr 2007 23:19:44 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> The save_fp_context()/restore_fp_context() might sleep on accessing
> user stack and therefore might lose FPU ownership in middle of them.
> 
> If these function failed due to "in_atomic" test in do_page_fault,
> touch the sigcontext area in non-atomic context and retry these
> save/restore operation.
> 
> This is a replacement of a (broken) fix which was titled "Allow CpU
> exception in kernel partially".
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

And this is for 2.6.20-stable.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
This patch depends on:
> Subject: Re: [PATCH] Disallow CpU exception in kernel again.
> Message-Id: <20070414.024450.07456615.anemo@mba.ocn.ne.jp>

 arch/mips/kernel/signal-common.h |   59 +++++++++++++++++++++++++++++++-----
 arch/mips/kernel/signal.c        |    2 +-
 arch/mips/kernel/signal32.c      |   61 ++++++++++++++++++++++++++++++++-----
 arch/mips/kernel/signal_n32.c    |    2 +-
 include/asm-mips/fpu.h           |    9 ++++-
 5 files changed, 112 insertions(+), 21 deletions(-)

diff --git a/arch/mips/kernel/signal-common.h b/arch/mips/kernel/signal-common.h
index b625e9c..6e479f6 100644
--- a/arch/mips/kernel/signal-common.h
+++ b/arch/mips/kernel/signal-common.h
@@ -9,6 +9,55 @@
  */
 
 
+/* Make sure we will not lose FPU ownership */
+#ifdef CONFIG_PREEMPT
+#define lock_fpu_owner()	preempt_disable()
+#define unlock_fpu_owner()	preempt_enable()
+#else
+#define lock_fpu_owner()	pagefault_disable()
+#define unlock_fpu_owner()	pagefault_enable()
+#endif
+
+static inline int protected_save_fp_context(struct sigcontext __user *sc)
+{
+	int err;
+	while (1) {
+		lock_fpu_owner();
+		own_fpu(1);
+		err = save_fp_context(sc); /* this might fail */
+		unlock_fpu_owner();
+		if (likely(!err))
+			break;
+		/* touch the sigcontext and try again */
+		err = __put_user(0, &sc->sc_fpregs[0]) |
+			__put_user(0, &sc->sc_fpregs[31]) |
+			__put_user(0, &sc->sc_fpc_csr);
+		if (err)
+			break;	/* really bad sigcontext */
+	}
+	return err;
+}
+
+static inline int protected_restore_fp_context(struct sigcontext __user *sc)
+{
+	int err, tmp;
+	while (1) {
+		lock_fpu_owner();
+		own_fpu(0);
+		err = restore_fp_context(sc); /* this might fail */
+		unlock_fpu_owner();
+		if (likely(!err))
+			break;
+		/* touch the sigcontext and try again */
+		err = __get_user(tmp, &sc->sc_fpregs[0]) |
+			__get_user(tmp, &sc->sc_fpregs[31]) |
+			__get_user(tmp, &sc->sc_fpc_csr);
+		if (err)
+			break;	/* really bad sigcontext */
+	}
+	return err;
+}
+
 static inline int
 setup_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc)
 {
@@ -51,10 +100,7 @@ setup_sigcontext(struct pt_regs *regs, s
 	 * Save FPU state to signal context.  Signal handler will "inherit"
 	 * current FPU state.
 	 */
-	preempt_disable();
-	own_fpu(1);
-	err |= save_fp_context(sc);
-	preempt_enable();
+	err |= protected_save_fp_context(sc);
 
 out:
 	return err;
@@ -71,10 +117,7 @@ check_and_restore_fp_context(struct sigc
 	err = sig = fpcsr_pending(&sc->sc_fpc_csr);
 	if (err > 0)
 		err = 0;
-	preempt_disable();
-	own_fpu(0);
-	err |= restore_fp_context(sc);
-	preempt_enable();
+	err |= protected_restore_fp_context(sc);
 	return err ?: sig;
 }
 
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index 60960f7..95d9585 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -20,6 +20,7 @@
 #include <linux/ptrace.h>
 #include <linux/unistd.h>
 #include <linux/compiler.h>
+#include <linux/uaccess.h>
 
 #include <asm/abi.h>
 #include <asm/asm.h>
@@ -27,7 +28,6 @@
 #include <asm/cacheflush.h>
 #include <asm/fpu.h>
 #include <asm/sim.h>
-#include <asm/uaccess.h>
 #include <asm/ucontext.h>
 #include <asm/cpu-features.h>
 #include <asm/war.h>
diff --git a/arch/mips/kernel/signal32.c b/arch/mips/kernel/signal32.c
index 1a3f541..fee8547 100644
--- a/arch/mips/kernel/signal32.c
+++ b/arch/mips/kernel/signal32.c
@@ -22,6 +22,7 @@
 #include <linux/compat.h>
 #include <linux/suspend.h>
 #include <linux/compiler.h>
+#include <linux/uaccess.h>
 
 #include <asm/abi.h>
 #include <asm/asm.h>
@@ -29,7 +30,6 @@
 #include <linux/bitops.h>
 #include <asm/cacheflush.h>
 #include <asm/sim.h>
-#include <asm/uaccess.h>
 #include <asm/ucontext.h>
 #include <asm/system.h>
 #include <asm/fpu.h>
@@ -137,6 +137,55 @@ struct ucontext32 {
 /* Check and clear pending FPU exceptions in saved CSR */
 extern int fpcsr_pending(unsigned int __user *fpcsr);
 
+/* Make sure we will not lose FPU ownership */
+#ifdef CONFIG_PREEMPT
+#define lock_fpu_owner()	preempt_disable()
+#define unlock_fpu_owner()	preempt_enable()
+#else
+#define lock_fpu_owner()	pagefault_disable()
+#define unlock_fpu_owner()	pagefault_enable()
+#endif
+
+static int protected_save_fp_context32(struct sigcontext32 __user *sc)
+{
+	int err;
+	while (1) {
+		lock_fpu_owner();
+		own_fpu(1);
+		err = save_fp_context32(sc); /* this might fail */
+		unlock_fpu_owner();
+		if (likely(!err))
+			break;
+		/* touch the sigcontext and try again */
+		err = __put_user(0, &sc->sc_fpregs[0]) |
+			__put_user(0, &sc->sc_fpregs[31]) |
+			__put_user(0, &sc->sc_fpc_csr);
+		if (err)
+			break;	/* really bad sigcontext */
+	}
+	return err;
+}
+
+static int protected_restore_fp_context32(struct sigcontext32 __user *sc)
+{
+	int err, tmp;
+	while (1) {
+		lock_fpu_owner();
+		own_fpu(0);
+		err = restore_fp_context32(sc); /* this might fail */
+		unlock_fpu_owner();
+		if (likely(!err))
+			break;
+		/* touch the sigcontext and try again */
+		err = __get_user(tmp, &sc->sc_fpregs[0]) |
+			__get_user(tmp, &sc->sc_fpregs[31]) |
+			__get_user(tmp, &sc->sc_fpc_csr);
+		if (err)
+			break;	/* really bad sigcontext */
+	}
+	return err;
+}
+
 static int
 check_and_restore_fp_context32(struct sigcontext32 __user *sc)
 {
@@ -145,10 +194,7 @@ check_and_restore_fp_context32(struct si
 	err = sig = fpcsr_pending(&sc->sc_fpc_csr);
 	if (err > 0)
 		err = 0;
-	preempt_disable();
-	own_fpu(0);
-	err |= restore_fp_context32(sc);
-	preempt_enable();
+	err |= protected_restore_fp_context32(sc);
 	return err ?: sig;
 }
 
@@ -614,10 +660,7 @@ static inline int setup_sigcontext32(str
 	 * Save FPU state to signal context.  Signal handler will "inherit"
 	 * current FPU state.
 	 */
-	preempt_disable();
-	own_fpu(1);
-	err |= save_fp_context32(sc);
-	preempt_enable();
+	err |= protected_save_fp_context32(sc);
 
 out:
 	return err;
diff --git a/arch/mips/kernel/signal_n32.c b/arch/mips/kernel/signal_n32.c
index 617edd9..2279963 100644
--- a/arch/mips/kernel/signal_n32.c
+++ b/arch/mips/kernel/signal_n32.c
@@ -28,12 +28,12 @@
 #include <linux/unistd.h>
 #include <linux/compat.h>
 #include <linux/bitops.h>
+#include <linux/uaccess.h>
 
 #include <asm/asm.h>
 #include <asm/cacheflush.h>
 #include <asm/compat-signal.h>
 #include <asm/sim.h>
-#include <asm/uaccess.h>
 #include <asm/ucontext.h>
 #include <asm/system.h>
 #include <asm/fpu.h>
diff --git a/include/asm-mips/fpu.h b/include/asm-mips/fpu.h
index 6b9d1bf..f5cdcaa 100644
--- a/include/asm-mips/fpu.h
+++ b/include/asm-mips/fpu.h
@@ -100,14 +100,19 @@ static inline void __own_fpu(void)
 	set_thread_flag(TIF_USEDFPU);
 }
 
-static inline void own_fpu(int restore)
+static inline void own_fpu_inatomic(int restore)
 {
-	preempt_disable();
 	if (cpu_has_fpu && !__is_fpu_owner()) {
 		__own_fpu();
 		if (restore)
 			_restore_fp(current);
 	}
+}
+
+static inline void own_fpu(int restore)
+{
+	preempt_disable();
+	own_fpu_inatomic(restore);
 	preempt_enable();
 }
 

Re: [PATCH] Retry {save,restore}_fp_context if failed in atomic context.

[Ralf Baechle]

On Mon, Apr 16, 2007 at 11:32:35PM +0900, Atsushi Nemoto wrote:

> > The save_fp_context()/restore_fp_context() might sleep on accessing
> > user stack and therefore might lose FPU ownership in middle of them.
> > 
> > If these function failed due to "in_atomic" test in do_page_fault,
> > touch the sigcontext area in non-atomic context and retry these
> > save/restore operation.
> > 
> > This is a replacement of a (broken) fix which was titled "Allow CpU
> > exception in kernel partially".
> > 
> > Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> 
> And this is for 2.6.20-stable.

Both applied, also to older -stable branches except 2.6.16.  In case of
2.6.16 it would have been more time consuming than justifyable and since
the bug this patch fixes is comparable to what we had before starting the
whole surgery I have no problem to leave 2.6.16 as it is.  Anybody still
using 2.6.16 should upgrade anyway ...

  Ralf

Re: [PATCH] Retry {save,restore}_fp_context if failed in atomic context.

[Atsushi Nemoto]

On Wed, 18 Apr 2007 12:30:46 +0100, Ralf Baechle <ralf@linux-mips.org> wrote:
> > And this is for 2.6.20-stable.
> 
> Both applied, also to older -stable branches except 2.6.16.  In case of
> 2.6.16 it would have been more time consuming than justifyable and since
> the bug this patch fixes is comparable to what we had before starting the
> whole surgery I have no problem to leave 2.6.16 as it is.  Anybody still
> using 2.6.16 should upgrade anyway ...

I agree this patch is not critical for older kernel.  But
2.6.16-stable already applied the broken "Allow CpU exception in
kernel partially" patch.  This should be reverted.  Just revert the
commit a0d2a152ec0917b0c1b0c84a00fd95d17090a5f8 should be enough.

---
Atsushi Nemoto

Re: [PATCH] Retry {save,restore}_fp_context if failed in atomic context.

[Atsushi Nemoto]

On Wed, 18 Apr 2007 23:13:15 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> I agree this patch is not critical for older kernel.  But
> 2.6.16-stable already applied the broken "Allow CpU exception in
> kernel partially" patch.  This should be reverted.  Just revert the
> commit a0d2a152ec0917b0c1b0c84a00fd95d17090a5f8 should be enough.

I confirmed the reverse-patch can be cleanly applied on 2.6.16-stable
head.

---
Atsushi Nemoto