[RFC][PATCH 1/2] x86, fpu: disable XSAVE if init buf too small

[RFC][PATCH 1/2] x86, fpu: disable XSAVE if init buf too small

[Dave Hansen]

This is _very_ lightly tested.  Just RFC for now.  The fallback
code is not tested at all because I'm hitting another bug.

--

The recent x86 FPU rework changed a dynamic allocation to a
static one.  But, the static one is undersized and we overrun it,
corrupting memory in some cases.

This patch detects the situation and disables the XSAVE feature.

I have not been able to test this in practice because I'm hitting
another bug.  tsk->signal gets corrupted if I disable XSAVE.
tsk->signal is conveniently located a few hundred bytes after the
'struct fpu' which is now embedded in task_struct, so I suspect
this is another overrun.  Anyone should be able to reproduce this
by under-sizing XSTATE_MAX_SIZE in the next patch.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: leg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>

---

 b/arch/x86/include/asm/fpu/internal.h |    1 
 b/arch/x86/kernel/fpu/init.c          |   17 ++++++++------
 b/arch/x86/kernel/fpu/xstate.c        |   41 +++++++++++++++++++++++-----------
 3 files changed, 39 insertions(+), 20 deletions(-)

diff -puN arch/x86/include/asm/fpu/internal.h~disable-xsave-if-init-buf-too-small arch/x86/include/asm/fpu/internal.h
--- a/arch/x86/include/asm/fpu/internal.h~disable-xsave-if-init-buf-too-small	2015-07-16 01:11:02.889010467 -0700
+++ b/arch/x86/include/asm/fpu/internal.h	2015-07-16 01:11:33.928398118 -0700
@@ -42,6 +42,7 @@ extern void fpu__init_cpu_xstate(void);
 extern void fpu__init_system(struct cpuinfo_x86 *c);
 extern void fpu__init_check_bugs(void);
 extern void fpu__resume_cpu(void);
+extern void fpu__clear_all_xsave_cpu_caps(void);
 
 /*
  * Debugging facility:
diff -puN arch/x86/kernel/fpu/init.c~disable-xsave-if-init-buf-too-small arch/x86/kernel/fpu/init.c
--- a/arch/x86/kernel/fpu/init.c~disable-xsave-if-init-buf-too-small	2015-07-16 01:11:02.890010512 -0700
+++ b/arch/x86/kernel/fpu/init.c	2015-07-16 01:11:02.896010780 -0700
@@ -301,20 +301,23 @@ static int __init no_387(char *s)
 }
 __setup("no387", no_387);
 
-/*
- * Disable all xstate CPU features:
- */
-static int __init x86_noxsave_setup(char *s)
+void fpu__clear_all_xsave_cpu_caps(void)
 {
-	if (strlen(s))
-		return 0;
-
 	setup_clear_cpu_cap(X86_FEATURE_XSAVE);
 	setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
 	setup_clear_cpu_cap(X86_FEATURE_XSAVES);
 	setup_clear_cpu_cap(X86_FEATURE_AVX);
 	setup_clear_cpu_cap(X86_FEATURE_AVX2);
+}
 
+/*
+ * Disable all xstate CPU features:
+ */
+static int __init x86_noxsave_setup(char *s)
+{
+	if (strlen(s))
+		return 0;
+	fpu__clear_all_xsave_cpu_caps();
 	return 1;
 }
 __setup("noxsave", x86_noxsave_setup);
diff -puN arch/x86/kernel/fpu/xstate.c~disable-xsave-if-init-buf-too-small arch/x86/kernel/fpu/xstate.c
--- a/arch/x86/kernel/fpu/xstate.c~disable-xsave-if-init-buf-too-small	2015-07-16 01:11:02.892010601 -0700
+++ b/arch/x86/kernel/fpu/xstate.c	2015-07-16 01:11:02.895010735 -0700
@@ -291,26 +291,37 @@ static void __init setup_init_fpu_buf(vo
 }
 
 /*
- * Calculate total size of enabled xstates in XCR0/xfeatures_mask.
+ * Ask the CPU for the total size of the buffer needed to save all the
+ * enabled xstates in XCR0/xfeatures_mask.
  */
-static void __init init_xstate_size(void)
+static int __init fetch_xstate_size(void)
 {
 	unsigned int eax, ebx, ecx, edx;
-	int i;
 
 	if (!cpu_has_xsaves) {
+		/* Standard format */
 		cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
-		xstate_size = ebx;
-		return;
+	} else {
+		/* Compacted format */
+		cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
 	}
 
-	xstate_size = FXSAVE_SIZE + XSAVE_HDR_SIZE;
-	for (i = 2; i < 64; i++) {
-		if (test_bit(i, (unsigned long *)&xfeatures_mask)) {
-			cpuid_count(XSTATE_CPUID, i, &eax, &ebx, &ecx, &edx);
-			xstate_size += eax;
-		}
-	}
+	return ebx;
+}
+
+/*
+ * We did not allocate enough size in our init_fpstate to hold
+ * the xsave buffer on this system.  Back out the xsave enabling
+ * we did so far and disable xsaves.
+ */
+static void xstate_size_overflow_error(void)
+{
+	pr_warn("x86/fpu: xstates too large (%d), disabling XSAVE\n", xstate_size);
+	/* Disable the xsave feature itself */
+	cr4_clear_bits(X86_CR4_OSXSAVE);
+
+	/* Clear the software copy of all the xsave-related cpuid bits */
+	fpu__clear_all_xsave_cpu_caps();
 }
 
 /*
@@ -350,7 +361,11 @@ void __init fpu__init_system_xstate(void
 	fpu__init_cpu_xstate();
 
 	/* Recompute the context size for enabled features: */
-	init_xstate_size();
+	xstate_size = fetch_xstate_size();
+	if (xstate_size > sizeof(init_fpstate.xsave)) {
+		xstate_size_overflow_error();
+		return;
+	}
 
 	update_regset_xstate_info(xstate_size, xfeatures_mask);
 	fpu__init_prepare_fx_sw_frame();
_

[RFC][PATCH 2/2] x86, fpu: enlarge xregs_state

[Dave Hansen]

This takes a stab at sizing the xsave buffer so that it will work for
AVX-512 systems.  The logic is explained in the comment.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: leg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>

---

 b/arch/x86/include/asm/fpu/types.h |   27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff -puN arch/x86/include/asm/fpu/types.h~enlarge-init-fpu-buf arch/x86/include/asm/fpu/types.h
--- a/arch/x86/include/asm/fpu/types.h~enlarge-init-fpu-buf	2015-07-16 01:11:03.301028886 -0700
+++ b/arch/x86/include/asm/fpu/types.h	2015-07-16 01:11:03.304029021 -0700
@@ -159,10 +159,19 @@ struct xstate_header {
 	u64				reserved[6];
 } __attribute__((packed));
 
-/* New processor state extensions should be added here: */
-#define XSTATE_RESERVE			(sizeof(struct ymmh_struct) + \
-					 sizeof(struct lwp_struct)  + \
-					 sizeof(struct mpx_struct)  )
+/*
+ * The largest xsave buffer known today is 2752 bytes on a system
+ * implementing AVX-512.  This includes the 512-byte i387 state
+ * and 64-byte header.  We add a small amount of padding in case
+ * an implementation adds some padding or wastes some space.
+ *
+ * Note, if we overflow this, we will disable XSAVE completely.
+ *
+ * Also, note that the real size we need is enumerated by
+ * cpuid leaves and can not be known at compile time.
+ */
+#define XSTATE_MAX_SIZE	(2752 + 256)
+
 /*
  * This is our most modern FPU state format, as saved by the XSAVE
  * and restored by the XRSTOR instructions.
@@ -172,9 +181,13 @@ struct xstate_header {
  * Not all CPUs support all the extensions.
  */
 struct xregs_state {
-	struct fxregs_state		i387;
-	struct xstate_header		header;
-	u8				__reserved[XSTATE_RESERVE];
+	union {
+		struct {
+			struct fxregs_state		i387;
+			struct xstate_header		header;
+		};
+		u8 __reserved[XSTATE_MAX_SIZE];
+	};
 } __attribute__ ((packed, aligned (64)));
 
 /*
_

Re: [RFC][PATCH 2/2] x86, fpu: enlarge xregs_state

[Dave Hansen]

On 07/16/2015 01:13 AM, Dave Hansen wrote:
> +/*
> + * The largest xsave buffer known today is 2752 bytes on a system
> + * implementing AVX-512.  This includes the 512-byte i387 state
> + * and 64-byte header.  We add a small amount of padding in case
> + * an implementation adds some padding or wastes some space.
> + *
> + * Note, if we overflow this, we will disable XSAVE completely.
> + *
> + * Also, note that the real size we need is enumerated by
> + * cpuid leaves and can not be known at compile time.
> + */
> +#define XSTATE_MAX_SIZE	(2752 + 256)

BTW, this has one big-ish side-effect.  It takes the size of task_struct
from ~3.5k to ~5.4k for me:

slabinfo before:
task_struct          198    198   3456    9    8 : ...
after:
task_struct          166    180   5376    6    8 : ...

I'm sure folks on small systems are going to cringe at eating 2k/thread,
so we've got to revisit this _somehow_.