linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer
@ 2015-01-30 17:43 Dave Hansen
  2015-01-30 17:43 ` [RFC][PATCH 2/2] x86, mpx: use new tsk_get_xsave_addr() Dave Hansen
  2015-01-30 18:28 ` [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer Oleg Nesterov
  0 siblings, 2 replies; 6+ messages in thread
From: Dave Hansen @ 2015-01-30 17:43 UTC (permalink / raw)
  To: oleg
  Cc: Dave Hansen, dave.hansen, riel, sbsiddha, luto, tglx, mingo, hpa,
	fenghua.yu, x86, linux-kernel


The MPX code appears to be saving off the FPU in an unsafe
way.   It does not disable preemption or ensure that the
FPU state has been allocated.

This patch introduces a new helper which will do both of
those things internally to a helper.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc:Andy Lutomirski <luto@amacapital.net>
Cc:Thomas Gleixner <tglx@linutronix.de>
Cc:Ingo Molnar <mingo@redhat.com>
Cc:"H. Peter Anvin" <hpa@zytor.com>
Cc:Fenghua Yu <fenghua.yu@intel.com>
Cc:the arch/x86 maintainers <x86@kernel.org>
Cc:linux-kernel <linux-kernel@vger.kernel.org>
---

 b/arch/x86/include/asm/xsave.h |    1 +
 b/arch/x86/kernel/xsave.c      |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff -puN arch/x86/include/asm/xsave.h~tsk_get_xsave_addr arch/x86/include/asm/xsave.h
--- a/arch/x86/include/asm/xsave.h~tsk_get_xsave_addr	2015-01-30 09:42:15.457555582 -0800
+++ b/arch/x86/include/asm/xsave.h	2015-01-30 09:42:15.462555808 -0800
@@ -258,6 +258,7 @@ static inline int xrestore_user(struct x
 }
 
 void *get_xsave_addr(struct xsave_struct *xsave, int xstate);
+void *tsk_get_xsave_field(struct task_struct *tsk, int xstate_field);
 void setup_xstate_comp(void);
 
 #endif
diff -puN arch/x86/kernel/xsave.c~tsk_get_xsave_addr arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~tsk_get_xsave_addr	2015-01-30 09:42:15.459555673 -0800
+++ b/arch/x86/kernel/xsave.c	2015-01-30 09:42:15.463555853 -0800
@@ -739,3 +739,35 @@ void *get_xsave_addr(struct xsave_struct
 	return (void *)xsave + xstate_comp_offsets[feature];
 }
 EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from an xsave struct.  It first ensures that the task was actually
+ * using the FPU and retrieves the data in to a buffer.  It then calculates
+ * the offset of the requested field in the buffer.
+ *
+ * This function is safe to call whether the FPU is in use or not.
+ *
+ * Inputs:
+ *	tsk: the task from which we are fetching xsave state
+ *	xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
+ *	etc.)
+ * Output:
+ *	address of the state in the xsave area.
+ */
+void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
+{
+	union thread_xstate *xstate;
+
+	unlazy_fpu(tsk);
+	xstate = tsk->thread.fpu.state;
+	/*
+	 * This might be unallocated if the FPU
+	 * was never in use.
+	 */
+	if (!xstate)
+		return NULL;
+
+	return get_xsave_addr(&xstate->xsave, xsave_field);
+}
+EXPORT_SYMBOL_GPL(tsk_get_xsave_field);
_

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RFC][PATCH 2/2] x86, mpx: use new tsk_get_xsave_addr()
  2015-01-30 17:43 [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer Dave Hansen
@ 2015-01-30 17:43 ` Dave Hansen
  2015-01-30 18:34   ` Oleg Nesterov
  2015-01-30 18:28 ` [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer Oleg Nesterov
  1 sibling, 1 reply; 6+ messages in thread
From: Dave Hansen @ 2015-01-30 17:43 UTC (permalink / raw)
  To: oleg
  Cc: Dave Hansen, dave.hansen, riel, sbsiddha, luto, tglx, mingo, hpa,
	fenghua.yu, x86, linux-kernel


The MPX registers (bndcsr/bndcfgu/bndstatus) are not directly
accessible via normal instructions.  They essentially act as
if they were floating point registers and are saved/restored
along with those registers.

There are two main paths in the MPX code where we care about
the contents of these registers:
	1. #BR (bounds) faults
	2. the prctl() code where we are setting MPX up

Both of those paths _might_ be called without the FPU having
been used.  That means that 'tsk->thread.fpu.state' might
never be allocated.

Also, fpu_save_init() is not preempt-safe.  It was a bug to
call it without disabling preemption.  The new
tsk_get_xsave_addr() calls unlazy_fpu() instead and properly
disables preemption.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: the arch/x86 maintainers <x86@kernel.org>
Cc: linux-kernel <linux-kernel@vger.kernel.org>
---

 b/arch/x86/include/asm/mpx.h |    8 ++++----
 b/arch/x86/kernel/traps.c    |   10 ++++------
 b/arch/x86/mm/mpx.c          |   21 ++++++++++-----------
 3 files changed, 18 insertions(+), 21 deletions(-)

diff -puN arch/x86/include/asm/mpx.h~use-new-tsk_get_xsave_addr arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~use-new-tsk_get_xsave_addr	2015-01-30 09:42:15.852573398 -0800
+++ b/arch/x86/include/asm/mpx.h	2015-01-30 09:42:15.859573714 -0800
@@ -60,8 +60,8 @@
 
 #ifdef CONFIG_X86_INTEL_MPX
 siginfo_t *mpx_generate_siginfo(struct pt_regs *regs,
-				struct xsave_struct *xsave_buf);
-int mpx_handle_bd_fault(struct xsave_struct *xsave_buf);
+				struct task_struct *tsk);
+int mpx_handle_bd_fault(struct task_struct *tsk);
 static inline int kernel_managing_mpx_tables(struct mm_struct *mm)
 {
 	return (mm->bd_addr != MPX_INVALID_BOUNDS_DIR);
@@ -78,11 +78,11 @@ void mpx_notify_unmap(struct mm_struct *
 		      unsigned long start, unsigned long end);
 #else
 static inline siginfo_t *mpx_generate_siginfo(struct pt_regs *regs,
-					      struct xsave_struct *xsave_buf)
+					      struct task_struct *tsk)
 {
 	return NULL;
 }
-static inline int mpx_handle_bd_fault(struct xsave_struct *xsave_buf)
+static inline int mpx_handle_bd_fault(struct task_struct *tsk)
 {
 	return -EINVAL;
 }
diff -puN arch/x86/kernel/traps.c~use-new-tsk_get_xsave_addr arch/x86/kernel/traps.c
--- a/arch/x86/kernel/traps.c~use-new-tsk_get_xsave_addr	2015-01-30 09:42:15.854573489 -0800
+++ b/arch/x86/kernel/traps.c	2015-01-30 09:42:15.859573714 -0800
@@ -61,6 +61,7 @@
 #include <asm/mach_traps.h>
 #include <asm/alternative.h>
 #include <asm/mpx.h>
+#include <asm/xsave.h>
 
 #ifdef CONFIG_X86_64
 #include <asm/x86_init.h>
@@ -289,7 +290,6 @@ dotraplinkage void do_double_fault(struc
 dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 {
 	struct task_struct *tsk = current;
-	struct xsave_struct *xsave_buf;
 	enum ctx_state prev_state;
 	struct bndcsr *bndcsr;
 	siginfo_t *info;
@@ -313,9 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
 	 * It is not directly accessible, though, so we need to
 	 * do an xsave and then pull it out of the xsave buffer.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
-	xsave_buf = &(tsk->thread.fpu.state->xsave);
-	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
+	bndcsr = tsk_get_xsave_field(tsk, XSTATE_BNDCSR);
 	if (!bndcsr)
 		goto exit_trap;
 
@@ -326,11 +324,11 @@ dotraplinkage void do_bounds(struct pt_r
 	 */
 	switch (bndcsr->bndstatus & MPX_BNDSTA_ERROR_CODE) {
 	case 2:	/* Bound directory has invalid entry. */
-		if (mpx_handle_bd_fault(xsave_buf))
+		if (mpx_handle_bd_fault(tsk))
 			goto exit_trap;
 		break; /* Success, it was handled */
 	case 1: /* Bound violation. */
-		info = mpx_generate_siginfo(regs, xsave_buf);
+		info = mpx_generate_siginfo(regs, tsk);
 		if (IS_ERR(info)) {
 			/*
 			 * We failed to decode the MPX instruction.  Act as if
diff -puN arch/x86/mm/mpx.c~use-new-tsk_get_xsave_addr arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~use-new-tsk_get_xsave_addr	2015-01-30 09:42:15.855573534 -0800
+++ b/arch/x86/mm/mpx.c	2015-01-30 09:42:15.860573759 -0800
@@ -273,7 +273,7 @@ bad_opcode:
  * The caller is expected to kfree() the returned siginfo_t.
  */
 siginfo_t *mpx_generate_siginfo(struct pt_regs *regs,
-				struct xsave_struct *xsave_buf)
+				struct task_struct *tsk)
 {
 	struct bndreg *bndregs, *bndreg;
 	siginfo_t *info = NULL;
@@ -296,7 +296,7 @@ siginfo_t *mpx_generate_siginfo(struct p
 		goto err_out;
 	}
 	/* get the bndregs _area_ of the xsave structure */
-	bndregs = get_xsave_addr(xsave_buf, XSTATE_BNDREGS);
+	bndregs = tsk_get_xsave_field(tsk, XSTATE_BNDREGS);
 	if (!bndregs) {
 		err = -EINVAL;
 		goto err_out;
@@ -358,8 +358,7 @@ static __user void *task_get_bounds_dir(
 	 * The bounds directory pointer is stored in a register
 	 * only accessible if we first do an xsave.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
-	bndcsr = get_xsave_addr(&tsk->thread.fpu.state->xsave, XSTATE_BNDCSR);
+	bndcsr = tsk_get_xsave_field(tsk, XSTATE_BNDCSR);
 	if (!bndcsr)
 		return MPX_INVALID_BOUNDS_DIR;
 
@@ -390,9 +389,9 @@ int mpx_enable_management(struct task_st
 	 * directory into XSAVE/XRSTOR Save Area and enable MPX through
 	 * XRSTOR instruction.
 	 *
-	 * fpu_xsave() is expected to be very expensive. Storing the bounds
-	 * directory here means that we do not have to do xsave in the unmap
-	 * path; we can just use mm->bd_addr instead.
+	 * xsaves are expected to be very expensive.  Storing the bounds
+	 * directory here means that we do not have to do xsave in the
+	 * unmap path; we can just use mm->bd_addr instead.
 	 */
 	bd_base = task_get_bounds_dir(tsk);
 	down_write(&mm->mmap_sem);
@@ -498,12 +497,12 @@ out_unmap:
  * bound table is 16KB. With 64-bit mode, the size of BD is 2GB,
  * and the size of each bound table is 4MB.
  */
-static int do_mpx_bt_fault(struct xsave_struct *xsave_buf)
+static int do_mpx_bt_fault(struct task_struct *tsk)
 {
 	unsigned long bd_entry, bd_base;
 	struct bndcsr *bndcsr;
 
-	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
+	bndcsr = tsk_get_xsave_field(tsk, XSTATE_BNDCSR);
 	if (!bndcsr)
 		return -EINVAL;
 	/*
@@ -526,7 +525,7 @@ static int do_mpx_bt_fault(struct xsave_
 	return allocate_bt((long __user *)bd_entry);
 }
 
-int mpx_handle_bd_fault(struct xsave_struct *xsave_buf)
+int mpx_handle_bd_fault(struct task_struct *tsk)
 {
 	/*
 	 * Userspace never asked us to manage the bounds tables,
@@ -535,7 +534,7 @@ int mpx_handle_bd_fault(struct xsave_str
 	if (!kernel_managing_mpx_tables(current->mm))
 		return -EINVAL;
 
-	if (do_mpx_bt_fault(xsave_buf)) {
+	if (do_mpx_bt_fault(tsk)) {
 		force_sig(SIGSEGV, current);
 		/*
 		 * The force_sig() is essentially "handling" this
_

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer
  2015-01-30 17:43 [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer Dave Hansen
  2015-01-30 17:43 ` [RFC][PATCH 2/2] x86, mpx: use new tsk_get_xsave_addr() Dave Hansen
@ 2015-01-30 18:28 ` Oleg Nesterov
  2015-01-30 18:38   ` Oleg Nesterov
  1 sibling, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2015-01-30 18:28 UTC (permalink / raw)
  To: Dave Hansen
  Cc: dave.hansen, riel, sbsiddha, luto, tglx, mingo, hpa, fenghua.yu,
	x86, linux-kernel

On 01/30, Dave Hansen wrote:
>
> +void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
> +{
> +	union thread_xstate *xstate;
> +
> +	unlazy_fpu(tsk);

ack, but to remind this depends on 2/3 I sent.

> +	xstate = tsk->thread.fpu.state;
> +	/*
> +	 * This might be unallocated if the FPU
> +	 * was never in use.
> +	 */
> +	if (!xstate)
> +		return NULL;

This is cosmetic, unlazy_fpu() is safe if !xstate, __thread_has_fpu()
is not possible in this case.

But perhaps

	if (!used_math())
		return NULL;

will look better.

Oleg.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH 2/2] x86, mpx: use new tsk_get_xsave_addr()
  2015-01-30 17:43 ` [RFC][PATCH 2/2] x86, mpx: use new tsk_get_xsave_addr() Dave Hansen
@ 2015-01-30 18:34   ` Oleg Nesterov
  2015-01-30 21:37     ` Dave Hansen
  0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2015-01-30 18:34 UTC (permalink / raw)
  To: Dave Hansen
  Cc: dave.hansen, riel, sbsiddha, luto, tglx, mingo, hpa, fenghua.yu,
	x86, linux-kernel

On 01/30, Dave Hansen wrote:
>
> The new
> tsk_get_xsave_addr() calls unlazy_fpu() instead and properly
> disables preemption.

And it seems it becomes the only caller of get_xsave_addr(), so you
can unexport and make it static.

Oleg.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer
  2015-01-30 18:28 ` [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer Oleg Nesterov
@ 2015-01-30 18:38   ` Oleg Nesterov
  0 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2015-01-30 18:38 UTC (permalink / raw)
  To: Dave Hansen
  Cc: dave.hansen, riel, sbsiddha, luto, tglx, mingo, hpa, fenghua.yu,
	x86, linux-kernel

On 01/30, Oleg Nesterov wrote:
>
> On 01/30, Dave Hansen wrote:
> >
> > +void *tsk_get_xsave_field(struct task_struct *tsk, int xsave_field)
> > +{
> > +	union thread_xstate *xstate;
> > +
> > +	unlazy_fpu(tsk);
> 
> ack, but to remind this depends on 2/3 I sent.
> 
> > +	xstate = tsk->thread.fpu.state;
> > +	/*
> > +	 * This might be unallocated if the FPU
> > +	 * was never in use.
> > +	 */
> > +	if (!xstate)
> > +		return NULL;
> 
> This is cosmetic, unlazy_fpu() is safe if !xstate, __thread_has_fpu()
> is not possible in this case.
> 
> But perhaps
> 
> 	if (!used_math())
> 		return NULL;
> 
> will look better.

at the start of this helper, I meant

Oleg.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC][PATCH 2/2] x86, mpx: use new tsk_get_xsave_addr()
  2015-01-30 18:34   ` Oleg Nesterov
@ 2015-01-30 21:37     ` Dave Hansen
  0 siblings, 0 replies; 6+ messages in thread
From: Dave Hansen @ 2015-01-30 21:37 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: dave.hansen, riel, sbsiddha, luto, tglx, mingo, hpa, fenghua.yu,
	x86, linux-kernel

On 01/30/2015 10:34 AM, Oleg Nesterov wrote:
> On 01/30, Dave Hansen wrote:
>> > The new
>> > tsk_get_xsave_addr() calls unlazy_fpu() instead and properly
>> > disables preemption.
> And it seems it becomes the only caller of get_xsave_addr(), so you
> can unexport and make it static.

They're pretty new functions, and I _think_ Fenghua intended for them to
get used my more users down the line.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-01-30 21:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-30 17:43 [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer Dave Hansen
2015-01-30 17:43 ` [RFC][PATCH 2/2] x86, mpx: use new tsk_get_xsave_addr() Dave Hansen
2015-01-30 18:34   ` Oleg Nesterov
2015-01-30 21:37     ` Dave Hansen
2015-01-30 18:28 ` [RFC][PATCH 1/2] x86, fpu: wrap get_xsave_addr() to make it safer Oleg Nesterov
2015-01-30 18:38   ` Oleg Nesterov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).