[PATCH v2 0/8] Support XSAVES supervisor states

[PATCH v2 0/8] Support XSAVES supervisor states

[Yu-cheng Yu]

Changes from v1 to v2:

- Split out small changes to:
    https://lkml.kernel.org/r/20191212210855.19260-1-yu-cheng.yu@intel.com/

- Fix an issue in patch #4, where fpu__clear_user_states() drops
  supervisor xstates.

- Add three patches:
    Patch #6: Update sanitize_restored_xstate() for supervisor xstates.
    Patch #7: Update copy_kernel_to_xregs_err() to use XRSTORS when
              supervisor xstates are present.
    Patch #8: Update __fpu__restore_sig() to preserve supervisor xstates.

Also make some small changes in response to comments.  More details are in
each patch's commit log.

There are two types of XSAVE-managed states (xstates): user and supervisor.
This series introduces the supervisor xstate support in preparation for new
features that will make use of supervisor xstates.

This series has been separated for ease of review from the series that add
supervisor xstate features [3].

In current and near future generations of Intel processors there are three
classes of objects that can be managed as supervisor xstates:

- Processor Trace (PT):
    Linux already supports PT, but PT xstates are not saved to the FPU
    context and not context-switched by the kernel.  There are no plans to
    integrate PT into XSAVES supervisor states.

- ENQCMD Process Address Space ID (MSR_IA32_PASID):
    ENQCMD is a new instruction and will be introduced shortly in a
    separate series [2].

- Control-flow Enforcement Technology (CET):
    CET is being reviewed on the LKML [1] [3].

Supervisor xstates can be accessed only from the kernel (PL-0) with XSAVES/
XRSTORS instructions.  They cannot be accessed with other XSAVE*/XRSTOR*
instructions.  MSR_IA32_XSS sets enabled supervisor xstates, while XCR0
sets enabled user xstates.

This series separates the two xstate types by declaring new macros for each
type.  The kernel finds all available features during system initialization
and stores them in xfeatures_mask_all.  It then retrieves perspective
xstate type with xfeatures_mask_supervisor()/xfeatures_mask_user() for
handling signals and PTRACE.

[1] Detailed information on supervisor xstates can be found in "Intel 64
    and IA-32 Architectures Software Developer's Manual":

    https://software.intel.com/en-us/download/intel-64-and-ia-32-
    architectures-sdm-combined-volumes-1-2a-2b-2c-2d-3a-3b-3c-3d-and-4

[2] Detailed information on the ENQCMD instruction and MSR_IA32_PASID can
    be found in "Intel Architecture Instruction Set Extensions and Future
    Features Programming Reference":

    https://software.intel.com/sites/default/files/managed/c5/15/
    architecture-instruction-set-extensions-programming-reference.pdf

[3] CET patches:

    https://lkml.kernel.org/r/20190813205225.12032-1-yu-cheng.yu@intel.com/
    https://lkml.kernel.org/r/20190813205359.12196-1-yu-cheng.yu@intel.com/

Fenghua Yu (3):
  x86/fpu/xstate: Define new macros for supervisor and user xstates
  x86/fpu/xstate: Define new functions for clearing fpregs and xstates
  x86/fpu/xstate: Rename validate_xstate_header() to
    validate_xstate_header_from_user()

Yu-cheng Yu (5):
  x86/fpu/xstate: Separate user and supervisor xfeatures mask
  x86/fpu/xstate: Introduce XSAVES supervisor states
  x86/fpu/xstate: Update sanitize_restored_xstate() for supervisor
    xstates
  x86/fpu/xstate: Update copy_kernel_to_xregs_err() for XSAVES
    supervisor states
  x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

 arch/x86/include/asm/fpu/internal.h |  10 ++-
 arch/x86/include/asm/fpu/xstate.h   |  51 +++++++++----
 arch/x86/kernel/fpu/core.c          |  41 ++++++++---
 arch/x86/kernel/fpu/init.c          |   3 +-
 arch/x86/kernel/fpu/regset.c        |   2 +-
 arch/x86/kernel/fpu/signal.c        |  80 ++++++++++++++------
 arch/x86/kernel/fpu/xstate.c        | 109 ++++++++++++++++------------
 arch/x86/kernel/process.c           |   2 +-
 arch/x86/kernel/signal.c            |   2 +-
 9 files changed, 198 insertions(+), 102 deletions(-)

-- 
2.21.0

[PATCH v2 1/8] x86/fpu/xstate: Define new macros for supervisor and user xstates

[Yu-cheng Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

XCNTXT_MASK is 'all supported xfeatures' before introducing supervisor
xstates.  Rename it to SUPPORTED_XFEATURES_MASK_USER to make clear that
these are user xstates.

XFEATURE_MASK_SUPERVISOR is replaced with the following:
- SUPPORTED_XFEATURES_MASK_SUPERVISOR: Currently nothing.  ENQCMD and
  Control-flow Enforcement Technology (CET) will be introduced in separate
  series.
- UNSUPPORTED_XFEATURES_MASK_SUPERVISOR: Currently only Processor Trace.
- ALL_XFEATURES_MASK_SUPERVISOR: the combination of above.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Co-developed-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/fpu/xstate.h | 36 ++++++++++++++++++++-----------
 arch/x86/kernel/fpu/init.c        |  3 ++-
 arch/x86/kernel/fpu/xstate.c      | 26 +++++++++++-----------
 3 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index c6136d79f8c0..014c386deaa3 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -21,19 +21,29 @@
 #define XSAVE_YMM_SIZE	    256
 #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
 
-/* Supervisor features */
-#define XFEATURE_MASK_SUPERVISOR (XFEATURE_MASK_PT)
-
-/* All currently supported features */
-#define XCNTXT_MASK		(XFEATURE_MASK_FP | \
-				 XFEATURE_MASK_SSE | \
-				 XFEATURE_MASK_YMM | \
-				 XFEATURE_MASK_OPMASK | \
-				 XFEATURE_MASK_ZMM_Hi256 | \
-				 XFEATURE_MASK_Hi16_ZMM	 | \
-				 XFEATURE_MASK_PKRU | \
-				 XFEATURE_MASK_BNDREGS | \
-				 XFEATURE_MASK_BNDCSR)
+/* All currently supported user features */
+#define SUPPORTED_XFEATURES_MASK_USER (XFEATURE_MASK_FP | \
+				       XFEATURE_MASK_SSE | \
+				       XFEATURE_MASK_YMM | \
+				       XFEATURE_MASK_OPMASK | \
+				       XFEATURE_MASK_ZMM_Hi256 | \
+				       XFEATURE_MASK_Hi16_ZMM	 | \
+				       XFEATURE_MASK_PKRU | \
+				       XFEATURE_MASK_BNDREGS | \
+				       XFEATURE_MASK_BNDCSR)
+
+/* All currently supported supervisor features */
+#define SUPPORTED_XFEATURES_MASK_SUPERVISOR (0)
+
+/*
+ * Unsupported supervisor features. When a supervisor feature in this mask is
+ * supported in the future, move it to the supported supervisor feature mask.
+ */
+#define UNSUPPORTED_XFEATURES_MASK_SUPERVISOR (XFEATURE_MASK_PT)
+
+/* All supervisor states including supported and unsupported states. */
+#define ALL_XFEATURES_MASK_SUPERVISOR (SUPPORTED_XFEATURES_MASK_SUPERVISOR | \
+				       UNSUPPORTED_XFEATURES_MASK_SUPERVISOR)
 
 #ifdef CONFIG_X86_64
 #define REX_PREFIX	"0x48, "
diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index 6ce7e0a23268..ba3705d25162 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -224,7 +224,8 @@ static void __init fpu__init_system_xstate_size_legacy(void)
  */
 u64 __init fpu__get_supported_xfeatures_mask(void)
 {
-	return XCNTXT_MASK;
+	return SUPPORTED_XFEATURES_MASK_USER |
+	       SUPPORTED_XFEATURES_MASK_SUPERVISOR;
 }
 
 /* Legacy code to initialize eager fpu mode. */
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 4fa494073289..7d8e9414efa4 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -213,14 +213,13 @@ void fpu__init_cpu_xstate(void)
 	if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask)
 		return;
 	/*
-	 * Make it clear that XSAVES supervisor states are not yet
-	 * implemented should anyone expect it to work by changing
-	 * bits in XFEATURE_MASK_* macros and XCR0.
+	 * Unsupported supervisor xstates should not be found in
+	 * the xfeatures mask.
 	 */
-	WARN_ONCE((xfeatures_mask & XFEATURE_MASK_SUPERVISOR),
-		"x86/fpu: XSAVES supervisor states are not yet implemented.\n");
+	WARN_ONCE((xfeatures_mask & UNSUPPORTED_XFEATURES_MASK_SUPERVISOR),
+		  "x86/fpu: Found unsupported supervisor xstates.\n");
 
-	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
+	xfeatures_mask &= ~UNSUPPORTED_XFEATURES_MASK_SUPERVISOR;
 
 	cr4_set_bits(X86_CR4_OSXSAVE);
 	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
@@ -443,7 +442,7 @@ static int xfeature_uncompacted_offset(int xfeature_nr)
 	 * format. Checking a supervisor state's uncompacted offset is
 	 * an error.
 	 */
-	if (XFEATURE_MASK_SUPERVISOR & BIT_ULL(xfeature_nr)) {
+	if (ALL_XFEATURES_MASK_SUPERVISOR & BIT_ULL(xfeature_nr)) {
 		WARN_ONCE(1, "No fixed offset for xstate %d\n", xfeature_nr);
 		return -1;
 	}
@@ -480,7 +479,7 @@ int using_compacted_format(void)
 int validate_xstate_header(const struct xstate_header *hdr)
 {
 	/* No unknown or supervisor features may be set */
-	if (hdr->xfeatures & (~xfeatures_mask | XFEATURE_MASK_SUPERVISOR))
+	if (hdr->xfeatures & ~(xfeatures_mask & SUPPORTED_XFEATURES_MASK_USER))
 		return -EINVAL;
 
 	/* Userspace must use the uncompacted format */
@@ -773,7 +772,8 @@ void __init fpu__init_system_xstate(void)
 	 * Update info used for ptrace frames; use standard-format size and no
 	 * supervisor xstates:
 	 */
-	update_regset_xstate_info(fpu_user_xstate_size,	xfeatures_mask & ~XFEATURE_MASK_SUPERVISOR);
+	update_regset_xstate_info(fpu_user_xstate_size,
+				  xfeatures_mask & SUPPORTED_XFEATURES_MASK_USER);
 
 	fpu__init_prepare_fx_sw_frame();
 	setup_init_fpu_buf();
@@ -996,7 +996,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 	 */
 	memset(&header, 0, sizeof(header));
 	header.xfeatures = xsave->header.xfeatures;
-	header.xfeatures &= ~XFEATURE_MASK_SUPERVISOR;
+	header.xfeatures &= SUPPORTED_XFEATURES_MASK_USER;
 
 	/*
 	 * Copy xregs_state->header:
@@ -1080,7 +1080,7 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
 	 */
 	memset(&header, 0, sizeof(header));
 	header.xfeatures = xsave->header.xfeatures;
-	header.xfeatures &= ~XFEATURE_MASK_SUPERVISOR;
+	header.xfeatures &= SUPPORTED_XFEATURES_MASK_USER;
 
 	/*
 	 * Copy xregs_state->header:
@@ -1173,7 +1173,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)
 	 * The state that came in from userspace was user-state only.
 	 * Mask all the user states out of 'xfeatures':
 	 */
-	xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR;
+	xsave->header.xfeatures &= ALL_XFEATURES_MASK_SUPERVISOR;
 
 	/*
 	 * Add back in the features that came in from userspace:
@@ -1229,7 +1229,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 	 * The state that came in from userspace was user-state only.
 	 * Mask all the user states out of 'xfeatures':
 	 */
-	xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR;
+	xsave->header.xfeatures &= ALL_XFEATURES_MASK_SUPERVISOR;
 
 	/*
 	 * Add back in the features that came in from userspace:
-- 
2.21.0

Re: [PATCH v2 1/8] x86/fpu/xstate: Define new macros for supervisor and user xstates

[Borislav Petkov]

On Tue, Jan 21, 2020 at 12:18:36PM -0800, Yu-cheng Yu wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> XCNTXT_MASK is 'all supported xfeatures' before introducing supervisor
> xstates.  Rename it to SUPPORTED_XFEATURES_MASK_USER to make clear that
> these are user xstates.
> 
> XFEATURE_MASK_SUPERVISOR is replaced with the following:
> - SUPPORTED_XFEATURES_MASK_SUPERVISOR: Currently nothing.  ENQCMD and
>   Control-flow Enforcement Technology (CET) will be introduced in separate
>   series.
> - UNSUPPORTED_XFEATURES_MASK_SUPERVISOR: Currently only Processor Trace.
> - ALL_XFEATURES_MASK_SUPERVISOR: the combination of above.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Co-developed-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/include/asm/fpu/xstate.h | 36 ++++++++++++++++++++-----------
>  arch/x86/kernel/fpu/init.c        |  3 ++-
>  arch/x86/kernel/fpu/xstate.c      | 26 +++++++++++-----------
>  3 files changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
> index c6136d79f8c0..014c386deaa3 100644
> --- a/arch/x86/include/asm/fpu/xstate.h
> +++ b/arch/x86/include/asm/fpu/xstate.h
> @@ -21,19 +21,29 @@
>  #define XSAVE_YMM_SIZE	    256
>  #define XSAVE_YMM_OFFSET    (XSAVE_HDR_SIZE + XSAVE_HDR_OFFSET)
>  
> -/* Supervisor features */
> -#define XFEATURE_MASK_SUPERVISOR (XFEATURE_MASK_PT)
> -
> -/* All currently supported features */
> -#define XCNTXT_MASK		(XFEATURE_MASK_FP | \
> -				 XFEATURE_MASK_SSE | \
> -				 XFEATURE_MASK_YMM | \
> -				 XFEATURE_MASK_OPMASK | \
> -				 XFEATURE_MASK_ZMM_Hi256 | \
> -				 XFEATURE_MASK_Hi16_ZMM	 | \
> -				 XFEATURE_MASK_PKRU | \
> -				 XFEATURE_MASK_BNDREGS | \
> -				 XFEATURE_MASK_BNDCSR)
> +/* All currently supported user features */
> +#define SUPPORTED_XFEATURES_MASK_USER (XFEATURE_MASK_FP | \
> +				       XFEATURE_MASK_SSE | \
> +				       XFEATURE_MASK_YMM | \
> +				       XFEATURE_MASK_OPMASK | \
> +				       XFEATURE_MASK_ZMM_Hi256 | \
> +				       XFEATURE_MASK_Hi16_ZMM	 | \
> +				       XFEATURE_MASK_PKRU | \
> +				       XFEATURE_MASK_BNDREGS | \
> +				       XFEATURE_MASK_BNDCSR)
> +
> +/* All currently supported supervisor features */
> +#define SUPPORTED_XFEATURES_MASK_SUPERVISOR (0)
> +
> +/*
> + * Unsupported supervisor features. When a supervisor feature in this mask is
> + * supported in the future, move it to the supported supervisor feature mask.
> + */
> +#define UNSUPPORTED_XFEATURES_MASK_SUPERVISOR (XFEATURE_MASK_PT)
> +
> +/* All supervisor states including supported and unsupported states. */
> +#define ALL_XFEATURES_MASK_SUPERVISOR (SUPPORTED_XFEATURES_MASK_SUPERVISOR | \
> +				       UNSUPPORTED_XFEATURES_MASK_SUPERVISOR)

So frankly having the namespace prepended in those macros makes it more
readable to me: you know that those masks all belong together if you had
this:

XFEATURE_MASK_SUPERVISOR
XFEATURE_MASK_SUPERVISOR_SUPPORTED
XFEATURE_MASK_SUPERVISOR_UNSUPPORTED
XFEATURE_MASK_SUPERVISOR_ALL
XFEATURE_MASK_USER_SUPPORTED

Now they all begin with different words: "ALL", "UNSUPPORTED",
"SUPPORTED", ... and makes you go and look up the mask to make sure it
is the correct type of mask used.

Even more so if the single feature masks also start with
"XFEATURE_MASK_" so it is only logical to have them all start with
XFEATURE_MASK_ IMO.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/8] x86/fpu/xstate: Define new macros for supervisor and user xstates

[Yu-cheng Yu]

On Thu, 2020-02-20 at 12:47 +0100, Borislav Petkov wrote:
> On Tue, Jan 21, 2020 at 12:18:36PM -0800, Yu-cheng Yu wrote:
> > From: Fenghua Yu <fenghua.yu@intel.com>
> > [...]
> > +/* All currently supported supervisor features */
> > +#define SUPPORTED_XFEATURES_MASK_SUPERVISOR (0)
> > +
> > +/*
> > + * Unsupported supervisor features. When a supervisor feature in this mask is
> > + * supported in the future, move it to the supported supervisor feature mask.
> > + */
> > +#define UNSUPPORTED_XFEATURES_MASK_SUPERVISOR (XFEATURE_MASK_PT)
> > +
> > +/* All supervisor states including supported and unsupported states. */
> > +#define ALL_XFEATURES_MASK_SUPERVISOR (SUPPORTED_XFEATURES_MASK_SUPERVISOR | \
> > +				       UNSUPPORTED_XFEATURES_MASK_SUPERVISOR)
> 
> So frankly having the namespace prepended in those macros makes it more
> readable to me: you know that those masks all belong together if you had
> this:
> 
> XFEATURE_MASK_SUPERVISOR
> XFEATURE_MASK_SUPERVISOR_SUPPORTED
> XFEATURE_MASK_SUPERVISOR_UNSUPPORTED
> XFEATURE_MASK_SUPERVISOR_ALL
> XFEATURE_MASK_USER_SUPPORTED
> 
> Now they all begin with different words: "ALL", "UNSUPPORTED",
> "SUPPORTED", ... and makes you go and look up the mask to make sure it
> is the correct type of mask used.
> 
> Even more so if the single feature masks also start with
> "XFEATURE_MASK_" so it is only logical to have them all start with
> XFEATURE_MASK_ IMO.

I will make the changes.

Yu-cheng

[PATCH v2 2/8] x86/fpu/xstate: Separate user and supervisor xfeatures mask

[Yu-cheng Yu]

Before the introduction of XSAVES supervisor states, 'xfeatures_mask' is
used at various places to determine XSAVE buffer components and XCR0 bits.
It contains only user xstates.  To support supervisor xstates, it is
necessary to separate user and supervisor xstates:

- First, change 'xfeatures_mask' to 'xfeatures_mask_all', which represents
  the full set of bits that should ever be set in a kernel XSAVE buffer.
- Introduce xfeatures_mask_supervisor() and xfeatures_mask_user() to
  extract relevant xfeatures from xfeatures_mask_all.

v2:
- Fix typo in commit log.
- Move xfeatures_mask_supervisor() from xstate.c to xstate.h.
- Remove printing of user xstates from fpu__init_system_xstate().

Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/fpu/internal.h |  2 +-
 arch/x86/include/asm/fpu/xstate.h   | 13 +++++-
 arch/x86/kernel/fpu/signal.c        | 15 ++++---
 arch/x86/kernel/fpu/xstate.c        | 66 +++++++++++++++++------------
 4 files changed, 61 insertions(+), 35 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 44c48e34d799..ccb1bb32ad7d 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -92,7 +92,7 @@ static inline void fpstate_init_xstate(struct xregs_state *xsave)
 	 * XRSTORS requires these bits set in xcomp_bv, or it will
 	 * trigger #GP:
 	 */
-	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xfeatures_mask;
+	xsave->header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT | xfeatures_mask_all;
 }
 
 static inline void fpstate_init_fxstate(struct fxregs_state *fx)
diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 014c386deaa3..2d510819e4ec 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -51,7 +51,18 @@
 #define REX_PREFIX
 #endif
 
-extern u64 xfeatures_mask;
+extern u64 xfeatures_mask_all;
+
+static inline u64 xfeatures_mask_supervisor(void)
+{
+	return xfeatures_mask_all & SUPPORTED_XFEATURES_MASK_SUPERVISOR;
+}
+
+static inline u64 xfeatures_mask_user(void)
+{
+	return xfeatures_mask_all & SUPPORTED_XFEATURES_MASK_USER;
+}
+
 extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 
 extern void __init update_regset_xstate_info(unsigned int size,
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 400a05e1c1c5..7c7f3efa3c57 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -254,11 +254,14 @@ static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only)
 {
 	if (use_xsave()) {
 		if (fx_only) {
-			u64 init_bv = xfeatures_mask & ~XFEATURE_MASK_FPSSE;
+			u64 init_bv;
+
+			init_bv = xfeatures_mask_user() & ~XFEATURE_MASK_FPSSE;
 			copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
 			return copy_user_to_fxregs(buf);
 		} else {
-			u64 init_bv = xfeatures_mask & ~xbv;
+			u64 init_bv = xfeatures_mask_user() & ~xbv;
+
 			if (unlikely(init_bv))
 				copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
 			return copy_user_to_xregs(buf, xbv);
@@ -358,7 +361,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 
 
 	if (use_xsave() && !fx_only) {
-		u64 init_bv = xfeatures_mask & ~xfeatures;
+		u64 init_bv = xfeatures_mask_user() & ~xfeatures;
 
 		if (using_compacted_format()) {
 			ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
@@ -389,7 +392,9 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 
 		fpregs_lock();
 		if (use_xsave()) {
-			u64 init_bv = xfeatures_mask & ~XFEATURE_MASK_FPSSE;
+			u64 init_bv;
+
+			init_bv = xfeatures_mask_user() & ~XFEATURE_MASK_FPSSE;
 			copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
 		}
 
@@ -465,7 +470,7 @@ void fpu__init_prepare_fx_sw_frame(void)
 
 	fx_sw_reserved.magic1 = FP_XSTATE_MAGIC1;
 	fx_sw_reserved.extended_size = size;
-	fx_sw_reserved.xfeatures = xfeatures_mask;
+	fx_sw_reserved.xfeatures = xfeatures_mask_user();
 	fx_sw_reserved.xstate_size = fpu_user_xstate_size;
 
 	if (IS_ENABLED(CONFIG_IA32_EMULATION) ||
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 7d8e9414efa4..3b22d630f5bb 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -54,9 +54,10 @@ static short xsave_cpuid_features[] __initdata = {
 };
 
 /*
- * Mask of xstate features supported by the CPU and the kernel:
+ * This represents the full set of bits that should ever be set in a kernel
+ * XSAVE buffer, both supervisor and user xstates.
  */
-u64 xfeatures_mask __read_mostly;
+u64 xfeatures_mask_all __read_mostly;
 
 static unsigned int xstate_offsets[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] = -1};
 static unsigned int xstate_sizes[XFEATURE_MAX]   = { [ 0 ... XFEATURE_MAX - 1] = -1};
@@ -76,7 +77,7 @@ unsigned int fpu_user_xstate_size;
  */
 int cpu_has_xfeatures(u64 xfeatures_needed, const char **feature_name)
 {
-	u64 xfeatures_missing = xfeatures_needed & ~xfeatures_mask;
+	u64 xfeatures_missing = xfeatures_needed & ~xfeatures_mask_all;
 
 	if (unlikely(feature_name)) {
 		long xfeature_idx, max_idx;
@@ -155,7 +156,7 @@ void fpstate_sanitize_xstate(struct fpu *fpu)
 	 * None of the feature bits are in init state. So nothing else
 	 * to do for us, as the memory layout is up to date.
 	 */
-	if ((xfeatures & xfeatures_mask) == xfeatures_mask)
+	if ((xfeatures & xfeatures_mask_all) == xfeatures_mask_all)
 		return;
 
 	/*
@@ -182,7 +183,7 @@ void fpstate_sanitize_xstate(struct fpu *fpu)
 	 * in a special way already:
 	 */
 	feature_bit = 0x2;
-	xfeatures = (xfeatures_mask & ~xfeatures) >> 2;
+	xfeatures = (xfeatures_mask_user() & ~xfeatures) >> 2;
 
 	/*
 	 * Update all the remaining memory layouts according to their
@@ -210,19 +211,24 @@ void fpstate_sanitize_xstate(struct fpu *fpu)
  */
 void fpu__init_cpu_xstate(void)
 {
-	if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask)
+	if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_all)
 		return;
 	/*
 	 * Unsupported supervisor xstates should not be found in
 	 * the xfeatures mask.
 	 */
-	WARN_ONCE((xfeatures_mask & UNSUPPORTED_XFEATURES_MASK_SUPERVISOR),
+	WARN_ONCE((xfeatures_mask_all & UNSUPPORTED_XFEATURES_MASK_SUPERVISOR),
 		  "x86/fpu: Found unsupported supervisor xstates.\n");
 
-	xfeatures_mask &= ~UNSUPPORTED_XFEATURES_MASK_SUPERVISOR;
+	xfeatures_mask_all &= ~UNSUPPORTED_XFEATURES_MASK_SUPERVISOR;
 
 	cr4_set_bits(X86_CR4_OSXSAVE);
-	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
+	/*
+	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
+	 * managed by XSAVE{C, OPT, S} and XRSTOR{S}.  Only XSAVE user
+	 * states can be set here.
+	 */
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user());
 }
 
 /*
@@ -232,7 +238,7 @@ void fpu__init_cpu_xstate(void)
  */
 static int xfeature_enabled(enum xfeature xfeature)
 {
-	return !!(xfeatures_mask & (1UL << xfeature));
+	return !!(xfeatures_mask_all & (1UL << xfeature));
 }
 
 /*
@@ -419,7 +425,7 @@ static void __init setup_init_fpu_buf(void)
 
 	if (boot_cpu_has(X86_FEATURE_XSAVES))
 		init_fpstate.xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
-						     xfeatures_mask;
+						     xfeatures_mask_all;
 
 	/*
 	 * Init all the features state with header.xfeatures being 0x0
@@ -479,7 +485,7 @@ int using_compacted_format(void)
 int validate_xstate_header(const struct xstate_header *hdr)
 {
 	/* No unknown or supervisor features may be set */
-	if (hdr->xfeatures & ~(xfeatures_mask & SUPPORTED_XFEATURES_MASK_USER))
+	if (hdr->xfeatures & ~xfeatures_mask_user())
 		return -EINVAL;
 
 	/* Userspace must use the uncompacted format */
@@ -614,7 +620,7 @@ static void do_extra_xstate_size_checks(void)
 
 
 /*
- * Get total size of enabled xstates in XCR0/xfeatures_mask.
+ * Get total size of enabled xstates in XCR0 | IA32_XSS.
  *
  * Note the SDM's wording here.  "sub-function 0" only enumerates
  * the size of the *user* states.  If we use it to size a buffer
@@ -704,7 +710,7 @@ static int __init init_xstate_size(void)
  */
 static void fpu__init_disable_system_xstate(void)
 {
-	xfeatures_mask = 0;
+	xfeatures_mask_all = 0;
 	cr4_clear_bits(X86_CR4_OSXSAVE);
 	setup_clear_cpu_cap(X86_FEATURE_XSAVE);
 }
@@ -739,16 +745,22 @@ void __init fpu__init_system_xstate(void)
 		return;
 	}
 
+	/*
+	 * Find user xstates supported by the processor.
+	 */
 	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
-	xfeatures_mask = eax + ((u64)edx << 32);
+	xfeatures_mask_all = eax + ((u64)edx << 32);
+
+	/* Place supervisor features in xfeatures_mask_all here */
 
-	if ((xfeatures_mask & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) {
+	if ((xfeatures_mask_user() & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) {
 		/*
 		 * This indicates that something really unexpected happened
 		 * with the enumeration.  Disable XSAVE and try to continue
 		 * booting without it.  This is too early to BUG().
 		 */
-		pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n", xfeatures_mask);
+		pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n",
+		       xfeatures_mask_all);
 		goto out_disable;
 	}
 
@@ -757,10 +769,10 @@ void __init fpu__init_system_xstate(void)
 	 */
 	for (i = 0; i < ARRAY_SIZE(xsave_cpuid_features); i++) {
 		if (!boot_cpu_has(xsave_cpuid_features[i]))
-			xfeatures_mask &= ~BIT(i);
+			xfeatures_mask_all &= ~BIT_ULL(i);
 	}
 
-	xfeatures_mask &= fpu__get_supported_xfeatures_mask();
+	xfeatures_mask_all &= fpu__get_supported_xfeatures_mask();
 
 	/* Enable xstate instructions to be able to continue with initialization: */
 	fpu__init_cpu_xstate();
@@ -772,8 +784,7 @@ void __init fpu__init_system_xstate(void)
 	 * Update info used for ptrace frames; use standard-format size and no
 	 * supervisor xstates:
 	 */
-	update_regset_xstate_info(fpu_user_xstate_size,
-				  xfeatures_mask & SUPPORTED_XFEATURES_MASK_USER);
+	update_regset_xstate_info(fpu_user_xstate_size, xfeatures_mask_user());
 
 	fpu__init_prepare_fx_sw_frame();
 	setup_init_fpu_buf();
@@ -781,7 +792,7 @@ void __init fpu__init_system_xstate(void)
 	print_xstate_offset_size();
 
 	pr_info("x86/fpu: Enabled xstate features 0x%llx, context size is %d bytes, using '%s' format.\n",
-		xfeatures_mask,
+		xfeatures_mask_all,
 		fpu_kernel_xstate_size,
 		boot_cpu_has(X86_FEATURE_XSAVES) ? "compacted" : "standard");
 	return;
@@ -800,7 +811,7 @@ void fpu__resume_cpu(void)
 	 * Restore XCR0 on xsave capable CPUs:
 	 */
 	if (boot_cpu_has(X86_FEATURE_XSAVE))
-		xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
+		xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user());
 }
 
 /*
@@ -845,10 +856,9 @@ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
 
 	/*
 	 * We should not ever be requesting features that we
-	 * have not enabled.  Remember that xfeatures_mask is
-	 * what we write to the XCR0 register.
+	 * have not enabled.
 	 */
-	WARN_ONCE(!(xfeatures_mask & BIT_ULL(xfeature_nr)),
+	WARN_ONCE(!(xfeatures_mask_all & BIT_ULL(xfeature_nr)),
 		  "get of unsupported state");
 	/*
 	 * This assumes the last 'xsave*' instruction to
@@ -996,7 +1006,7 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
 	 */
 	memset(&header, 0, sizeof(header));
 	header.xfeatures = xsave->header.xfeatures;
-	header.xfeatures &= SUPPORTED_XFEATURES_MASK_USER;
+	header.xfeatures &= xfeatures_mask_user();
 
 	/*
 	 * Copy xregs_state->header:
@@ -1080,7 +1090,7 @@ int copy_xstate_to_user(void __user *ubuf, struct xregs_state *xsave, unsigned i
 	 */
 	memset(&header, 0, sizeof(header));
 	header.xfeatures = xsave->header.xfeatures;
-	header.xfeatures &= SUPPORTED_XFEATURES_MASK_USER;
+	header.xfeatures &= xfeatures_mask_user();
 
 	/*
 	 * Copy xregs_state->header:
-- 
2.21.0

Re: [PATCH v2 2/8] x86/fpu/xstate: Separate user and supervisor xfeatures mask

[Borislav Petkov]

On Tue, Jan 21, 2020 at 12:18:37PM -0800, Yu-cheng Yu wrote:
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 400a05e1c1c5..7c7f3efa3c57 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -254,11 +254,14 @@ static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only)
>  {
>  	if (use_xsave()) {
>  		if (fx_only) {
> -			u64 init_bv = xfeatures_mask & ~XFEATURE_MASK_FPSSE;
> +			u64 init_bv;
> +
> +			init_bv = xfeatures_mask_user() & ~XFEATURE_MASK_FPSSE;
>  			copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
>  			return copy_user_to_fxregs(buf);
>  		} else {
> -			u64 init_bv = xfeatures_mask & ~xbv;
> +			u64 init_bv = xfeatures_mask_user() & ~xbv;
> +
>  			if (unlikely(init_bv))
>  				copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
>  			return copy_user_to_xregs(buf, xbv);

If you don't want to overflow 80 cols here in the fx_only() case, you can do:

---
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 7c7f3efa3c57..d7e94677cd31 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -252,15 +252,16 @@ sanitize_restored_xstate(union fpregs_state *state,
  */
 static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only)
 {
+	u64 init_bv;
+
 	if (use_xsave()) {
 		if (fx_only) {
-			u64 init_bv;
-
 			init_bv = xfeatures_mask_user() & ~XFEATURE_MASK_FPSSE;
+
 			copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
 			return copy_user_to_fxregs(buf);
 		} else {
-			u64 init_bv = xfeatures_mask_user() & ~xbv;
+			init_bv = xfeatures_mask_user() & ~xbv;
 
 			if (unlikely(init_bv))
 				copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);

...

> @@ -210,19 +211,24 @@ void fpstate_sanitize_xstate(struct fpu *fpu)
>   */
>  void fpu__init_cpu_xstate(void)
>  {
> -	if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask)
> +	if (!boot_cpu_has(X86_FEATURE_XSAVE) || !xfeatures_mask_all)
>  		return;
>  	/*
>  	 * Unsupported supervisor xstates should not be found in
>  	 * the xfeatures mask.
>  	 */
> -	WARN_ONCE((xfeatures_mask & UNSUPPORTED_XFEATURES_MASK_SUPERVISOR),
> +	WARN_ONCE((xfeatures_mask_all & UNSUPPORTED_XFEATURES_MASK_SUPERVISOR),
>  		  "x86/fpu: Found unsupported supervisor xstates.\n");

Let's say which to ease debugging:

	u64 unsup_bits = xfeatures_mask_all & UNSUPPORTED_XFEATURES_MASK_SUPERVISOR;

	...

	WARN_ONCE(unsup_bits, "x86/fpu: Found unsupported supervisor xstates: 0x%llx\n",
		  unsup_bits);


>  
> -	xfeatures_mask &= ~UNSUPPORTED_XFEATURES_MASK_SUPERVISOR;
> +	xfeatures_mask_all &= ~UNSUPPORTED_XFEATURES_MASK_SUPERVISOR;
>  
>  	cr4_set_bits(X86_CR4_OSXSAVE);
> -	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);

<---- newline here.

> +	/*
> +	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
> +	 * managed by XSAVE{C, OPT, S} and XRSTOR{S}.  Only XSAVE user
> +	 * states can be set here.
> +	 */
> +	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user());
>  }
>  
>  /*
> @@ -232,7 +238,7 @@ void fpu__init_cpu_xstate(void)
>   */
>  static int xfeature_enabled(enum xfeature xfeature)

static bool

>  {
> -	return !!(xfeatures_mask & (1UL << xfeature));
> +	return !!(xfeatures_mask_all & (1UL << xfeature));

	return xfeatures_mask_all & BIT(xfeature);

while at it.

>  }
>  
>  /*
> @@ -419,7 +425,7 @@ static void __init setup_init_fpu_buf(void)
>  
>  	if (boot_cpu_has(X86_FEATURE_XSAVES))
>  		init_fpstate.xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
> -						     xfeatures_mask;
> +						     xfeatures_mask_all;
>  
>  	/*
>  	 * Init all the features state with header.xfeatures being 0x0
> @@ -479,7 +485,7 @@ int using_compacted_format(void)
>  int validate_xstate_header(const struct xstate_header *hdr)
>  {
>  	/* No unknown or supervisor features may be set */
> -	if (hdr->xfeatures & ~(xfeatures_mask & SUPPORTED_XFEATURES_MASK_USER))
> +	if (hdr->xfeatures & ~xfeatures_mask_user())
>  		return -EINVAL;
>  
>  	/* Userspace must use the uncompacted format */
> @@ -614,7 +620,7 @@ static void do_extra_xstate_size_checks(void)
>  
>  
>  /*
> - * Get total size of enabled xstates in XCR0/xfeatures_mask.
> + * Get total size of enabled xstates in XCR0 | IA32_XSS.
>   *
>   * Note the SDM's wording here.  "sub-function 0" only enumerates
>   * the size of the *user* states.  If we use it to size a buffer
> @@ -704,7 +710,7 @@ static int __init init_xstate_size(void)
>   */
>  static void fpu__init_disable_system_xstate(void)
>  {
> -	xfeatures_mask = 0;
> +	xfeatures_mask_all = 0;
>  	cr4_clear_bits(X86_CR4_OSXSAVE);
>  	setup_clear_cpu_cap(X86_FEATURE_XSAVE);
>  }
> @@ -739,16 +745,22 @@ void __init fpu__init_system_xstate(void)
>  		return;
>  	}
>  
> +	/*
> +	 * Find user xstates supported by the processor.
> +	 */
>  	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
> -	xfeatures_mask = eax + ((u64)edx << 32);
> +	xfeatures_mask_all = eax + ((u64)edx << 32);
> +
> +	/* Place supervisor features in xfeatures_mask_all here */
>  

Superfluous newline.

> -	if ((xfeatures_mask & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) {
> +	if ((xfeatures_mask_user() & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) {
>  		/*
>  		 * This indicates that something really unexpected happened
>  		 * with the enumeration.  Disable XSAVE and try to continue
>  		 * booting without it.  This is too early to BUG().
>  		 */
> -		pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n", xfeatures_mask);
> +		pr_err("x86/fpu: FP/SSE not present amongst the CPU's xstate features: 0x%llx.\n",
> +		       xfeatures_mask_all);
>  		goto out_disable;
>  	}

...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v2 3/8] x86/fpu/xstate: Introduce XSAVES supervisor states

[Yu-cheng Yu]

Enable XSAVES supervisor states by setting MSR_IA32_XSS bits according to
CPUID enumeration results.  Also revise comments at various places.

v2:
- Remove printing of supervisor xstates from fpu__init_system_xstate().

Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 3b22d630f5bb..0c5c91ee235e 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -229,13 +229,14 @@ void fpu__init_cpu_xstate(void)
 	 * states can be set here.
 	 */
 	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user());
+
+	/*
+	 * MSR_IA32_XSS sets supervisor states managed by XSAVES.
+	 */
+	if (boot_cpu_has(X86_FEATURE_XSAVES))
+		wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());
 }
 
-/*
- * Note that in the future we will likely need a pair of
- * functions here: one for user xstates and the other for
- * system xstates.  For now, they are the same.
- */
 static int xfeature_enabled(enum xfeature xfeature)
 {
 	return !!(xfeatures_mask_all & (1UL << xfeature));
@@ -626,9 +627,6 @@ static void do_extra_xstate_size_checks(void)
  * the size of the *user* states.  If we use it to size a buffer
  * that we use 'XSAVES' on, we could potentially overflow the
  * buffer because 'XSAVES' saves system states too.
- *
- * Note that we do not currently set any bits on IA32_XSS so
- * 'XCR0 | IA32_XSS == XCR0' for now.
  */
 static unsigned int __init get_xsaves_size(void)
 {
@@ -751,7 +749,11 @@ void __init fpu__init_system_xstate(void)
 	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
 	xfeatures_mask_all = eax + ((u64)edx << 32);
 
-	/* Place supervisor features in xfeatures_mask_all here */
+	/*
+	 * Find supervisor xstates supported by the processor.
+	 */
+	cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
+	xfeatures_mask_all |= ecx + ((u64)edx << 32);
 
 	if ((xfeatures_mask_user() & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) {
 		/*
@@ -812,6 +814,13 @@ void fpu__resume_cpu(void)
 	 */
 	if (boot_cpu_has(X86_FEATURE_XSAVE))
 		xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask_user());
+
+	/*
+	 * Restore IA32_XSS. The same CPUID bit enumerates support
+	 * of XSAVES and MSR_IA32_XSS.
+	 */
+	if (boot_cpu_has(X86_FEATURE_XSAVES))
+		wrmsrl(MSR_IA32_XSS, xfeatures_mask_supervisor());
 }
 
 /*
-- 
2.21.0

[PATCH v2 4/8] x86/fpu/xstate: Define new functions for clearing fpregs and xstates

[Yu-cheng Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

Currently, fpu__clear() clears all fpregs and xstates.  Once XSAVES
supervisor states are introduced, supervisor settings (e.g. CET xstates)
must remain active for signals; It is necessary to have separate functions:

- Create fpu__clear_user_states(): clear only user settings for signals;
- Create fpu__clear_all(): clear both user and supervisor settings in
   flush_thread().

Also modify copy_init_fpstate_to_fpregs() to take a mask from above two
functions.

v2:
- Fixed an issue where fpu__clear_user_states() drops supervisor xstates.
- Revise commit log.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Co-developed-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/fpu/internal.h |  3 ++-
 arch/x86/kernel/fpu/core.c          | 41 +++++++++++++++++++++--------
 arch/x86/kernel/fpu/signal.c        |  4 +--
 arch/x86/kernel/process.c           |  2 +-
 arch/x86/kernel/signal.c            |  2 +-
 5 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index ccb1bb32ad7d..a42fcb4b690d 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -31,7 +31,8 @@ extern void fpu__save(struct fpu *fpu);
 extern int  fpu__restore_sig(void __user *buf, int ia32_frame);
 extern void fpu__drop(struct fpu *fpu);
 extern int  fpu__copy(struct task_struct *dst, struct task_struct *src);
-extern void fpu__clear(struct fpu *fpu);
+extern void fpu__clear_user_states(struct fpu *fpu);
+extern void fpu__clear_all(struct fpu *fpu);
 extern int  fpu__exception_code(struct fpu *fpu, int trap_nr);
 extern int  dump_fpu(struct pt_regs *ptregs, struct user_i387_struct *fpstate);
 
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 12c70840980e..176cf62ab757 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -294,12 +294,10 @@ void fpu__drop(struct fpu *fpu)
  * Clear FPU registers by setting them up from
  * the init fpstate:
  */
-static inline void copy_init_fpstate_to_fpregs(void)
+static inline void copy_init_fpstate_to_fpregs(u64 features_mask)
 {
-	fpregs_lock();
-
 	if (use_xsave())
-		copy_kernel_to_xregs(&init_fpstate.xsave, -1);
+		copy_kernel_to_xregs(&init_fpstate.xsave, features_mask);
 	else if (static_cpu_has(X86_FEATURE_FXSR))
 		copy_kernel_to_fxregs(&init_fpstate.fxsave);
 	else
@@ -307,9 +305,6 @@ static inline void copy_init_fpstate_to_fpregs(void)
 
 	if (boot_cpu_has(X86_FEATURE_OSPKE))
 		copy_init_pkru_to_fpregs();
-
-	fpregs_mark_activate();
-	fpregs_unlock();
 }
 
 /*
@@ -318,9 +313,29 @@ static inline void copy_init_fpstate_to_fpregs(void)
  * Called by sys_execve(), by the signal handler code and by various
  * error paths.
  */
-void fpu__clear(struct fpu *fpu)
+void fpu__clear_user_states(struct fpu *fpu)
+{
+	WARN_ON_FPU(fpu != &current->thread.fpu);
+
+	if (static_cpu_has(X86_FEATURE_FPU)) {
+		fpregs_lock();
+		if (!fpregs_state_valid(fpu, smp_processor_id()) &&
+		    xfeatures_mask_supervisor())
+			copy_kernel_to_xregs(&fpu->state.xsave,
+					     xfeatures_mask_supervisor());
+		copy_init_fpstate_to_fpregs(xfeatures_mask_user());
+		fpregs_mark_activate();
+		fpregs_unlock();
+		return;
+	} else {
+		fpu__drop(fpu);
+		fpu__initialize(fpu);
+	}
+}
+
+void fpu__clear_all(struct fpu *fpu)
 {
-	WARN_ON_FPU(fpu != &current->thread.fpu); /* Almost certainly an anomaly */
+	WARN_ON_FPU(fpu != &current->thread.fpu);
 
 	fpu__drop(fpu);
 
@@ -328,8 +343,12 @@ void fpu__clear(struct fpu *fpu)
 	 * Make sure fpstate is cleared and initialized.
 	 */
 	fpu__initialize(fpu);
-	if (static_cpu_has(X86_FEATURE_FPU))
-		copy_init_fpstate_to_fpregs();
+	if (static_cpu_has(X86_FEATURE_FPU)) {
+		fpregs_lock();
+		copy_init_fpstate_to_fpregs(xfeatures_mask_all);
+		fpregs_mark_activate();
+		fpregs_unlock();
+	}
 }
 
 /*
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 7c7f3efa3c57..98c970420da6 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -288,7 +288,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 			 IS_ENABLED(CONFIG_IA32_EMULATION));
 
 	if (!buf) {
-		fpu__clear(fpu);
+		fpu__clear_user_states(fpu);
 		return 0;
 	}
 
@@ -415,7 +415,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 
 err_out:
 	if (ret)
-		fpu__clear(fpu);
+		fpu__clear_user_states(fpu);
 	return ret;
 }
 
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 61e93a318983..8d0b9442202e 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -192,7 +192,7 @@ void flush_thread(void)
 	flush_ptrace_hw_breakpoint(tsk);
 	memset(tsk->thread.tls_array, 0, sizeof(tsk->thread.tls_array));
 
-	fpu__clear(&tsk->thread.fpu);
+	fpu__clear_all(&tsk->thread.fpu);
 }
 
 void disable_TSC(void)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 8eb7193e158d..ce9421ec285f 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -763,7 +763,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		/*
 		 * Ensure the signal handler starts with the new fpu state.
 		 */
-		fpu__clear(fpu);
+		fpu__clear_user_states(fpu);
 	}
 	signal_setup_done(failed, ksig, stepping);
 }
-- 
2.21.0

Re: [PATCH v2 4/8] x86/fpu/xstate: Define new functions for clearing fpregs and xstates

[Borislav Petkov]

On Tue, Jan 21, 2020 at 12:18:39PM -0800, Yu-cheng Yu wrote:
> @@ -318,9 +313,29 @@ static inline void copy_init_fpstate_to_fpregs(void)
>   * Called by sys_execve(), by the signal handler code and by various
>   * error paths.
>   */
> -void fpu__clear(struct fpu *fpu)
> +void fpu__clear_user_states(struct fpu *fpu)
> +{
> +	WARN_ON_FPU(fpu != &current->thread.fpu);
> +
> +	if (static_cpu_has(X86_FEATURE_FPU)) {
> +		fpregs_lock();
> +		if (!fpregs_state_valid(fpu, smp_processor_id()) &&
> +		    xfeatures_mask_supervisor())
> +			copy_kernel_to_xregs(&fpu->state.xsave,
> +					     xfeatures_mask_supervisor());
> +		copy_init_fpstate_to_fpregs(xfeatures_mask_user());
> +		fpregs_mark_activate();
> +		fpregs_unlock();
> +		return;
> +	} else {
> +		fpu__drop(fpu);
> +		fpu__initialize(fpu);
> +	}
> +}
> +
> +void fpu__clear_all(struct fpu *fpu)
>  {
> -	WARN_ON_FPU(fpu != &current->thread.fpu); /* Almost certainly an anomaly */
> +	WARN_ON_FPU(fpu != &current->thread.fpu);
>  
>  	fpu__drop(fpu);
>  
> @@ -328,8 +343,12 @@ void fpu__clear(struct fpu *fpu)
>  	 * Make sure fpstate is cleared and initialized.
>  	 */
>  	fpu__initialize(fpu);
> -	if (static_cpu_has(X86_FEATURE_FPU))
> -		copy_init_fpstate_to_fpregs();
> +	if (static_cpu_has(X86_FEATURE_FPU)) {
> +		fpregs_lock();
> +		copy_init_fpstate_to_fpregs(xfeatures_mask_all);
> +		fpregs_mark_activate();
> +		fpregs_unlock();
> +	}
>  }

Why do you need two different functions which are pretty similar if you
can do

fpu__clear(struct fpu *fpu, bool user_only)
{
	...

and query that user_only variable in the fpu__clear() body to do the
respective work dependent on the its setting?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v2 5/8] x86/fpu/xstate: Rename validate_xstate_header() to validate_xstate_header_from_user()

[Yu-cheng Yu]

From: Fenghua Yu <fenghua.yu@intel.com>

The function validate_xstate_header() validates an xstate header coming
from userspace (PTRACE or sigreturn).  To make it clear, rename it to
validate_xstate_header_from_user().

Suggested-by: Dave Hansen <dave.hansen@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/fpu/xstate.h | 2 +-
 arch/x86/kernel/fpu/regset.c      | 2 +-
 arch/x86/kernel/fpu/signal.c      | 2 +-
 arch/x86/kernel/fpu/xstate.c      | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/fpu/xstate.h b/arch/x86/include/asm/fpu/xstate.h
index 2d510819e4ec..9ebfdd543576 100644
--- a/arch/x86/include/asm/fpu/xstate.h
+++ b/arch/x86/include/asm/fpu/xstate.h
@@ -77,6 +77,6 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf);
 int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf);
 
 /* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
-extern int validate_xstate_header(const struct xstate_header *hdr);
+int validate_xstate_header_from_user(const struct xstate_header *hdr);
 
 #endif
diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index d652b939ccfb..9789f95cfb66 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -139,7 +139,7 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
 	} else {
 		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, xsave, 0, -1);
 		if (!ret)
-			ret = validate_xstate_header(&xsave->header);
+			ret = validate_xstate_header_from_user(&xsave->header);
 	}
 
 	/*
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 98c970420da6..4afe61987e03 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -369,7 +369,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 			ret = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
 
 			if (!ret && state_size > offsetof(struct xregs_state, header))
-				ret = validate_xstate_header(&fpu->state.xsave.header);
+				ret = validate_xstate_header_from_user(&fpu->state.xsave.header);
 		}
 		if (ret)
 			goto err_out;
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 0c5c91ee235e..04f7c6b8dbbc 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -483,7 +483,7 @@ int using_compacted_format(void)
 }
 
 /* Validate an xstate header supplied by userspace (ptrace or sigreturn) */
-int validate_xstate_header(const struct xstate_header *hdr)
+int validate_xstate_header_from_user(const struct xstate_header *hdr)
 {
 	/* No unknown or supervisor features may be set */
 	if (hdr->xfeatures & ~xfeatures_mask_user())
@@ -1166,7 +1166,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf)
 
 	memcpy(&hdr, kbuf + offset, size);
 
-	if (validate_xstate_header(&hdr))
+	if (validate_xstate_header_from_user(&hdr))
 		return -EINVAL;
 
 	for (i = 0; i < XFEATURE_MAX; i++) {
@@ -1220,7 +1220,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 	if (__copy_from_user(&hdr, ubuf + offset, size))
 		return -EFAULT;
 
-	if (validate_xstate_header(&hdr))
+	if (validate_xstate_header_from_user(&hdr))
 		return -EINVAL;
 
 	for (i = 0; i < XFEATURE_MAX; i++) {
-- 
2.21.0

Re: [PATCH v2 5/8] x86/fpu/xstate: Rename validate_xstate_header() to validate_xstate_header_from_user()

[Borislav Petkov]

On Tue, Jan 21, 2020 at 12:18:40PM -0800, Yu-cheng Yu wrote:
> From: Fenghua Yu <fenghua.yu@intel.com>
> 
> The function validate_xstate_header() validates an xstate header coming
> from userspace (PTRACE or sigreturn).  To make it clear, rename it to
> validate_xstate_header_from_user().

Or simply:

validate_user_xstate_header()

Also, make that patch the first in your series.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v2 6/8] x86/fpu/xstate: Update sanitize_restored_xstate() for supervisor xstates

[Yu-cheng Yu]

The function sanitize_restored_xstate() sanitizes user xstates of an XSAVE
buffer by setting the buffer's header->xfeatures to the input 'xfeatures',
effectively resetting features not in 'xfeatures' back to the init state.

When supervisor xstates are introduced, it is necessary to make sure only
user xstates are sanitized.  This patch ensures supervisor xstates are not
changed by ensuring supervisor bits stay set in header->xfeatures.

To make names clear, also:

- Rename the function to sanitize_restored_user_xstate().
- Rename input parameter 'xfeatures' to 'xfeatures_from_user'.
- In __fpu__restore_sig(), rename 'xfeatures' to 'xfeatures_user'.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/kernel/fpu/signal.c | 37 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 4afe61987e03..e3781a4a52a8 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -211,9 +211,9 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 }
 
 static inline void
-sanitize_restored_xstate(union fpregs_state *state,
-			 struct user_i387_ia32_struct *ia32_env,
-			 u64 xfeatures, int fx_only)
+sanitize_restored_user_xstate(union fpregs_state *state,
+			      struct user_i387_ia32_struct *ia32_env,
+			      u64 xfeatures_from_user, int fx_only)
 {
 	struct xregs_state *xsave = &state->xsave;
 	struct xstate_header *header = &xsave->header;
@@ -226,13 +226,22 @@ sanitize_restored_xstate(union fpregs_state *state,
 		 */
 
 		/*
-		 * Init the state that is not present in the memory
-		 * layout and not enabled by the OS.
+		 * 'xfeatures_from_user' might have bits clear which are
+		 * set in header->xfeatures. This represents features that
+		 * were in init state prior to a signal delivery, and need
+		 * to be reset back to the init state.  Clear any user
+		 * feature bits which are set in the kernel buffer to get
+		 * them back to the init state.
+		 *
+		 * Supervisor state is unchanged by input from userspace.
+		 * Ensure that supervisor state is not modified by ensuring
+		 * supervisor state bits stay set.
 		 */
 		if (fx_only)
 			header->xfeatures = XFEATURE_MASK_FPSSE;
 		else
-			header->xfeatures &= xfeatures;
+			header->xfeatures &= xfeatures_from_user |
+					     xfeatures_mask_supervisor();
 	}
 
 	if (use_fxsr()) {
@@ -280,7 +289,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 	struct task_struct *tsk = current;
 	struct fpu *fpu = &tsk->thread.fpu;
 	struct user_i387_ia32_struct env;
-	u64 xfeatures = 0;
+	u64 xfeatures_user = 0;
 	int fx_only = 0;
 	int ret = 0;
 
@@ -313,7 +322,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 			trace_x86_fpu_xstate_check_failed(fpu);
 		} else {
 			state_size = fx_sw_user.xstate_size;
-			xfeatures = fx_sw_user.xfeatures;
+			xfeatures_user = fx_sw_user.xfeatures;
 		}
 	}
 
@@ -348,7 +357,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		 */
 		fpregs_lock();
 		pagefault_disable();
-		ret = copy_user_to_fpregs_zeroing(buf_fx, xfeatures, fx_only);
+		ret = copy_user_to_fpregs_zeroing(buf_fx, xfeatures_user, fx_only);
 		pagefault_enable();
 		if (!ret) {
 			fpregs_mark_activate();
@@ -361,7 +370,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 
 
 	if (use_xsave() && !fx_only) {
-		u64 init_bv = xfeatures_mask_user() & ~xfeatures;
+		u64 init_bv = xfeatures_mask_user() & ~xfeatures_user;
 
 		if (using_compacted_format()) {
 			ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
@@ -374,12 +383,13 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		if (ret)
 			goto err_out;
 
-		sanitize_restored_xstate(&fpu->state, envp, xfeatures, fx_only);
+		sanitize_restored_user_xstate(&fpu->state, envp, xfeatures_user,
+					      fx_only);
 
 		fpregs_lock();
 		if (unlikely(init_bv))
 			copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
-		ret = copy_kernel_to_xregs_err(&fpu->state.xsave, xfeatures);
+		ret = copy_kernel_to_xregs_err(&fpu->state.xsave, xfeatures_user);
 
 	} else if (use_fxsr()) {
 		ret = __copy_from_user(&fpu->state.fxsave, buf_fx, state_size);
@@ -388,7 +398,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 			goto err_out;
 		}
 
-		sanitize_restored_xstate(&fpu->state, envp, xfeatures, fx_only);
+		sanitize_restored_user_xstate(&fpu->state, envp,
+					      xfeatures_user, fx_only);
 
 		fpregs_lock();
 		if (use_xsave()) {
-- 
2.21.0

Re: [PATCH v2 6/8] x86/fpu/xstate: Update sanitize_restored_xstate() for supervisor xstates

[Borislav Petkov]

On Tue, Jan 21, 2020 at 12:18:41PM -0800, Yu-cheng Yu wrote:
> The function sanitize_restored_xstate() sanitizes user xstates of an XSAVE
> buffer by setting the buffer's header->xfeatures to the input 'xfeatures',
> effectively resetting features not in 'xfeatures' back to the init state.
> 
> When supervisor xstates are introduced, it is necessary to make sure only
> user xstates are sanitized.  This patch ensures supervisor xstates are not

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> changed by ensuring supervisor bits stay set in header->xfeatures.
> 
> To make names clear, also:
> 
> - Rename the function to sanitize_restored_user_xstate().
> - Rename input parameter 'xfeatures' to 'xfeatures_from_user'.
> - In __fpu__restore_sig(), rename 'xfeatures' to 'xfeatures_user'.

Call them all "user_xfeatures" to differentiate that it is a function
argument and not our xfeature_* macro and function names.

> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
>  arch/x86/kernel/fpu/signal.c | 37 +++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 4afe61987e03..e3781a4a52a8 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -211,9 +211,9 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
>  }
>  
>  static inline void
> -sanitize_restored_xstate(union fpregs_state *state,
> -			 struct user_i387_ia32_struct *ia32_env,
> -			 u64 xfeatures, int fx_only)
> +sanitize_restored_user_xstate(union fpregs_state *state,
> +			      struct user_i387_ia32_struct *ia32_env,
> +			      u64 xfeatures_from_user, int fx_only)
>  {
>  	struct xregs_state *xsave = &state->xsave;
>  	struct xstate_header *header = &xsave->header;
> @@ -226,13 +226,22 @@ sanitize_restored_xstate(union fpregs_state *state,
>  		 */
>  
>  		/*
> -		 * Init the state that is not present in the memory
> -		 * layout and not enabled by the OS.
> +		 * 'xfeatures_from_user' might have bits clear which are
> +		 * set in header->xfeatures. This represents features that
> +		 * were in init state prior to a signal delivery, and need
> +		 * to be reset back to the init state.  Clear any user
> +		 * feature bits which are set in the kernel buffer to get
> +		 * them back to the init state.
> +		 *
> +		 * Supervisor state is unchanged by input from userspace.
> +		 * Ensure that supervisor state is not modified by ensuring
> +		 * supervisor state bits stay set.

"Ensure ... by ensuring ..." Simplify pls.

>  		 */
>  		if (fx_only)
>  			header->xfeatures = XFEATURE_MASK_FPSSE;
>  		else
> -			header->xfeatures &= xfeatures;
> +			header->xfeatures &= xfeatures_from_user |
> +					     xfeatures_mask_supervisor();
>  	}
>  
>  	if (use_fxsr()) {

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH v2 7/8] x86/fpu/xstate: Update copy_kernel_to_xregs_err() for XSAVES supervisor states

[Yu-cheng Yu]

The function copy_kernel_to_xregs_err() uses XRSTOR, which can work with
standard or compacted format without supervisor xstates.  However, when
supervisor xstates are present, XRSTORS must be used.  Fix it by using
XRSTORS when XSAVES is enabled.

I also considered if there were additional cases where XRSTOR might be
mistakenly called instead of XRSTORS.  There are only three XRSTOR sites
in kernel:

1. copy_kernel_to_xregs_booting(), already switches between XRSTOR and
   XRSTORS based on X86_FEATURE_XSAVES.
2. copy_user_to_xregs(), which *needs* XRSTOR because it is copying from
   userspace and must never copy supervisor state with XRSTORS.
3. copy_kernel_to_xregs_err() mistakenly used XRSTOR only.  Fixed in
   this patch.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/include/asm/fpu/internal.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index a42fcb4b690d..42159f45bf9c 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -400,7 +400,10 @@ static inline int copy_kernel_to_xregs_err(struct xregs_state *xstate, u64 mask)
 	u32 hmask = mask >> 32;
 	int err;
 
-	XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
+	if (static_cpu_has(X86_FEATURE_XSAVES))
+		XSTATE_OP(XRSTORS, xstate, lmask, hmask, err);
+	else
+		XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
 
 	return err;
 }
-- 
2.21.0

[PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

When returning from a signal, there is user state in a userspace memory
buffer and supervisor state in registers.  The in-kernel buffer has neither
valid user or supervisor state.  To restore both, save supervisor fpregs
first (and protect them across context switches), then restore it along
with user state.

This patch introduces saving and restoring of supervisor xstates for a
sigreturn in the following steps:

- Preserve supervisor register values by saving the whole fpu xstates.
  Saving the whole is necessary because doing XSAVES with a partial
  requested-feature bitmap (RFBM) changes xcomp_bv.  Since user xstates are
  to be restored from a user buffer, saved user xstates are discarded.

- Set TIF_NEED_FPU_LOAD, and do __fpu_invalidate_fpregs_state().
  This prevents a context switch from corrupting the saved xstates,
  and xstate is considered to be loaded again on return to userland.

- Under fpregs_lock(), restore user xstates (from the user buffer), and
  then supervisor xstates (from previously saved content).

- When both parts of the restoration succeed, mark fpregs as valid.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/kernel/fpu/signal.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index e3781a4a52a8..0d3e06a772b0 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -327,14 +327,22 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 	}
 
 	/*
-	 * The current state of the FPU registers does not matter. By setting
-	 * TIF_NEED_FPU_LOAD unconditionally it is ensured that the our xstate
-	 * is not modified on context switch and that the xstate is considered
+	 * Supervisor xstates are not modified by user space input, and
+	 * need to be saved and restored.  Save the whole because doing
+	 * partial XSAVES changes xcomp_bv.
+	 * By setting TIF_NEED_FPU_LOAD it is ensured that our xstate is
+	 * not modified on context switch and that the xstate is considered
 	 * to be loaded again on return to userland (overriding last_cpu avoids
 	 * the optimisation).
 	 */
-	set_thread_flag(TIF_NEED_FPU_LOAD);
+	fpregs_lock();
+	if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
+		if (xfeatures_mask_supervisor())
+			copy_xregs_to_kernel(&fpu->state.xsave);
+		set_thread_flag(TIF_NEED_FPU_LOAD);
+	}
 	__fpu_invalidate_fpregs_state(fpu);
+	fpregs_unlock();
 
 	if ((unsigned long)buf_fx % 64)
 		fx_only = 1;
@@ -360,6 +368,9 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		ret = copy_user_to_fpregs_zeroing(buf_fx, xfeatures_user, fx_only);
 		pagefault_enable();
 		if (!ret) {
+			if (xfeatures_mask_supervisor())
+				copy_kernel_to_xregs(&fpu->state.xsave,
+						     xfeatures_mask_supervisor());
 			fpregs_mark_activate();
 			fpregs_unlock();
 			return 0;
@@ -389,7 +400,12 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		fpregs_lock();
 		if (unlikely(init_bv))
 			copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
-		ret = copy_kernel_to_xregs_err(&fpu->state.xsave, xfeatures_user);
+		/*
+		 * Restore previously saved supervisor xstates along with
+		 * copied-in user xstates.
+		 */
+		ret = copy_kernel_to_xregs_err(&fpu->state.xsave,
+					       xfeatures_user | xfeatures_mask_supervisor());
 
 	} else if (use_fxsr()) {
 		ret = __copy_from_user(&fpu->state.fxsave, buf_fx, state_size);
-- 
2.21.0

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Borislav Petkov]

On Tue, Jan 21, 2020 at 12:18:43PM -0800, Yu-cheng Yu wrote:
> When returning from a signal, there is user state in a userspace memory
> buffer and supervisor state in registers.  The in-kernel buffer has neither
> valid user or supervisor state.  To restore both, save supervisor fpregs

The correct formulation is "neither ... nor ... "

> first (and protect them across context switches), then restore it along

s/it/them/

> with user state.
> 
> This patch introduces saving and restoring of supervisor xstates for a

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> sigreturn in the following steps:
> 
> - Preserve supervisor register values by saving the whole fpu xstates.
>   Saving the whole is necessary because doing XSAVES with a partial
>   requested-feature bitmap (RFBM) changes xcomp_bv.  Since user xstates are
>   to be restored from a user buffer, saved user xstates are discarded.
> 
> - Set TIF_NEED_FPU_LOAD, and do __fpu_invalidate_fpregs_state().
>   This prevents a context switch from corrupting the saved xstates,
>   and xstate is considered to be loaded again on return to userland.

s/considered/going to/

> - Under fpregs_lock(), restore user xstates (from the user buffer), and
>   then supervisor xstates (from previously saved content).
> 
> - When both parts of the restoration succeed, mark fpregs as valid.
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
>  arch/x86/kernel/fpu/signal.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index e3781a4a52a8..0d3e06a772b0 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -327,14 +327,22 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>  	}
>  
>  	/*
> -	 * The current state of the FPU registers does not matter. By setting
> -	 * TIF_NEED_FPU_LOAD unconditionally it is ensured that the our xstate
> -	 * is not modified on context switch and that the xstate is considered
> +	 * Supervisor xstates are not modified by user space input, and
> +	 * need to be saved and restored.  Save the whole because doing
> +	 * partial XSAVES changes xcomp_bv.
> +	 * By setting TIF_NEED_FPU_LOAD it is ensured that our xstate is
> +	 * not modified on context switch and that the xstate is considered

Reflow those comments to 80 cols. There's room to the right.

>  	 * to be loaded again on return to userland (overriding last_cpu avoids
>  	 * the optimisation).
>  	 */
> -	set_thread_flag(TIF_NEED_FPU_LOAD);
> +	fpregs_lock();
> +	if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
> +		if (xfeatures_mask_supervisor())
> +			copy_xregs_to_kernel(&fpu->state.xsave);
> +		set_thread_flag(TIF_NEED_FPU_LOAD);

So the code sets TIF_NEED_FPU_LOAD unconditionally, why are you changing
this?

Why don't you simply do:

		set_thread_flag(TIF_NEED_FPU_LOAD);
		fpregs_lock();
		if (xfeatures_mask_supervisor())
			copy_xregs_to_kernel(&fpu->state.xsave);
		fpregs_unlock();


> +	}
>  	__fpu_invalidate_fpregs_state(fpu);
> +	fpregs_unlock();
>  
>  	if ((unsigned long)buf_fx % 64)
>  		fx_only = 1;
> @@ -360,6 +368,9 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>  		ret = copy_user_to_fpregs_zeroing(buf_fx, xfeatures_user, fx_only);
>  		pagefault_enable();

<--- comment here why you're doing this. That function is an abomination
and needs commenting at least.

>  		if (!ret) {
> +			if (xfeatures_mask_supervisor())
> +				copy_kernel_to_xregs(&fpu->state.xsave,
> +						     xfeatures_mask_supervisor());
>  			fpregs_mark_activate();
>  			fpregs_unlock();
>  			return 0;
> @@ -389,7 +400,12 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>  		fpregs_lock();
>  		if (unlikely(init_bv))
>  			copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
> -		ret = copy_kernel_to_xregs_err(&fpu->state.xsave, xfeatures_user);
> +		/*
> +		 * Restore previously saved supervisor xstates along with
> +		 * copied-in user xstates.
> +		 */
> +		ret = copy_kernel_to_xregs_err(&fpu->state.xsave,
> +					       xfeatures_user | xfeatures_mask_supervisor());
>  
>  	} else if (use_fxsr()) {
>  		ret = __copy_from_user(&fpu->state.fxsave, buf_fx, state_size);
> -- 
> 2.21.0
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Fri, 2020-02-21 at 18:58 +0100, Borislav Petkov wrote:
> On Tue, Jan 21, 2020 at 12:18:43PM -0800, Yu-cheng Yu wrote:
> > [...]
> >  arch/x86/kernel/fpu/signal.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> > index e3781a4a52a8..0d3e06a772b0 100644
> > --- a/arch/x86/kernel/fpu/signal.c
> > +++ b/arch/x86/kernel/fpu/signal.c
> > @@ -327,14 +327,22 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> >  	}
> >  
> >  	/*
> > -	 * The current state of the FPU registers does not matter. By setting
> > -	 * TIF_NEED_FPU_LOAD unconditionally it is ensured that the our xstate
> > -	 * is not modified on context switch and that the xstate is considered
> > +	 * Supervisor xstates are not modified by user space input, and
> > +	 * need to be saved and restored.  Save the whole because doing
> > +	 * partial XSAVES changes xcomp_bv.
> > +	 * By setting TIF_NEED_FPU_LOAD it is ensured that our xstate is
> > +	 * not modified on context switch and that the xstate is considered
> 
> Reflow those comments to 80 cols. There's room to the right.
> 
> >  	 * to be loaded again on return to userland (overriding last_cpu avoids
> >  	 * the optimisation).
> >  	 */
> > -	set_thread_flag(TIF_NEED_FPU_LOAD);
> > +	fpregs_lock();
> > +	if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
> > +		if (xfeatures_mask_supervisor())
> > +			copy_xregs_to_kernel(&fpu->state.xsave);
> > +		set_thread_flag(TIF_NEED_FPU_LOAD);
> 
> So the code sets TIF_NEED_FPU_LOAD unconditionally, why are you changing
> this?
> 
> Why don't you simply do:
> 
> 		set_thread_flag(TIF_NEED_FPU_LOAD);
> 		fpregs_lock();
> 		if (xfeatures_mask_supervisor())
> 			copy_xregs_to_kernel(&fpu->state.xsave);
> 		fpregs_unlock();

If TIF_NEED_FPU_LOAD is set, then xstates are already in the xsave buffer. 
We can skip saving them again.

Yu-cheng

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Borislav Petkov]

On Thu, Feb 27, 2020 at 02:52:12PM -0800, Yu-cheng Yu wrote:
> > So the code sets TIF_NEED_FPU_LOAD unconditionally, why are you changing
> > this?
> > 
> > Why don't you simply do:
> > 
> > 		set_thread_flag(TIF_NEED_FPU_LOAD);
> > 		fpregs_lock();
> > 		if (xfeatures_mask_supervisor())
> > 			copy_xregs_to_kernel(&fpu->state.xsave);
> > 		fpregs_unlock();
> 
> If TIF_NEED_FPU_LOAD is set, then xstates are already in the xsave buffer. 
> We can skip saving them again.

Ok, then pls use test_and_set_thread_flag().

Also, in talking to Sebastian about this on IRC, he raised a valid
concern: if we are going to save supervisor states here, then
copy_xregs_to_kernel() should better save *only* supervisor states
because we're not interested in the user states - they're going to be
overwritten with the states from the stack.

So copy_xregs_to_kernel() needs to learn about a second parameter called
@mask like copy_kernel_to_xregs().

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Sebastian Andrzej Siewior]

On 2020-02-28 13:17:24 [+0100], Borislav Petkov wrote:
> On Thu, Feb 27, 2020 at 02:52:12PM -0800, Yu-cheng Yu wrote:
> > If TIF_NEED_FPU_LOAD is set, then xstates are already in the xsave buffer. 
> > We can skip saving them again.
> 
> Ok, then pls use test_and_set_thread_flag().

I've been told not to do this while I crafted kernel_fpu_begin() because
this would introduce an atomic operation which we want avoid.

Sebastian

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Fri, 2020-02-28 at 13:17 +0100, Borislav Petkov wrote:
> On Thu, Feb 27, 2020 at 02:52:12PM -0800, Yu-cheng Yu wrote:
> > > So the code sets TIF_NEED_FPU_LOAD unconditionally, why are you changing
> > > this?
> > > 
> > > Why don't you simply do:
> > > 
> > > 		set_thread_flag(TIF_NEED_FPU_LOAD);
> > > 		fpregs_lock();
> > > 		if (xfeatures_mask_supervisor())
> > > 			copy_xregs_to_kernel(&fpu->state.xsave);
> > > 		fpregs_unlock();
> > 
> > If TIF_NEED_FPU_LOAD is set, then xstates are already in the xsave buffer. 
> > We can skip saving them again.
> 
> Ok, then pls use test_and_set_thread_flag().
> 
> Also, in talking to Sebastian about this on IRC, he raised a valid
> concern: if we are going to save supervisor states here, then
> copy_xregs_to_kernel() should better save *only* supervisor states
> because we're not interested in the user states - they're going to be
> overwritten with the states from the stack.

Yes, saving only supervisor states is optimal, but doing XSAVES with a
partial RFBM changes xcomp_bv.

Yu-cheng

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Borislav Petkov]

On Fri, Feb 28, 2020 at 07:53:38AM -0800, Yu-cheng Yu wrote:
> Yes, saving only supervisor states is optimal, but doing XSAVES with a
> partial RFBM changes xcomp_bv.

... and that means what exactly in plain english?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Fri, 2020-02-28 at 17:23 +0100, Borislav Petkov wrote:
> On Fri, Feb 28, 2020 at 07:53:38AM -0800, Yu-cheng Yu wrote:
> > Yes, saving only supervisor states is optimal, but doing XSAVES with a
> > partial RFBM changes xcomp_bv.
> 
> ... and that means what exactly in plain english?

When XSAVES writes to an xsave buffer, xsave->header.xcomp_bv is set to
include only saved components, effectively changing the buffer's format.

I will include this in the comments.

Yu-cheng

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Sebastian Andrzej Siewior]

On 2020-02-28 08:20:27 [-0800], Yu-cheng Yu wrote:
> On Fri, 2020-02-28 at 17:23 +0100, Borislav Petkov wrote:
> > On Fri, Feb 28, 2020 at 07:53:38AM -0800, Yu-cheng Yu wrote:
> > > Yes, saving only supervisor states is optimal, but doing XSAVES with a
> > > partial RFBM changes xcomp_bv.
> > 
> > ... and that means what exactly in plain english?
> 
> When XSAVES writes to an xsave buffer, xsave->header.xcomp_bv is set to
> include only saved components, effectively changing the buffer's format.

How large is this supervisor state at most? I guess saving the AVX512
state just to get the 2 bytes of the supervisor state at the right spot
is not really optimal.
But this is the performance division…

> I will include this in the comments.

If you do so, please state that the first hunk is only interested in the
supervisor-state bits and everything else is ignored.

> Yu-cheng

Sebastian

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Fri, 2020-02-28 at 17:50 +0100, Sebastian Andrzej Siewior wrote:
> On 2020-02-28 08:20:27 [-0800], Yu-cheng Yu wrote:
> > On Fri, 2020-02-28 at 17:23 +0100, Borislav Petkov wrote:
> > > On Fri, Feb 28, 2020 at 07:53:38AM -0800, Yu-cheng Yu wrote:
> > > > Yes, saving only supervisor states is optimal, but doing XSAVES with a
> > > > partial RFBM changes xcomp_bv.
> > > 
> > > ... and that means what exactly in plain english?
> > 
> > When XSAVES writes to an xsave buffer, xsave->header.xcomp_bv is set to
> > include only saved components, effectively changing the buffer's format.
> 
> How large is this supervisor state at most? I guess saving the AVX512
> state just to get the 2 bytes of the supervisor state at the right spot
> is not really optimal.
> But this is the performance division…
> 
> > I will include this in the comments.
> 
> If you do so, please state that the first hunk is only interested in the
> supervisor-state bits and everything else is ignored.

Yes, I will include that.

Yu-cheng

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Borislav Petkov]

On Fri, Feb 28, 2020 at 08:20:27AM -0800, Yu-cheng Yu wrote:
> When XSAVES writes to an xsave buffer, xsave->header.xcomp_bv is set to
> include only saved components, effectively changing the buffer's format.

So you want to *save* the supervisor states and xcomp_bv will be set to
supervisor states only and since we don't care about the user states
there - they will be loaded later - we're good.

Or do you have to set xcomp_bv later in order to save the user
components too and also rearrange the buffer to undo the format change
above?

We have using_compacted_format() and we do conversion from compacted to
standard buffers - I'm looking at copy_xstate_to_kernel() et al - so it
shouldn't be impossible. So to repeat Sebastian's question which you
ignored:

"How large is this supervisor state at most? I guess saving the AVX512
state just to get the 2 bytes of the supervisor state at the right spot
is not really optimal."

In any case, this performance penalty better be paid only by those
who are actually using some supervisor states. I haven't looked at
the CET patchset but I'm assuming you're setting the CET bit in
xfeatures_mask_all only when the feature is being actually used?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Fri, 2020-02-28 at 18:22 +0100, Borislav Petkov wrote:
> On Fri, Feb 28, 2020 at 08:20:27AM -0800, Yu-cheng Yu wrote:
> > When XSAVES writes to an xsave buffer, xsave->header.xcomp_bv is set to
> > include only saved components, effectively changing the buffer's format.
> 
> So you want to *save* the supervisor states and xcomp_bv will be set to
> supervisor states only and since we don't care about the user states
> there - they will be loaded later - we're good.

No!

> Or do you have to set xcomp_bv later in order to save the user
> components too and also rearrange the buffer to undo the format change
> above?

That is the case.  If we save only supervisor states, the buffer becomes
smaller and has only supervisor states.

> We have using_compacted_format() and we do conversion from compacted to
> standard buffers - I'm looking at copy_xstate_to_kernel() et al - so it
> shouldn't be impossible. So to repeat Sebastian's question which you
> ignored:
> 
> "How large is this supervisor state at most? I guess saving the AVX512
> state just to get the 2 bytes of the supervisor state at the right spot
> is not really optimal."

I thought Sebastian was saying XSAVES is not optimal.

CET has 16 bytes for ring-3 setting, 24 bytes for ring-0.
Saving supervisor states somewhere else and copying back is not better
either.

> In any case, this performance penalty better be paid only by those
> who are actually using some supervisor states. I haven't looked at
> the CET patchset but I'm assuming you're setting the CET bit in
> xfeatures_mask_all only when the feature is being actually used?

We save supervisor states only when xfeatures_mask_supervisor() is not
zero.

Yu-cheng

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Borislav Petkov]

On Fri, Feb 28, 2020 at 10:11:44AM -0800, Yu-cheng Yu wrote:
> CET has 16 bytes for ring-3 setting, 24 bytes for ring-0.
> Saving supervisor states somewhere else and copying back is not better
> either.

Well, if you're going to save a lot bigger user states area which is
going to be absolutely wasted cycles in that case, you better save those
couple of bytes in another buffer and then copy them into the final state
buffer which gets restored.

> We save supervisor states only when xfeatures_mask_supervisor() is not
> zero.

And on which systems is it not zero? On systems which have supervisor
features or on systems which have *and* *are* *using* supervisor
features?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Fri, 2020-02-28 at 19:31 +0100, Borislav Petkov wrote:
> On Fri, Feb 28, 2020 at 10:11:44AM -0800, Yu-cheng Yu wrote:
> > CET has 16 bytes for ring-3 setting, 24 bytes for ring-0.
> > Saving supervisor states somewhere else and copying back is not better
> > either.
> 
> Well, if you're going to save a lot bigger user states area which is
> going to be absolutely wasted cycles in that case, you better save those
> couple of bytes in another buffer and then copy them into the final state
> buffer which gets restored.

The code is for sigreturn only.  Because of lazy-restore,
copy_xregs_to_kernel() does not happen all the time.  It is attractive in
terms of simplicity.

XSAVES buffer has fixed 576-byte overhead (512-byte legacy + 64-byte
header) and not suitable for partial saving.  To save only supervisor
states, we need to read out each MSR separately and store them in a struct.

> 
> > We save supervisor states only when xfeatures_mask_supervisor() is not
> > zero.
> 
> And on which systems is it not zero? On systems which have supervisor
> features or on systems which have *and* *are* *using* supervisor
> features?

On systems using supervisor features.

Yu-cheng

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Borislav Petkov]

On Fri, Feb 28, 2020 at 01:22:39PM -0800, Yu-cheng Yu wrote:
> The code is for sigreturn only.  Because of lazy-restore,
> copy_xregs_to_kernel() does not happen all the time.

What does "not all the time" mean? You need to quantify this more
precisely.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Fri, 2020-02-28 at 22:47 +0100, Borislav Petkov wrote:
> On Fri, Feb 28, 2020 at 01:22:39PM -0800, Yu-cheng Yu wrote:
> > The code is for sigreturn only.  Because of lazy-restore,
> > copy_xregs_to_kernel() does not happen all the time.
> 
> What does "not all the time" mean? You need to quantify this more
> precisely.

If the XSAVES buffer already has current data (i.e. TIF_NEED_FPU_LOAD is
set), then skip copy_xregs_to_kernel().  This happens when the task was
context-switched out and has not returned to user-mode.

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Borislav Petkov]

On Fri, Feb 28, 2020 at 02:13:29PM -0800, Yu-cheng Yu wrote:
> If the XSAVES buffer already has current data (i.e. TIF_NEED_FPU_LOAD is
> set), then skip copy_xregs_to_kernel().  This happens when the task was
> context-switched out and has not returned to user-mode.

So I got tired of this peacemeal game back'n'forth and went and did your
work for ya.

First of all, on my fairly new KBL test box, the context size is almost
a kB:

[    0.000000] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers'
[    0.000000] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers'
[    0.000000] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers'
[    0.000000] x86/fpu: Supporting XSAVE feature 0x008: 'MPX bounds registers'
[    0.000000] x86/fpu: Supporting XSAVE feature 0x010: 'MPX CSR'
[    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
[    0.000000] x86/fpu: xstate_offset[3]:  832, xstate_sizes[3]:   64
[    0.000000] x86/fpu: xstate_offset[4]:  896, xstate_sizes[4]:   64
[    0.000000] x86/fpu: Enabled xstate features 0x1f, context size is 960 bytes, using 'compacted' format.

Then, I added this ontop of your patchset:

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 0d3e06a772b0..2e57b8d79c0e 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -337,6 +337,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
         */
        fpregs_lock();
        if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
+               trace_printk("!NEED_FPU_LOAD, size: %d, supervisor: 0x%llx\n",
+                            size, xfeatures_mask_supervisor());
                if (xfeatures_mask_supervisor())
                        copy_xregs_to_kernel(&fpu->state.xsave);
                set_thread_flag(TIF_NEED_FPU_LOAD);

and traced a fairly boring kernel build workload where the kernel
.config is not even a distro one but a tailored for this machine.

Which means, it took 3m35.058s to build and the trace buffer had 53973
entries like this one:

bash-1211  [002] ...1   648.238585: __fpu__restore_sig: !NEED_FPU_LOAD, size: 1092, supervisor: 0x0

which means I have

53973 / (3*60 + 35) =~ 251 XSAVES invocations per second!

And this only during this single workload - I don't even wanna imagine
what that number would be if it were a huge, overloaded box with a
signal heavy workload.

And all this overhead to save 16 + 24 bytes supervisor states and throw
away the rest up to 960 bytes each time.

Err, I don't think so.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Sat, 2020-02-29 at 15:36 +0100, Borislav Petkov wrote:
> On Fri, Feb 28, 2020 at 02:13:29PM -0800, Yu-cheng Yu wrote:
> > If the XSAVES buffer already has current data (i.e. TIF_NEED_FPU_LOAD is
> > set), then skip copy_xregs_to_kernel().  This happens when the task was
> > context-switched out and has not returned to user-mode.
> 
> So I got tired of this peacemeal game back'n'forth and went and did your
> work for ya.
> 
> First of all, on my fairly new KBL test box, the context size is almost
> a kB:
> 
> [    0.000000] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers'
> [    0.000000] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers'
> [    0.000000] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers'
> [    0.000000] x86/fpu: Supporting XSAVE feature 0x008: 'MPX bounds registers'
> [    0.000000] x86/fpu: Supporting XSAVE feature 0x010: 'MPX CSR'
> [    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
> [    0.000000] x86/fpu: xstate_offset[3]:  832, xstate_sizes[3]:   64
> [    0.000000] x86/fpu: xstate_offset[4]:  896, xstate_sizes[4]:   64
> [    0.000000] x86/fpu: Enabled xstate features 0x1f, context size is 960 bytes, using 'compacted' format.
> 
> Then, I added this ontop of your patchset:
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 0d3e06a772b0..2e57b8d79c0e 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -337,6 +337,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>          */
>         fpregs_lock();
>         if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
> +               trace_printk("!NEED_FPU_LOAD, size: %d, supervisor: 0x%llx\n",
> +                            size, xfeatures_mask_supervisor());
>                 if (xfeatures_mask_supervisor())
>                         copy_xregs_to_kernel(&fpu->state.xsave);
>                 set_thread_flag(TIF_NEED_FPU_LOAD);
> 
> and traced a fairly boring kernel build workload where the kernel
> .config is not even a distro one but a tailored for this machine.
> 
> Which means, it took 3m35.058s to build and the trace buffer had 53973
> entries like this one:
> 
> bash-1211  [002] ...1   648.238585: __fpu__restore_sig: !NEED_FPU_LOAD, size: 1092, supervisor: 0x0
> 
> which means I have
> 
> 53973 / (3*60 + 35) =~ 251 XSAVES invocations per second!
> 
> And this only during this single workload - I don't even wanna imagine
> what that number would be if it were a huge, overloaded box with a
> signal heavy workload.
> 
> And all this overhead to save 16 + 24 bytes supervisor states and throw
> away the rest up to 960 bytes each time.
> 
> Err, I don't think so.

This patch serves supervisor states that has not been saved prior to
sigreturn.  CET state is in sigcontext and does not need to be saved here.

We can drop this for now, and for new supervisor states, replace
copy_xregs_to_kernel() with a callback that saves only necessary
information.

I will send out v3.

Yu-cheng

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Mon, 2020-03-02 at 10:09 -0800, Yu-cheng Yu wrote:
> On Sat, 2020-02-29 at 15:36 +0100, Borislav Petkov wrote:
> > On Fri, Feb 28, 2020 at 02:13:29PM -0800, Yu-cheng Yu wrote:
> > > If the XSAVES buffer already has current data (i.e. TIF_NEED_FPU_LOAD is
> > > set), then skip copy_xregs_to_kernel().  This happens when the task was
> > > context-switched out and has not returned to user-mode.
> > 
> > So I got tired of this peacemeal game back'n'forth and went and did your
> > work for ya.
> > 
> > First of all, on my fairly new KBL test box, the context size is almost
> > a kB:
> > 
> > [    0.000000] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers'
> > [    0.000000] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers'
> > [    0.000000] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers'
> > [    0.000000] x86/fpu: Supporting XSAVE feature 0x008: 'MPX bounds registers'
> > [    0.000000] x86/fpu: Supporting XSAVE feature 0x010: 'MPX CSR'
> > [    0.000000] x86/fpu: xstate_offset[2]:  576, xstate_sizes[2]:  256
> > [    0.000000] x86/fpu: xstate_offset[3]:  832, xstate_sizes[3]:   64
> > [    0.000000] x86/fpu: xstate_offset[4]:  896, xstate_sizes[4]:   64
> > [    0.000000] x86/fpu: Enabled xstate features 0x1f, context size is 960 bytes, using 'compacted' format.
> > 
> > Then, I added this ontop of your patchset:
> > 
> > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> > index 0d3e06a772b0..2e57b8d79c0e 100644
> > --- a/arch/x86/kernel/fpu/signal.c
> > +++ b/arch/x86/kernel/fpu/signal.c
> > @@ -337,6 +337,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
> >          */
> >         fpregs_lock();
> >         if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
> > +               trace_printk("!NEED_FPU_LOAD, size: %d, supervisor: 0x%llx\n",
> > +                            size, xfeatures_mask_supervisor());
> >                 if (xfeatures_mask_supervisor())
> >                         copy_xregs_to_kernel(&fpu->state.xsave);
> >                 set_thread_flag(TIF_NEED_FPU_LOAD);
> > 
> > and traced a fairly boring kernel build workload where the kernel
> > .config is not even a distro one but a tailored for this machine.
> > 
> > Which means, it took 3m35.058s to build and the trace buffer had 53973
> > entries like this one:
> > 
> > bash-1211  [002] ...1   648.238585: __fpu__restore_sig: !NEED_FPU_LOAD, size: 1092, supervisor: 0x0
> > 
> > which means I have
> > 
> > 53973 / (3*60 + 35) =~ 251 XSAVES invocations per second!
> > 
> > And this only during this single workload - I don't even wanna imagine
> > what that number would be if it were a huge, overloaded box with a
> > signal heavy workload.
> > 
> > And all this overhead to save 16 + 24 bytes supervisor states and throw
> > away the rest up to 960 bytes each time.
> > 
> > Err, I don't think so.
> 
> This patch serves supervisor states that has not been saved prior to
> sigreturn.  CET state is in sigcontext and does not need to be saved here.
> 
> We can drop this for now, and for new supervisor states, replace
> copy_xregs_to_kernel() with a callback that saves only necessary
> information.
> 
> I will send out v3.

There is another way to keep this patch...

if (xfeatures_mask_supervisor()) {
	fpu->state.xsave.xfeatures &= xfeatures_mask_supervisor();
	copy_xregs_to_kernel(&fpu->state.xsave);
}

That way, all the user states are in init state and only supervisor states
are saved, and the buffer format is unchanged.

Yu-cheng

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Borislav Petkov]

On Wed, Mar 04, 2020 at 10:18:46AM -0800, Yu-cheng Yu wrote:
> There is another way to keep this patch...
> 
> if (xfeatures_mask_supervisor()) {
> 	fpu->state.xsave.xfeatures &= xfeatures_mask_supervisor();

Is the subsequent XSAVE in copy_user_to_fpregs_zeroing() going to
restore the user bits in XSTATE_BV you just cleared?

Sorry, it looks like it would but the SDM text is abysmal.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Yu-cheng Yu]

On Fri, 2020-03-06 at 21:50 +0100, Borislav Petkov wrote:
> On Wed, Mar 04, 2020 at 10:18:46AM -0800, Yu-cheng Yu wrote:
> > There is another way to keep this patch...
> > 
> > if (xfeatures_mask_supervisor()) {
> > 	fpu->state.xsave.xfeatures &= xfeatures_mask_supervisor();
> 
> Is the subsequent XSAVE in copy_user_to_fpregs_zeroing() going to
> restore the user bits in XSTATE_BV you just cleared?
> 
> Sorry, it looks like it would but the SDM text is abysmal.

I checked and this won't work.



Earlier you wrote:

  53973 / (3*60 + 35) =~ 251 XSAVES invocations per second!

I would argue that the kernel does much more than that for context
switches.

These are from:
  perf record -a make -j32 bzImage

# Samples: 11M of event 'cycles'
# Event count (approx.): 7610600069602
#
# Overhead  Symbol
     2.19%  [.] ht_lookup_with_hash
     1.74%  [.] _int_malloc
     1.46%  [.] _cpp_lex_token
     1.46%  [.] ggc_internal_alloc
     1.10%  [.] cpp_get_token_with_location
     1.10%  [.] malloc
     1.05%  [.] _int_free
     0.71%  [.] elf_read
     0.70%  [.] ggc_internal_cleared_alloc
     0.69%  [.] htab_find_slot
     0.69%  [.] c_lex_with_flags
     0.61%  [.] get_combined_adhoc_loc
     0.57%  [.] linemap_position_for_column
[...]
     0.00%  [.] 0x0000000000bad020
     0.00%  [.] 0x0000000000b4952b
     0.00%  [k] __fpu__restore_sig

Here, __fpu__restore_sig() actually takes very little percentage.
Consider this and later maintenance, I think copy_xregs_to_kernel() is at
least not worse than saving each state separately.

Yu-cheng

Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

[Thomas Gleixner]

Yu-cheng Yu <yu-cheng.yu@intel.com> writes:
> On Fri, 2020-03-06 at 21:50 +0100, Borislav Petkov wrote:
> Earlier you wrote:
>
>   53973 / (3*60 + 35) =~ 251 XSAVES invocations per second!
>
> I would argue that the kernel does much more than that for context
> switches.

And that argument is completly bogus. You deliberately ignored that
Boris said that this is just a boring kernel build which is definitely
not a signal heavy workload. And just if you look at what the build
does, it spends a huge amount if time in ld which is not using signals
at all.

> These are from:
>   perf record -a make -j32 bzImage

How useful: This is a systemwide record and not a recording of the
workload. Try again and try with and without your stuff.

Even if you do it proper, those numbers are uninteresting, really.

What do they tell? Nothing but a picture of a workload which is known to
be not signal heavy. You can also run while(1); for 20 seconds thats
equally useful.

Run a signal benchmark and then show the numbers before and after and
chose a benchmark which is not running on a single CPU, chose one which
is bouncing signals back and forth between two busy threads pinned on
different CPUs.

> Consider this and later maintenance, I think copy_xregs_to_kernel() is at
> least not worse than saving each state separately.

Please provide precise numbers taken with TSC for both variants just for
that particular code path.

And these numbers will only tell half of the story because they still do
not take the cache foot print into account. 1000 bytes just to throw
away 960 of them?

While I appreciate that you worry about maintenance I can assure you
that the code will be in a maintainable state at the point where it is
going to be merged.

Thanks,

        tglx