linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/19] x86, mpx updates for 4.2 (take 8)
@ 2015-05-29 22:34 Dave Hansen
  2015-05-29 22:34 ` [PATCH 03/19] x86, mpx: Use new get_xsave_field_ptr() Dave Hansen
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Dave Hansen @ 2015-05-29 22:34 UTC (permalink / raw)
  To: linux-kernel; +Cc: x86, tglx, Dave Hansen


Changes from take 7 / v22:
 * Add Thomas's reviewed-by
 * merge with tip/x86/fpu changes
 * Fix tiny spelling nit

Changes from take 6 / v21:
 * Address a bunch of Thomas's review comments.

Changes from take 5 / v20:

 * Fix get_xsave_addr() to consult xstate_bv in anticipation
   of fixes to xsave code.
 * Bug fix for when an VMA being unmapped has neighbors which
   are bounds tables.
 * Rewrite unmapping code.  I didn't do this lightly. It was
   not originally my own code, and I resisted changing it
   because it worked.  But, I started bug chasing and decided
   it was unmaintainable.  The rewrite ended up removing
   about 20% of the unmapping code and made it much simpler.

Changes from take 4 / v19:

 * Do not pass a task_struct around when we are
   really just going to operate on current

Changes from take 3 / v18 (all minor):

 * use DECLARE_EVENT_CLASS()/DEFINE_EVENT() for
   the ranged tracepoints to save 10 lines of code.

Changes from take 2 / v17 (all minor):

 * fix a couple of whitespace borkages caught by checkpatch,
   and a spelling error or two.
 * replace printk with pr_info() for boot disable
 * change trace print format for address intervals
 * fix up variable name in tsk_get_xsave_addr() comment
 * remove tsk_get_xsave_field() GPL export
 * fix up Qiaowei's From:

--

Hi x86 maintainers,

There are a few basic things going on here:
1. Make FPU/xsave code preempt safe and work properly
2. Add trace points to make kernel and app debugging easier
3. Add a boot-time disable for mpx
4. Rewrite the unmapping code.
5. Support 32-bit binaries to run on 64-bit kernels

This set is also available against tip/x86/fpu (8c05f05edb) in git:

  git://git.kernel.org/pub/scm/linux/kernel/git/daveh/x86-mpx.git mpx-v23

Dave Hansen (19):
  x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions
  x86, fpu: Wrap get_xsave_addr() to make it safer
  x86, mpx: Use new get_xsave_field_ptr()
  x86, mpx: Cleanup: Do not pass task around when unnecessary
  x86, mpx: remove redundant MPX_BNDCFG_ADDR_MASK
  x86, mpx: Restrict mmap size check to bounds tables
  x86, mpx: boot-time disable
  x86, mpx: trace #BR exceptions
  x86, mpx: trace entry to bounds exception paths
  x86, mpx: Trace the attempts to find bounds tables
  x86, mpx: trace allocation of new bounds tables
  x86: make is_64bit_mm() widely available
  x86, mpx: Add temporary variable to reduce masking
  x86, mpx: new directory entry to addr helper
  x86, mpx: do 32-bit-only cmpxchg for 32-bit apps
  x86, mpx: support 32-bit binaries on 64-bit kernel
  x86, mpx: rewrite unmap code
  x86, mpx: do not count MPX VMAs as neighbors when unmapping
  x86, mpx: allow mixed binaries again

 Documentation/kernel-parameters.txt |   4 +
 arch/x86/include/asm/fpu/xstate.h   |   1 +
 arch/x86/include/asm/mmu_context.h  |  13 +
 arch/x86/include/asm/mpx.h          |  74 +++---
 arch/x86/include/asm/processor.h    |  12 +-
 arch/x86/include/asm/trace/mpx.h    | 131 ++++++++++
 arch/x86/kernel/cpu/common.c        |  16 ++
 arch/x86/kernel/fpu/xstate.c        |  78 +++++-
 arch/x86/kernel/traps.c             |  18 +-
 arch/x86/kernel/uprobes.c           |  10 +-
 arch/x86/mm/mpx.c                   | 508 ++++++++++++++++++++++--------------
 kernel/sys.c                        |   8 +-
 12 files changed, 606 insertions(+), 267 deletions(-)
 create mode 100644 arch/x86/include/asm/trace/mpx.h


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

* [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions
  2015-05-29 22:34 [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Dave Hansen
  2015-05-29 22:34 ` [PATCH 03/19] x86, mpx: Use new get_xsave_field_ptr() Dave Hansen
@ 2015-05-29 22:34 ` Dave Hansen
  2015-05-29 22:34 ` [PATCH 02/19] x86, fpu: Wrap get_xsave_addr() to make it safer Dave Hansen
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Dave Hansen @ 2015-05-29 22:34 UTC (permalink / raw)
  To: linux-kernel; +Cc: x86, tglx, Dave Hansen, dave.hansen


From: Dave Hansen <dave.hansen@linux.intel.com>

get_xsave_addr() assumes that if an xsave bit is present in the
hardware (pcntxt_mask) that it is present in a given xsave
buffer.  Due to an bug in the xsave code on all of the systems
that have MPX (and thus all the users of this code), that has
been a true assumption.

But, the bug is getting fixed, so our assumption is not going
to hold any more.

It's quite possible (and normal) for an enabled state to be
present on 'pcntxt_mask', but *not* in 'xstate_bv'.  We need
to consult 'xstate_bv'.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---

 b/arch/x86/kernel/fpu/xstate.c |   45 +++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff -puN arch/x86/kernel/fpu/xstate.c~consullt-xstate_bv arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~consullt-xstate_bv	2015-05-27 09:32:14.540445571 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-05-27 09:32:14.543445706 -0700
@@ -382,19 +382,48 @@ void fpu__resume_cpu(void)
  * This is the API that is called to get xstate address in either
  * standard format or compacted format of xsave area.
  *
+ * Note that if there is no data for the field in the xsave buffer
+ * this will return NULL.
+ *
  * Inputs:
- *	xsave: base address of the xsave area;
- *	xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
- *	etc.)
+ *	xstate: the thread's storage area for all FPU data
+ *	xstate_feature: state which is defined in xsave.h (e.g.
+ *	XSTATE_FP, XSTATE_SSE, etc...)
  * Output:
- *	address of the state in the xsave area.
+ *	address of the state in the xsave area, or NULL if the
+ *	field is not present in the xsave buffer.
  */
-void *get_xsave_addr(struct xregs_state *xsave, int xstate)
+void *get_xsave_addr(struct xregs_state *xsave, int xstate_feature)
 {
-	int feature = fls64(xstate) - 1;
-	if (!test_bit(feature, (unsigned long *)&xfeatures_mask))
+	int feature_nr = fls64(xstate_feature) - 1;
+	/*
+	 * Do we even *have* xsave state?
+	 */
+	if (!boot_cpu_has(X86_FEATURE_XSAVE))
+		return NULL;
+
+	xsave = &current->thread.fpu.state.xsave;
+	/*
+	 * We should not ever be requesting features that we
+	 * have not enabled.  Remember that pcntxt_mask is
+	 * what we write to the XCR0 register.
+	 */
+	WARN_ONCE(!(xfeatures_mask & xstate_feature),
+		  "get of unsupported state");
+	/*
+	 * This assumes the last 'xsave*' instruction to
+	 * have requested that 'xstate_feature' be saved.
+	 * If it did not, we might be seeing and old value
+	 * of the field in the buffer.
+	 *
+	 * This can happen because the last 'xsave' did not
+	 * request that this feature be saved (unlikely)
+	 * or because the "init optimization" caused it
+	 * to not be saved.
+	 */
+	if (!(xsave->header.xfeatures & xstate_feature))
 		return NULL;
 
-	return (void *)xsave + xstate_comp_offsets[feature];
+	return (void *)xsave + xstate_comp_offsets[feature_nr];
 }
 EXPORT_SYMBOL_GPL(get_xsave_addr);
_

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

* [PATCH 02/19] x86, fpu: Wrap get_xsave_addr() to make it safer
  2015-05-29 22:34 [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Dave Hansen
  2015-05-29 22:34 ` [PATCH 03/19] x86, mpx: Use new get_xsave_field_ptr() Dave Hansen
  2015-05-29 22:34 ` [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions Dave Hansen
@ 2015-05-29 22:34 ` Dave Hansen
  2015-05-29 22:34 ` [PATCH 04/19] x86, mpx: Cleanup: Do not pass task around when unnecessary Dave Hansen
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Dave Hansen @ 2015-05-29 22:34 UTC (permalink / raw)
  To: linux-kernel
  Cc: x86, tglx, Dave Hansen, dave.hansen, oleg, bp, riel, sbsiddha,
	luto, mingo, hpa, fenghua.yu


From: Dave Hansen <dave.hansen@linux.intel.com>

The MPX code appears to be saving off the FPU in a potntially
unsafe way (if eagerfpu=off).  It does not disable preemption or
ensure that the FPU state has been allocated.  All of the
preemption safety comes from the unfortunatley-named
'unlazy_fpu()'.

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

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: bp@alien8.de
Cc: Rik van Riel <riel@redhat.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
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>

---

Changes from v21:
 * add comments about preemption
 * rename helper to get_xsave_field_ptr()

Changes from "v19":
 * remove 'tsk' argument to get_xsave_addr() since the code
   can only realistically work on 'current', and fix up the
   comment a bit to match.

Changes from "v17":
 * fix s/xstate/xsave_field/ in the function comment
 * remove EXPORT_SYMBOL_GPL()

---

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

diff -puN arch/x86/include/asm/fpu/xstate.h~tsk_get_xsave_addr arch/x86/include/asm/fpu/xstate.h
--- a/arch/x86/include/asm/fpu/xstate.h~tsk_get_xsave_addr	2015-05-28 08:49:45.191271502 -0700
+++ b/arch/x86/include/asm/fpu/xstate.h	2015-05-29 13:43:34.291184369 -0700
@@ -41,5 +41,6 @@ extern u64 xstate_fx_sw_bytes[USER_XSTAT
 extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);
 
 void *get_xsave_addr(struct xregs_state *xsave, int xstate);
+const void *get_xsave_field_ptr(int xstate_field);
 
 #endif
diff -puN arch/x86/kernel/fpu/xstate.c~tsk_get_xsave_addr arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~tsk_get_xsave_addr	2015-05-28 08:49:45.192271546 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-05-29 12:32:47.869662576 -0700
@@ -427,3 +427,35 @@ void *get_xsave_addr(struct xregs_state
 	return (void *)xsave + xstate_comp_offsets[feature_nr];
 }
 EXPORT_SYMBOL_GPL(get_xsave_addr);
+
+/*
+ * This wraps up the common operations that need to occur when retrieving
+ * data from xsave state.  It first ensures that the current task was
+ * 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.
+ *
+ * Note that this only works on the current task.
+ *
+ * Inputs:
+ *	@xsave_state: state which is defined in xsave.h (e.g. XSTATE_FP,
+ *	XSTATE_SSE, etc...)
+ * Output:
+ *	address of the state in the xsave area or NULL if the state
+ *	is not present or is in its 'init state'.
+ */
+const void *get_xsave_field_ptr(int xsave_state)
+{
+	struct fpu *fpu = &current->thread.fpu;
+
+	if (!fpu->fpstate_active)
+		return NULL;
+	/*
+	 * fpu__save() takes the CPU's xstate registers
+	 * and saves them off to the 'fpu memory buffer.
+	 */
+	fpu__save(fpu);
+
+	return get_xsave_addr(&fpu->xstate->xsave, xsave_state);
+}
_

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

* [PATCH 03/19] x86, mpx: Use new get_xsave_field_ptr()
  2015-05-29 22:34 [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Dave Hansen
@ 2015-05-29 22:34 ` Dave Hansen
  2015-05-29 22:34 ` [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions Dave Hansen
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Dave Hansen @ 2015-05-29 22:34 UTC (permalink / raw)
  To: linux-kernel
  Cc: x86, tglx, Dave Hansen, dave.hansen, oleg, bp, riel, sbsiddha,
	luto, mingo, hpa, fenghua.yu


From: Dave Hansen <dave.hansen@linux.intel.com>

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
get_xsave_addr() calls unlazy_fpu() instead and properly
disables preemption.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: bp@alien8.de
Cc: Rik van Riel <riel@redhat.com>
Cc: Suresh Siddha <sbsiddha@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
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>

---

Changes from v21:
 * rename get_xsave_field() to get_xsave_field_ptr()
---

 b/arch/x86/include/asm/mpx.h |    8 ++++----
 b/arch/x86/kernel/traps.c    |   15 +++++++--------
 b/arch/x86/mm/mpx.c          |   24 ++++++++++++------------
 3 files changed, 23 insertions(+), 24 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-05-27 09:32:15.346481924 -0700
+++ b/arch/x86/include/asm/mpx.h	2015-05-27 09:32:15.353482240 -0700
@@ -60,8 +60,8 @@
 
 #ifdef CONFIG_X86_INTEL_MPX
 siginfo_t *mpx_generate_siginfo(struct pt_regs *regs,
-				struct xregs_state *xsave_buf);
-int mpx_handle_bd_fault(struct xregs_state *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 xregs_state *xsave_buf)
+					      struct task_struct *tsk)
 {
 	return NULL;
 }
-static inline int mpx_handle_bd_fault(struct xregs_state *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-05-27 09:32:15.348482015 -0700
+++ b/arch/x86/kernel/traps.c	2015-05-27 09:32:15.353482240 -0700
@@ -59,6 +59,7 @@
 #include <asm/fixmap.h>
 #include <asm/mach_traps.h>
 #include <asm/alternative.h>
+#include <asm/fpu/xstate.h>
 #include <asm/mpx.h>
 
 #ifdef CONFIG_X86_64
@@ -371,7 +372,6 @@ dotraplinkage void do_double_fault(struc
 dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 {
 	struct task_struct *tsk = current;
-	struct xregs_state *xsave_buf;
 	enum ctx_state prev_state;
 	struct bndcsr *bndcsr;
 	siginfo_t *info;
@@ -392,12 +392,11 @@ dotraplinkage void do_bounds(struct pt_r
 
 	/*
 	 * We need to look at BNDSTATUS to resolve this exception.
-	 * It is not directly accessible, though, so we need to
-	 * do an xsave and then pull it out of the xsave buffer.
+	 * A NULL here might mean that it is in its 'init state',
+	 * which is all zeros which indicates MPX was not
+	 * responsible for the exception.
 	 */
-	copy_fpregs_to_fpstate(&tsk->thread.fpu);
-	xsave_buf = &(tsk->thread.fpu.state.xsave);
-	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
+	bndcsr = get_xsave_field_ptr(XSTATE_BNDCSR);
 	if (!bndcsr)
 		goto exit_trap;
 
@@ -408,11 +407,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-05-27 09:32:15.349482059 -0700
+++ b/arch/x86/mm/mpx.c	2015-05-27 09:32:15.354482285 -0700
@@ -272,7 +272,7 @@ bad_opcode:
  * The caller is expected to kfree() the returned siginfo_t.
  */
 siginfo_t *mpx_generate_siginfo(struct pt_regs *regs,
-				struct xregs_state *xsave_buf)
+				struct task_struct *tsk)
 {
 	struct bndreg *bndregs, *bndreg;
 	siginfo_t *info = NULL;
@@ -294,8 +294,8 @@ siginfo_t *mpx_generate_siginfo(struct p
 		err = -EINVAL;
 		goto err_out;
 	}
-	/* get the bndregs _area_ of the xsave structure */
-	bndregs = get_xsave_addr(xsave_buf, XSTATE_BNDREGS);
+	/* get bndregs field from current task's xsave area */
+	bndregs = get_xsave_field_ptr(XSTATE_BNDREGS);
 	if (!bndregs) {
 		err = -EINVAL;
 		goto err_out;
@@ -357,8 +357,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.
 	 */
-	copy_fpregs_to_fpstate(&tsk->thread.fpu);
-	bndcsr = get_xsave_addr(&tsk->thread.fpu.state.xsave, XSTATE_BNDCSR);
+	bndcsr = get_xsave_field_ptr(XSTATE_BNDCSR);
 	if (!bndcsr)
 		return MPX_INVALID_BOUNDS_DIR;
 
@@ -389,9 +388,10 @@ int mpx_enable_management(struct task_st
 	 * directory into XSAVE/XRSTOR Save Area and enable MPX through
 	 * XRSTOR instruction.
 	 *
-	 * copy_xregs_to_kernel() 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.
+	 * The copy_xregs_to_kernel() beneath get_xsave_field_ptr() is
+	 * expected to be relatively 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);
@@ -497,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 xregs_state *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 = get_xsave_field_ptr(XSTATE_BNDCSR);
 	if (!bndcsr)
 		return -EINVAL;
 	/*
@@ -525,7 +525,7 @@ static int do_mpx_bt_fault(struct xregs_
 	return allocate_bt((long __user *)bd_entry);
 }
 
-int mpx_handle_bd_fault(struct xregs_state *xsave_buf)
+int mpx_handle_bd_fault(struct task_struct *tsk)
 {
 	/*
 	 * Userspace never asked us to manage the bounds tables,
@@ -534,7 +534,7 @@ int mpx_handle_bd_fault(struct xregs_sta
 	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] 13+ messages in thread

* [PATCH 05/19] x86, mpx: remove redundant MPX_BNDCFG_ADDR_MASK
  2015-05-29 22:34 [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Dave Hansen
                   ` (3 preceding siblings ...)
  2015-05-29 22:34 ` [PATCH 04/19] x86, mpx: Cleanup: Do not pass task around when unnecessary Dave Hansen
@ 2015-05-29 22:34 ` Dave Hansen
  2015-06-01 11:14 ` [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Ingo Molnar
  5 siblings, 0 replies; 13+ messages in thread
From: Dave Hansen @ 2015-05-29 22:34 UTC (permalink / raw)
  To: linux-kernel; +Cc: x86, tglx, Dave Hansen, qiaowei.ren, dave.hansen


From: Qiaowei Ren <qiaowei.ren@intel.com>

MPX_BNDCFG_ADDR_MASK is defined two times, so this patch removes
redundant one.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---

 b/arch/x86/include/asm/mpx.h |    1 -
 1 file changed, 1 deletion(-)

diff -puN arch/x86/include/asm/mpx.h~0001-x86-mpx-remove-redundant-MPX_BNDCFG_ADDR_MASK arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~0001-x86-mpx-remove-redundant-MPX_BNDCFG_ADDR_MASK	2015-05-27 09:32:16.296524773 -0700
+++ b/arch/x86/include/asm/mpx.h	2015-05-27 09:32:16.299524908 -0700
@@ -45,7 +45,6 @@
 #define MPX_BNDSTA_TAIL		2
 #define MPX_BNDCFG_TAIL		12
 #define MPX_BNDSTA_ADDR_MASK	(~((1UL<<MPX_BNDSTA_TAIL)-1))
-#define MPX_BNDCFG_ADDR_MASK	(~((1UL<<MPX_BNDCFG_TAIL)-1))
 #define MPX_BT_ADDR_MASK	(~((1UL<<MPX_BD_ENTRY_TAIL)-1))
 
 #define MPX_BNDCFG_ADDR_MASK	(~((1UL<<MPX_BNDCFG_TAIL)-1))
_

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

* [PATCH 04/19] x86, mpx: Cleanup: Do not pass task around when unnecessary
  2015-05-29 22:34 [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Dave Hansen
                   ` (2 preceding siblings ...)
  2015-05-29 22:34 ` [PATCH 02/19] x86, fpu: Wrap get_xsave_addr() to make it safer Dave Hansen
@ 2015-05-29 22:34 ` Dave Hansen
  2015-05-29 22:34 ` [PATCH 05/19] x86, mpx: remove redundant MPX_BNDCFG_ADDR_MASK Dave Hansen
  2015-06-01 11:14 ` [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Ingo Molnar
  5 siblings, 0 replies; 13+ messages in thread
From: Dave Hansen @ 2015-05-29 22:34 UTC (permalink / raw)
  To: linux-kernel; +Cc: x86, tglx, Dave Hansen, dave.hansen, oleg, bp


From: Dave Hansen <dave.hansen@linux.intel.com>

The MPX code can only work on the current task.  You can not, for
instance, enable MPX management in another process or thread.
You can also not handle a fault for another process or thread.

Despite this, we pass a task_struct around prolifically.  This
patch removes all of the task struct passing for code paths where
the code can not deal with another task (which turns out to be
all of them).

This has no functional changes.  It's just a cleanup.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: bp@alien8.de
Cc: the arch/x86 maintainers <x86@kernel.org>
Cc: linux-kernel <linux-kernel@vger.kernel.org>
---

 b/arch/x86/include/asm/mpx.h       |   10 ++++------
 b/arch/x86/include/asm/processor.h |   12 ++++++------
 b/arch/x86/kernel/traps.c          |    5 ++---
 b/arch/x86/mm/mpx.c                |   19 +++++++++----------
 b/kernel/sys.c                     |    8 ++++----
 5 files changed, 25 insertions(+), 29 deletions(-)

diff -puN arch/x86/include/asm/mpx.h~x86-mpx-dont-pass-current-around arch/x86/include/asm/mpx.h
--- a/arch/x86/include/asm/mpx.h~x86-mpx-dont-pass-current-around	2015-05-27 09:32:15.793502086 -0700
+++ b/arch/x86/include/asm/mpx.h	2015-05-27 09:32:15.804502582 -0700
@@ -59,9 +59,8 @@
 		MPX_BT_ENTRY_MASK) << MPX_BT_ENTRY_SHIFT)
 
 #ifdef CONFIG_X86_INTEL_MPX
-siginfo_t *mpx_generate_siginfo(struct pt_regs *regs,
-				struct task_struct *tsk);
-int mpx_handle_bd_fault(struct task_struct *tsk);
+siginfo_t *mpx_generate_siginfo(struct pt_regs *regs);
+int mpx_handle_bd_fault(void);
 static inline int kernel_managing_mpx_tables(struct mm_struct *mm)
 {
 	return (mm->bd_addr != MPX_INVALID_BOUNDS_DIR);
@@ -77,12 +76,11 @@ static inline void mpx_mm_init(struct mm
 void mpx_notify_unmap(struct mm_struct *mm, struct vm_area_struct *vma,
 		      unsigned long start, unsigned long end);
 #else
-static inline siginfo_t *mpx_generate_siginfo(struct pt_regs *regs,
-					      struct task_struct *tsk)
+static inline siginfo_t *mpx_generate_siginfo(struct pt_regs *regs)
 {
 	return NULL;
 }
-static inline int mpx_handle_bd_fault(struct task_struct *tsk)
+static inline int mpx_handle_bd_fault(void)
 {
 	return -EINVAL;
 }
diff -puN arch/x86/include/asm/processor.h~x86-mpx-dont-pass-current-around arch/x86/include/asm/processor.h
--- a/arch/x86/include/asm/processor.h~x86-mpx-dont-pass-current-around	2015-05-27 09:32:15.795502176 -0700
+++ b/arch/x86/include/asm/processor.h	2015-05-27 09:32:15.804502582 -0700
@@ -802,18 +802,18 @@ extern int get_tsc_mode(unsigned long ad
 extern int set_tsc_mode(unsigned int val);
 
 /* Register/unregister a process' MPX related resource */
-#define MPX_ENABLE_MANAGEMENT(tsk)	mpx_enable_management((tsk))
-#define MPX_DISABLE_MANAGEMENT(tsk)	mpx_disable_management((tsk))
+#define MPX_ENABLE_MANAGEMENT()	mpx_enable_management()
+#define MPX_DISABLE_MANAGEMENT()	mpx_disable_management()
 
 #ifdef CONFIG_X86_INTEL_MPX
-extern int mpx_enable_management(struct task_struct *tsk);
-extern int mpx_disable_management(struct task_struct *tsk);
+extern int mpx_enable_management(void);
+extern int mpx_disable_management(void);
 #else
-static inline int mpx_enable_management(struct task_struct *tsk)
+static inline int mpx_enable_management(void)
 {
 	return -EINVAL;
 }
-static inline int mpx_disable_management(struct task_struct *tsk)
+static inline int mpx_disable_management(void)
 {
 	return -EINVAL;
 }
diff -puN arch/x86/kernel/traps.c~x86-mpx-dont-pass-current-around arch/x86/kernel/traps.c
--- a/arch/x86/kernel/traps.c~x86-mpx-dont-pass-current-around	2015-05-27 09:32:15.797502266 -0700
+++ b/arch/x86/kernel/traps.c	2015-05-27 09:32:15.805502627 -0700
@@ -371,7 +371,6 @@ dotraplinkage void do_double_fault(struc
 
 dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 {
-	struct task_struct *tsk = current;
 	enum ctx_state prev_state;
 	struct bndcsr *bndcsr;
 	siginfo_t *info;
@@ -407,11 +406,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(tsk))
+		if (mpx_handle_bd_fault())
 			goto exit_trap;
 		break; /* Success, it was handled */
 	case 1: /* Bound violation. */
-		info = mpx_generate_siginfo(regs, tsk);
+		info = mpx_generate_siginfo(regs);
 		if (IS_ERR(info)) {
 			/*
 			 * We failed to decode the MPX instruction.  Act as if
diff -puN arch/x86/mm/mpx.c~x86-mpx-dont-pass-current-around arch/x86/mm/mpx.c
--- a/arch/x86/mm/mpx.c~x86-mpx-dont-pass-current-around	2015-05-27 09:32:15.799502356 -0700
+++ b/arch/x86/mm/mpx.c	2015-05-27 09:32:15.806502672 -0700
@@ -271,8 +271,7 @@ bad_opcode:
  *
  * The caller is expected to kfree() the returned siginfo_t.
  */
-siginfo_t *mpx_generate_siginfo(struct pt_regs *regs,
-				struct task_struct *tsk)
+siginfo_t *mpx_generate_siginfo(struct pt_regs *regs)
 {
 	struct bndreg *bndregs, *bndreg;
 	siginfo_t *info = NULL;
@@ -340,7 +339,7 @@ err_out:
 	return ERR_PTR(err);
 }
 
-static __user void *task_get_bounds_dir(struct task_struct *tsk)
+static __user void *mpx_get_bounds_dir(void)
 {
 	struct bndcsr *bndcsr;
 
@@ -376,10 +375,10 @@ static __user void *task_get_bounds_dir(
 		(bndcsr->bndcfgu & MPX_BNDCFG_ADDR_MASK);
 }
 
-int mpx_enable_management(struct task_struct *tsk)
+int mpx_enable_management(void)
 {
 	void __user *bd_base = MPX_INVALID_BOUNDS_DIR;
-	struct mm_struct *mm = tsk->mm;
+	struct mm_struct *mm = current->mm;
 	int ret = 0;
 
 	/*
@@ -393,7 +392,7 @@ int mpx_enable_management(struct task_st
 	 * 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);
+	bd_base = mpx_get_bounds_dir();
 	down_write(&mm->mmap_sem);
 	mm->bd_addr = bd_base;
 	if (mm->bd_addr == MPX_INVALID_BOUNDS_DIR)
@@ -403,7 +402,7 @@ int mpx_enable_management(struct task_st
 	return ret;
 }
 
-int mpx_disable_management(struct task_struct *tsk)
+int mpx_disable_management(void)
 {
 	struct mm_struct *mm = current->mm;
 
@@ -497,7 +496,7 @@ 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 task_struct *tsk)
+static int do_mpx_bt_fault(void)
 {
 	unsigned long bd_entry, bd_base;
 	struct bndcsr *bndcsr;
@@ -525,7 +524,7 @@ static int do_mpx_bt_fault(struct task_s
 	return allocate_bt((long __user *)bd_entry);
 }
 
-int mpx_handle_bd_fault(struct task_struct *tsk)
+int mpx_handle_bd_fault(void)
 {
 	/*
 	 * Userspace never asked us to manage the bounds tables,
@@ -534,7 +533,7 @@ int mpx_handle_bd_fault(struct task_stru
 	if (!kernel_managing_mpx_tables(current->mm))
 		return -EINVAL;
 
-	if (do_mpx_bt_fault(tsk)) {
+	if (do_mpx_bt_fault()) {
 		force_sig(SIGSEGV, current);
 		/*
 		 * The force_sig() is essentially "handling" this
diff -puN kernel/sys.c~x86-mpx-dont-pass-current-around kernel/sys.c
--- a/kernel/sys.c~x86-mpx-dont-pass-current-around	2015-05-27 09:32:15.800502402 -0700
+++ b/kernel/sys.c	2015-05-27 09:32:15.806502672 -0700
@@ -92,10 +92,10 @@
 # define SET_TSC_CTL(a)		(-EINVAL)
 #endif
 #ifndef MPX_ENABLE_MANAGEMENT
-# define MPX_ENABLE_MANAGEMENT(a)	(-EINVAL)
+# define MPX_ENABLE_MANAGEMENT()	(-EINVAL)
 #endif
 #ifndef MPX_DISABLE_MANAGEMENT
-# define MPX_DISABLE_MANAGEMENT(a)	(-EINVAL)
+# define MPX_DISABLE_MANAGEMENT()	(-EINVAL)
 #endif
 #ifndef GET_FP_MODE
 # define GET_FP_MODE(a)		(-EINVAL)
@@ -2230,12 +2230,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsi
 	case PR_MPX_ENABLE_MANAGEMENT:
 		if (arg2 || arg3 || arg4 || arg5)
 			return -EINVAL;
-		error = MPX_ENABLE_MANAGEMENT(me);
+		error = MPX_ENABLE_MANAGEMENT();
 		break;
 	case PR_MPX_DISABLE_MANAGEMENT:
 		if (arg2 || arg3 || arg4 || arg5)
 			return -EINVAL;
-		error = MPX_DISABLE_MANAGEMENT(me);
+		error = MPX_DISABLE_MANAGEMENT();
 		break;
 	case PR_SET_FP_MODE:
 		error = SET_FP_MODE(me, arg2);
_

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

* Re: [PATCH 00/19] x86, mpx updates for 4.2 (take 8)
  2015-05-29 22:34 [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Dave Hansen
                   ` (4 preceding siblings ...)
  2015-05-29 22:34 ` [PATCH 05/19] x86, mpx: remove redundant MPX_BNDCFG_ADDR_MASK Dave Hansen
@ 2015-06-01 11:14 ` Ingo Molnar
  2015-06-01 15:09   ` Dave Hansen
  5 siblings, 1 reply; 13+ messages in thread
From: Ingo Molnar @ 2015-06-01 11:14 UTC (permalink / raw)
  To: Dave Hansen; +Cc: linux-kernel, x86, tglx


* Dave Hansen <dave@sr71.net> wrote:

> 
> Changes from take 7 / v22:
>  * Add Thomas's reviewed-by
>  * merge with tip/x86/fpu changes
>  * Fix tiny spelling nit

So you already sent 'take 8', so it's unclear to me what changed in this series.

Also, only 1,3,4,5 made it to lkml it appears. (it's not in my spam box either.)

Thanks,

	Ingo

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

* Re: [PATCH 00/19] x86, mpx updates for 4.2 (take 8)
  2015-06-01 11:14 ` [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Ingo Molnar
@ 2015-06-01 15:09   ` Dave Hansen
  0 siblings, 0 replies; 13+ messages in thread
From: Dave Hansen @ 2015-06-01 15:09 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: linux-kernel, x86, tglx

On 06/01/2015 04:14 AM, Ingo Molnar wrote:
> So you already sent 'take 8', so it's unclear to me what changed in this series.
> 
> Also, only 1,3,4,5 made it to lkml it appears. (it's not in my spam box either.)

I accidentally send that.  I'll send a real series with a proper
changelog shortly.

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

* [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions
  2015-06-07 18:37 [PATCH 00/19] x86, mpx updates for 4.2 (take 9) Dave Hansen
@ 2015-06-07 18:37 ` Dave Hansen
  0 siblings, 0 replies; 13+ messages in thread
From: Dave Hansen @ 2015-06-07 18:37 UTC (permalink / raw)
  To: linux-kernel; +Cc: x86, tglx, Dave Hansen, dave.hansen


From: Dave Hansen <dave.hansen@linux.intel.com>

get_xsave_addr() assumes that if an xsave bit is present in the
hardware (pcntxt_mask) that it is present in a given xsave
buffer.  Due to an bug in the xsave code on all of the systems
that have MPX (and thus all the users of this code), that has
been a true assumption.

But, the bug is getting fixed, so our assumption is not going
to hold any more.

It's quite possible (and normal) for an enabled state to be
present on 'pcntxt_mask', but *not* in 'xstate_bv'.  We need
to consult 'xstate_bv'.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---

 b/arch/x86/kernel/fpu/xstate.c |   45 +++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff -puN arch/x86/kernel/fpu/xstate.c~consullt-xstate_bv arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~consullt-xstate_bv	2015-06-01 10:24:03.025676699 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-06-01 10:24:03.029676880 -0700
@@ -382,19 +382,48 @@ void fpu__resume_cpu(void)
  * This is the API that is called to get xstate address in either
  * standard format or compacted format of xsave area.
  *
+ * Note that if there is no data for the field in the xsave buffer
+ * this will return NULL.
+ *
  * Inputs:
- *	xsave: base address of the xsave area;
- *	xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
- *	etc.)
+ *	xstate: the thread's storage area for all FPU data
+ *	xstate_feature: state which is defined in xsave.h (e.g.
+ *	XSTATE_FP, XSTATE_SSE, etc...)
  * Output:
- *	address of the state in the xsave area.
+ *	address of the state in the xsave area, or NULL if the
+ *	field is not present in the xsave buffer.
  */
-void *get_xsave_addr(struct xregs_state *xsave, int xstate)
+void *get_xsave_addr(struct xregs_state *xsave, int xstate_feature)
 {
-	int feature = fls64(xstate) - 1;
-	if (!test_bit(feature, (unsigned long *)&xfeatures_mask))
+	int feature_nr = fls64(xstate_feature) - 1;
+	/*
+	 * Do we even *have* xsave state?
+	 */
+	if (!boot_cpu_has(X86_FEATURE_XSAVE))
+		return NULL;
+
+	xsave = &current->thread.fpu.state.xsave;
+	/*
+	 * We should not ever be requesting features that we
+	 * have not enabled.  Remember that pcntxt_mask is
+	 * what we write to the XCR0 register.
+	 */
+	WARN_ONCE(!(xfeatures_mask & xstate_feature),
+		  "get of unsupported state");
+	/*
+	 * This assumes the last 'xsave*' instruction to
+	 * have requested that 'xstate_feature' be saved.
+	 * If it did not, we might be seeing and old value
+	 * of the field in the buffer.
+	 *
+	 * This can happen because the last 'xsave' did not
+	 * request that this feature be saved (unlikely)
+	 * or because the "init optimization" caused it
+	 * to not be saved.
+	 */
+	if (!(xsave->header.xfeatures & xstate_feature))
 		return NULL;
 
-	return (void *)xsave + xstate_comp_offsets[feature];
+	return (void *)xsave + xstate_comp_offsets[feature_nr];
 }
 EXPORT_SYMBOL_GPL(get_xsave_addr);
_

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

* [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions
  2015-05-27 18:36 [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Dave Hansen
@ 2015-05-27 18:36 ` Dave Hansen
  0 siblings, 0 replies; 13+ messages in thread
From: Dave Hansen @ 2015-05-27 18:36 UTC (permalink / raw)
  To: linux-kernel; +Cc: x86, tglx, Dave Hansen, dave.hansen


From: Dave Hansen <dave.hansen@linux.intel.com>

get_xsave_addr() assumes that if an xsave bit is present in the
hardware (pcntxt_mask) that it is present in a given xsave
buffer.  Due to an bug in the xsave code on all of the systems
that have MPX (and thus all the users of this code), that has
been a true assumption.

But, the bug is getting fixed, so our assumption is not going
to hold any more.

It's quite possible (and normal) for an enabled state to be
present on 'pcntxt_mask', but *not* in 'xstate_bv'.  We need
to consult 'xstate_bv'.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---

 b/arch/x86/kernel/fpu/xstate.c |   45 +++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff -puN arch/x86/kernel/fpu/xstate.c~consullt-xstate_bv arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~consullt-xstate_bv	2015-05-27 09:32:14.540445571 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-05-27 09:32:14.543445706 -0700
@@ -382,19 +382,48 @@ void fpu__resume_cpu(void)
  * This is the API that is called to get xstate address in either
  * standard format or compacted format of xsave area.
  *
+ * Note that if there is no data for the field in the xsave buffer
+ * this will return NULL.
+ *
  * Inputs:
- *	xsave: base address of the xsave area;
- *	xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
- *	etc.)
+ *	xstate: the thread's storage area for all FPU data
+ *	xstate_feature: state which is defined in xsave.h (e.g.
+ *	XSTATE_FP, XSTATE_SSE, etc...)
  * Output:
- *	address of the state in the xsave area.
+ *	address of the state in the xsave area, or NULL if the
+ *	field is not present in the xsave buffer.
  */
-void *get_xsave_addr(struct xregs_state *xsave, int xstate)
+void *get_xsave_addr(struct xregs_state *xsave, int xstate_feature)
 {
-	int feature = fls64(xstate) - 1;
-	if (!test_bit(feature, (unsigned long *)&xfeatures_mask))
+	int feature_nr = fls64(xstate_feature) - 1;
+	/*
+	 * Do we even *have* xsave state?
+	 */
+	if (!boot_cpu_has(X86_FEATURE_XSAVE))
+		return NULL;
+
+	xsave = &current->thread.fpu.state.xsave;
+	/*
+	 * We should not ever be requesting features that we
+	 * have not enabled.  Remember that pcntxt_mask is
+	 * what we write to the XCR0 register.
+	 */
+	WARN_ONCE(!(xfeatures_mask & xstate_feature),
+		  "get of unsupported state");
+	/*
+	 * This assumes the last 'xsave*' instruction to
+	 * have requested that 'xstate_feature' be saved.
+	 * If it did not, we might be seeing and old value
+	 * of the field in the buffer.
+	 *
+	 * This can happen because the last 'xsave' did not
+	 * request that this feature be saved (unlikely)
+	 * or because the "init optimization" caused it
+	 * to not be saved.
+	 */
+	if (!(xsave->header.xfeatures & xstate_feature))
 		return NULL;
 
-	return (void *)xsave + xstate_comp_offsets[feature];
+	return (void *)xsave + xstate_comp_offsets[feature_nr];
 }
 EXPORT_SYMBOL_GPL(get_xsave_addr);
_

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

* [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions
  2015-05-19  6:25 [PATCH 00/19] x86, mpx updates for 4.2 (take 7) Dave Hansen
@ 2015-05-19  6:25 ` Dave Hansen
  0 siblings, 0 replies; 13+ messages in thread
From: Dave Hansen @ 2015-05-19  6:25 UTC (permalink / raw)
  To: linux-kernel; +Cc: x86, tglx, Dave Hansen, dave.hansen


From: Dave Hansen <dave.hansen@linux.intel.com>

get_xsave_addr() assumes that if an xsave bit is present in the
hardware (pcntxt_mask) that it is present in a given xsave
buffer.  Due to an bug in the xsave code on all of the systems
that have MPX (and thus all the users of this code), that has
been a true assumption.

But, the bug is getting fixed, so our assumption is not going
to hold any more.

It's quite possible (and normal) for an enabled state to be
present on 'pcntxt_mask', but *not* in 'xstate_bv'.  We need
to consult 'xstate_bv'.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---

 b/arch/x86/kernel/xsave.c |   44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff -puN arch/x86/kernel/xsave.c~consullt-xstate_bv arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~consullt-xstate_bv	2015-05-18 17:48:57.839373364 -0700
+++ b/arch/x86/kernel/xsave.c	2015-05-18 17:48:57.843373544 -0700
@@ -706,19 +706,47 @@ void __init_refok eager_fpu_init(void)
  * This is the API that is called to get xstate address in either
  * standard format or compacted format of xsave area.
  *
+ * Note that if there is no data for the field in the xsave buffer
+ * this will return NULL.
+ *
  * Inputs:
- *	xsave: base address of the xsave area;
- *	xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
- *	etc.)
+ *	xstate: the thread's storage area for all FPU data
+ *	xstate_field: state which is defined in xsave.h (e.g. XSTATE_FP,
+ *	XSTATE_SSE, etc...)
  * Output:
- *	address of the state in the xsave area.
+ *	address of the state in the xsave area, or NULL if the
+ *	field is not present in the xsave buffer.
  */
-void *get_xsave_addr(struct xsave_struct *xsave, int xstate)
+void *get_xsave_addr(struct xsave_struct *xsave, int xstate_field)
 {
-	int feature = fls64(xstate) - 1;
-	if (!test_bit(feature, (unsigned long *)&pcntxt_mask))
+	int feature_nr = fls64(xstate_field) - 1;
+	/*
+	 * Do we even *have* xsave state?
+	 */
+	if (!boot_cpu_has(X86_FEATURE_XSAVE))
+		return NULL;
+
+	xsave = &current->thread.fpu.state->xsave;
+	/*
+	 * We should not ever be requesting fields that we
+	 * have not enabled.  Remember that pcntxt_mask is
+	 * what we write to the XCR0 register.
+	 */
+	WARN_ONCE(!(pcntxt_mask & xstate_field), "get of unsupported state");
+	/*
+	 * This assumes the last 'xsave*' instruction to
+	 * have requested that 'xstate_field' be saved.
+	 * If it did not, we might be seeing and old value
+	 * of the field in the buffer.
+	 *
+	 * This can happen because the last 'xsave' did not
+	 * request that this feature be saved (unlikely)
+	 * or because the "init optimization" caused it
+	 * to not be saved.
+	 */
+	if (!(xsave->xsave_hdr.xstate_bv & xstate_field))
 		return NULL;
 
-	return (void *)xsave + xstate_comp_offsets[feature];
+	return (void *)xsave + xstate_comp_offsets[feature_nr];
 }
 EXPORT_SYMBOL_GPL(get_xsave_addr);
_

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

* Re: [PATCH 01/19] x86, mpx, xsave: fix up bad get_xsave_addr() assumptions
  2015-05-08 18:59 ` [PATCH 01/19] x86, mpx, xsave: fix up bad get_xsave_addr() assumptions Dave Hansen
@ 2015-05-18 19:34   ` Thomas Gleixner
  0 siblings, 0 replies; 13+ messages in thread
From: Thomas Gleixner @ 2015-05-18 19:34 UTC (permalink / raw)
  To: Dave Hansen; +Cc: linux-kernel, x86, dave.hansen

On Fri, 8 May 2015, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> get_xsave_addr() assumes that if an xsave bit is present in the
> hardware (pcntxt_mask) that it is present in a given xsave
> buffer.  Due to an bug in the xsave code on all of the systems
> that have MPX (and thus all the users of this code), that has
> been a true assumption.
> 
> But, the bug is getting fixed, so our assumption is not going
> to hold any more.
> 
> It's quite possible (and normal) for an enabled state to be
> present on 'pcntxt_mask', but *not* in 'xstate_bv'.  We need
> to consult 'xstate_bv'.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

* [PATCH 01/19] x86, mpx, xsave: fix up bad get_xsave_addr() assumptions
  2015-05-08 18:59 [PATCH 00/19] x86, mpx updates for 4.2 (take 6) Dave Hansen
@ 2015-05-08 18:59 ` Dave Hansen
  2015-05-18 19:34   ` Thomas Gleixner
  0 siblings, 1 reply; 13+ messages in thread
From: Dave Hansen @ 2015-05-08 18:59 UTC (permalink / raw)
  To: linux-kernel; +Cc: x86, tglx, Dave Hansen, dave.hansen


From: Dave Hansen <dave.hansen@linux.intel.com>

get_xsave_addr() assumes that if an xsave bit is present in the
hardware (pcntxt_mask) that it is present in a given xsave
buffer.  Due to an bug in the xsave code on all of the systems
that have MPX (and thus all the users of this code), that has
been a true assumption.

But, the bug is getting fixed, so our assumption is not going
to hold any more.

It's quite possible (and normal) for an enabled state to be
present on 'pcntxt_mask', but *not* in 'xstate_bv'.  We need
to consult 'xstate_bv'.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
---

 b/arch/x86/kernel/xsave.c |   41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff -puN arch/x86/kernel/xsave.c~consullt-xstate_bv arch/x86/kernel/xsave.c
--- a/arch/x86/kernel/xsave.c~consullt-xstate_bv	2015-05-08 11:46:10.595563814 -0700
+++ b/arch/x86/kernel/xsave.c	2015-05-08 11:46:10.598563949 -0700
@@ -706,19 +706,46 @@ void __init_refok eager_fpu_init(void)
  * This is the API that is called to get xstate address in either
  * standard format or compacted format of xsave area.
  *
+ * Note that if there is no data for the field in the xsave buffer
+ * this will return NULL.
+ *
  * Inputs:
- *	xsave: base address of the xsave area;
- *	xstate: state which is defined in xsave.h (e.g. XSTATE_FP, XSTATE_SSE,
- *	etc.)
+ *	xstate: the thread's storage area for all FPU data
+ *	xstate_field: state which is defined in xsave.h (e.g. XSTATE_FP,
+ *	XSTATE_SSE, etc...)
  * Output:
  *	address of the state in the xsave area.
  */
-void *get_xsave_addr(struct xsave_struct *xsave, int xstate)
+void *get_xsave_addr(struct xsave_struct *xsave, int xstate_field)
 {
-	int feature = fls64(xstate) - 1;
-	if (!test_bit(feature, (unsigned long *)&pcntxt_mask))
+	int feature_nr = fls64(xstate_field) - 1;
+	/*
+	 * Do we even *have* xsave state?
+	 */
+	if (!boot_cpu_has(X86_FEATURE_XSAVE))
+		return NULL;
+
+	xsave = &current->thread.fpu.state->xsave;
+	/*
+	 * We should not ever be requesting fields that we
+	 * have not enabled.  Remember that pcntxt_mask is
+	 * what we write to the XCR0 register.
+	 */
+	WARN_ONCE(!(pcntxt_mask & xstate_field), "get of unsupported state");
+	/*
+	 * This assumes the last 'xsave*' instruction to
+	 * have requested that 'xstate_field' be saved.
+	 * If it did not, we might be seeing and old value
+	 * of the field in the buffer.
+	 *
+	 * This can happen because the last 'xsave' did not
+	 * request that this feature be saved (unlikely)
+	 * or because the "init optimization" caused it
+	 * to not be saved.
+	 */
+	if (!(xsave->xsave_hdr.xstate_bv & xstate_field))
 		return NULL;
 
-	return (void *)xsave + xstate_comp_offsets[feature];
+	return (void *)xsave + xstate_comp_offsets[feature_nr];
 }
 EXPORT_SYMBOL_GPL(get_xsave_addr);
_

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

end of thread, other threads:[~2015-06-07 18:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-29 22:34 [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Dave Hansen
2015-05-29 22:34 ` [PATCH 03/19] x86, mpx: Use new get_xsave_field_ptr() Dave Hansen
2015-05-29 22:34 ` [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions Dave Hansen
2015-05-29 22:34 ` [PATCH 02/19] x86, fpu: Wrap get_xsave_addr() to make it safer Dave Hansen
2015-05-29 22:34 ` [PATCH 04/19] x86, mpx: Cleanup: Do not pass task around when unnecessary Dave Hansen
2015-05-29 22:34 ` [PATCH 05/19] x86, mpx: remove redundant MPX_BNDCFG_ADDR_MASK Dave Hansen
2015-06-01 11:14 ` [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Ingo Molnar
2015-06-01 15:09   ` Dave Hansen
  -- strict thread matches above, loose matches on Subject: below --
2015-06-07 18:37 [PATCH 00/19] x86, mpx updates for 4.2 (take 9) Dave Hansen
2015-06-07 18:37 ` [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions Dave Hansen
2015-05-27 18:36 [PATCH 00/19] x86, mpx updates for 4.2 (take 8) Dave Hansen
2015-05-27 18:36 ` [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions Dave Hansen
2015-05-19  6:25 [PATCH 00/19] x86, mpx updates for 4.2 (take 7) Dave Hansen
2015-05-19  6:25 ` [PATCH 01/19] x86, mpx, xsave: Fix up bad get_xsave_addr() assumptions Dave Hansen
2015-05-08 18:59 [PATCH 00/19] x86, mpx updates for 4.2 (take 6) Dave Hansen
2015-05-08 18:59 ` [PATCH 01/19] x86, mpx, xsave: fix up bad get_xsave_addr() assumptions Dave Hansen
2015-05-18 19:34   ` Thomas Gleixner

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).