All of lore.kernel.org
 help / color / mirror / Atom feed
From: James Hogan <james.hogan@imgtec.com>
To: Ralf Baechle <ralf@linux-mips.org>
Cc: <linux-mips@linux-mips.org>, Paul Burton <paul.burton@imgtec.com>,
	"James Hogan" <james.hogan@imgtec.com>
Subject: [PATCH 2/2] MIPS: Fix FPU disable with preemption
Date: Mon, 1 Feb 2016 13:50:37 +0000	[thread overview]
Message-ID: <1454334637-3860-3-git-send-email-james.hogan@imgtec.com> (raw)
In-Reply-To: <1454334637-3860-1-git-send-email-james.hogan@imgtec.com>

The FPU should not be left enabled after a task context switch. This
isn't usually a problem as the FPU enable bit is updated before
returning to userland, however it can potentially mask kernel bugs, and
in fact KVM assumes it won't happen and won't clear the FPU enable bit
before returning to the guest, which allows the guest to use stale FPU
context.

Interrupts and exceptions save and restore most bits of the CP0 Status
register which contains the FPU enable bit (CU1). When the kernel needs
to enable or disable the FPU (for example due to attempted FPU use by
userland, or the scheduler being invoked) both the actual Status
register and the saved value in the userland context are updated.

However this doesn't work correctly with full kernel preemption enabled,
since the FPU enable bit can be cleared from within an interrupt when
the scheduler is invoked, and only the userland context is updated, not
the interrupt context.

For example:
1) Enter kernel with FPU already enabled, TIF_USEDFPU=1, Status.CU1=1
   saved.
2) Take a timer interrupt while in kernel mode, Status.CU1=1 saved.
3) Timer interrupt invokes scheduler to preempt the task, which clears
   TIF_USEDFPU, disables the FPU in Status register (Status.CU1=0), and
   the value stored in user context from step (1), but not the interrupt
   context from step (2).
4) When the process is scheduled back in again Status.CU1=0.
5) The interrupt context from step (2) is restored, which sets
   Status.CU1=1. So from user context point of view, preemption has
   re-enabled FPU!
6) If the scheduler is invoked again (via preemption or voluntarily)
   before returning to userland, TIF_USEDFPU=0 so the FPU is not
   disabled before the task context switch.
7) The next task resumes from the context switch with FPU enabled!

The restoring of the Status register on return from interrupt/exception
is already selective about which bits to restore, leaving the interrupt
mask bits alone so enabling/disabling of CPU interrupt lines can
persist. Extend this to also leave both the CU1 bit (FPU enable) and the
FR bit (which specifies the FPU mode and gets changed with CU1). This
prevents a stale Status value being restored in step (5) above and
persisting through subsequent context switches.

Also switch to the use of definitions from asm/mipsregs.h while we're at
it.

Since this change also affects the restoration of Status register on the
path back to userland, it increases the sensitivity of the kernel to the
problem of the FPU being left enabled, allowing it to propagate to
userland, therefore a warning is also added to lose_fpu_inatomic() to
point out any future reoccurances before they do any damage.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Reviewed-by: Paul Burton <paul.burton@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
---
 arch/mips/include/asm/fpu.h        | 4 ++++
 arch/mips/include/asm/stackframe.h | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/fpu.h b/arch/mips/include/asm/fpu.h
index 9cbf383b8834..f06f97bd62df 100644
--- a/arch/mips/include/asm/fpu.h
+++ b/arch/mips/include/asm/fpu.h
@@ -179,6 +179,10 @@ static inline void lose_fpu_inatomic(int save, struct task_struct *tsk)
 		if (save)
 			_save_fp(tsk);
 		__disable_fpu();
+	} else {
+		/* FPU should not have been left enabled with no owner */
+		WARN(read_c0_status() & ST0_CU1,
+		     "Orphaned FPU left enabled");
 	}
 	KSTK_STATUS(tsk) &= ~ST0_CU1;
 	clear_tsk_thread_flag(tsk, TIF_USEDFPU);
diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
index a71da576883c..eebf39549606 100644
--- a/arch/mips/include/asm/stackframe.h
+++ b/arch/mips/include/asm/stackframe.h
@@ -289,7 +289,7 @@
 		.set	reorder
 		.set	noat
 		mfc0	a0, CP0_STATUS
-		li	v1, 0xff00
+		li	v1, ST0_CU1 | ST0_IM
 		ori	a0, STATMASK
 		xori	a0, STATMASK
 		mtc0	a0, CP0_STATUS
@@ -330,7 +330,7 @@
 		ori	a0, STATMASK
 		xori	a0, STATMASK
 		mtc0	a0, CP0_STATUS
-		li	v1, 0xff00
+		li	v1, ST0_CU1 | ST0_FR | ST0_IM
 		and	a0, v1
 		LONG_L	v0, PT_STATUS(sp)
 		nor	v1, $0, v1
-- 
2.4.10

WARNING: multiple messages have this Message-ID (diff)
From: James Hogan <james.hogan@imgtec.com>
To: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org, Paul Burton <paul.burton@imgtec.com>,
	James Hogan <james.hogan@imgtec.com>
Subject: [PATCH 2/2] MIPS: Fix FPU disable with preemption
Date: Mon, 1 Feb 2016 13:50:37 +0000	[thread overview]
Message-ID: <1454334637-3860-3-git-send-email-james.hogan@imgtec.com> (raw)
Message-ID: <20160201135037.GdC17yKxHajSArkVklDQxxKeGkTrPjv2diidOK7nzwQ@z> (raw)
In-Reply-To: <1454334637-3860-1-git-send-email-james.hogan@imgtec.com>

The FPU should not be left enabled after a task context switch. This
isn't usually a problem as the FPU enable bit is updated before
returning to userland, however it can potentially mask kernel bugs, and
in fact KVM assumes it won't happen and won't clear the FPU enable bit
before returning to the guest, which allows the guest to use stale FPU
context.

Interrupts and exceptions save and restore most bits of the CP0 Status
register which contains the FPU enable bit (CU1). When the kernel needs
to enable or disable the FPU (for example due to attempted FPU use by
userland, or the scheduler being invoked) both the actual Status
register and the saved value in the userland context are updated.

However this doesn't work correctly with full kernel preemption enabled,
since the FPU enable bit can be cleared from within an interrupt when
the scheduler is invoked, and only the userland context is updated, not
the interrupt context.

For example:
1) Enter kernel with FPU already enabled, TIF_USEDFPU=1, Status.CU1=1
   saved.
2) Take a timer interrupt while in kernel mode, Status.CU1=1 saved.
3) Timer interrupt invokes scheduler to preempt the task, which clears
   TIF_USEDFPU, disables the FPU in Status register (Status.CU1=0), and
   the value stored in user context from step (1), but not the interrupt
   context from step (2).
4) When the process is scheduled back in again Status.CU1=0.
5) The interrupt context from step (2) is restored, which sets
   Status.CU1=1. So from user context point of view, preemption has
   re-enabled FPU!
6) If the scheduler is invoked again (via preemption or voluntarily)
   before returning to userland, TIF_USEDFPU=0 so the FPU is not
   disabled before the task context switch.
7) The next task resumes from the context switch with FPU enabled!

The restoring of the Status register on return from interrupt/exception
is already selective about which bits to restore, leaving the interrupt
mask bits alone so enabling/disabling of CPU interrupt lines can
persist. Extend this to also leave both the CU1 bit (FPU enable) and the
FR bit (which specifies the FPU mode and gets changed with CU1). This
prevents a stale Status value being restored in step (5) above and
persisting through subsequent context switches.

Also switch to the use of definitions from asm/mipsregs.h while we're at
it.

Since this change also affects the restoration of Status register on the
path back to userland, it increases the sensitivity of the kernel to the
problem of the FPU being left enabled, allowing it to propagate to
userland, therefore a warning is also added to lose_fpu_inatomic() to
point out any future reoccurances before they do any damage.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Reviewed-by: Paul Burton <paul.burton@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
---
 arch/mips/include/asm/fpu.h        | 4 ++++
 arch/mips/include/asm/stackframe.h | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/fpu.h b/arch/mips/include/asm/fpu.h
index 9cbf383b8834..f06f97bd62df 100644
--- a/arch/mips/include/asm/fpu.h
+++ b/arch/mips/include/asm/fpu.h
@@ -179,6 +179,10 @@ static inline void lose_fpu_inatomic(int save, struct task_struct *tsk)
 		if (save)
 			_save_fp(tsk);
 		__disable_fpu();
+	} else {
+		/* FPU should not have been left enabled with no owner */
+		WARN(read_c0_status() & ST0_CU1,
+		     "Orphaned FPU left enabled");
 	}
 	KSTK_STATUS(tsk) &= ~ST0_CU1;
 	clear_tsk_thread_flag(tsk, TIF_USEDFPU);
diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
index a71da576883c..eebf39549606 100644
--- a/arch/mips/include/asm/stackframe.h
+++ b/arch/mips/include/asm/stackframe.h
@@ -289,7 +289,7 @@
 		.set	reorder
 		.set	noat
 		mfc0	a0, CP0_STATUS
-		li	v1, 0xff00
+		li	v1, ST0_CU1 | ST0_IM
 		ori	a0, STATMASK
 		xori	a0, STATMASK
 		mtc0	a0, CP0_STATUS
@@ -330,7 +330,7 @@
 		ori	a0, STATMASK
 		xori	a0, STATMASK
 		mtc0	a0, CP0_STATUS
-		li	v1, 0xff00
+		li	v1, ST0_CU1 | ST0_FR | ST0_IM
 		and	a0, v1
 		LONG_L	v0, PT_STATUS(sp)
 		nor	v1, $0, v1
-- 
2.4.10

  parent reply	other threads:[~2016-02-01 13:51 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-01 13:50 [PATCH 0/2] MIPS: Fix FPU preemption issues James Hogan
2016-02-01 13:50 ` James Hogan
2016-02-01 13:50 ` [PATCH 1/2] MIPS: Properly disable FPU in start_thread() James Hogan
2016-02-01 13:50   ` James Hogan
2016-02-01 13:50 ` James Hogan [this message]
2016-02-01 13:50   ` [PATCH 2/2] MIPS: Fix FPU disable with preemption James Hogan
2016-02-01 22:54 ` [PATCH 0/2] MIPS: Fix FPU preemption issues Ralf Baechle

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1454334637-3860-3-git-send-email-james.hogan@imgtec.com \
    --to=james.hogan@imgtec.com \
    --cc=linux-mips@linux-mips.org \
    --cc=paul.burton@imgtec.com \
    --cc=ralf@linux-mips.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.