From: Mitchell Levy levymitchell0@gmail.com
When computing which xfeatures are available, make sure that LBR is only present if both LBR is supported in general, as well as by XSAVES.
There are two distinct CPU features related to the use of XSAVES as it applies to LBR: whether LBR is itself supported (strictly speaking, I'm not sure that this is necessary to check though it's certainly a good sanity check), and whether XSAVES supports LBR (see sections 13.2 and 13.5.12 of the Intel 64 and IA-32 Architectures Software Developer's Manual, Volume 1). Currently, the LBR subsystem correctly checks both (see intel_pmu_arch_lbr_init), however the xstate initialization subsystem does not.
When calculating what value to place in the IA32_XSS MSR, xfeatures_mask_independent only checks whether LBR support is present, not whether XSAVES supports LBR. If XSAVES does not support LBR, this write causes #GP, leaving the state of IA32_XSS unchanged (i.e., set to zero, as its not written with other values, and its default value is zero out of RESET per section 13.3 of the arch manual).
Then, the next time XRSTORS is used to restore supervisor state, it will fail with #GP (because the RFBM has zero for all supervisor features, which does not match the XCOMP_BV field). In particular, XFEATURE_MASK_FPSTATE includes supervisor features, so setting up the FPU will cause a #GP. This results in a call to fpu_reset_from_exception_fixup, which by the same process results in another #GP. Eventually this causes the kernel to run out of stack space and #DF.
Fixes: d72c87018d00 ("x86/fpu/xstate: Move remaining xfeature helpers to core") Cc: stable@vger.kernel.org
Signed-off-by: Mitchell Levy levymitchell0@gmail.com --- arch/x86/kernel/fpu/xstate.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h index 2ee0b9c53dcc..574d2c2ea227 100644 --- a/arch/x86/kernel/fpu/xstate.h +++ b/arch/x86/kernel/fpu/xstate.h @@ -61,7 +61,8 @@ static inline u64 xfeatures_mask_supervisor(void)
static inline u64 xfeatures_mask_independent(void) { - if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR)) + if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR) || + (fpu_kernel_cfg.max_features & XFEATURE_MASK_LBR) != XFEATURE_MASK_LBR) return XFEATURE_MASK_INDEPENDENT & ~XFEATURE_MASK_LBR;
return XFEATURE_MASK_INDEPENDENT;
--- base-commit: de9c2c66ad8e787abec7c9d7eff4f8c3cdd28aed change-id: 20240807-xsave-lbr-fix-02d52f641653
Best regards,
On Thu, Aug 08, 2024 at 04:30:10PM -0700, Mitchell Levy via B4 Relay wrote:
Fixes: d72c87018d00 ("x86/fpu/xstate: Move remaining xfeature helpers to core")
Apologies, this Fixes tag is incorrect, it should instead be: Fixes: f0dccc9da4c0 ("x86/fpu/xstate: Support dynamic supervisor feature for LBR")
On Thu, Aug 08 2024 at 16:30, Mitchell Levy via wrote:
From: Mitchell Levy levymitchell0@gmail.com
When computing which xfeatures are available, make sure that LBR is only present if both LBR is supported in general, as well as by XSAVES.
There are two distinct CPU features related to the use of XSAVES as it applies to LBR: whether LBR is itself supported (strictly speaking, I'm not sure that this is necessary to check though it's certainly a good sanity check), and whether XSAVES supports LBR (see sections 13.2 and 13.5.12 of the Intel 64 and IA-32 Architectures Software Developer's Manual, Volume 1). Currently, the LBR subsystem correctly checks both (see intel_pmu_arch_lbr_init), however the xstate initialization subsystem does not.
When calculating what value to place in the IA32_XSS MSR, xfeatures_mask_independent only checks whether LBR support is present, not whether XSAVES supports LBR. If XSAVES does not support LBR, this write causes #GP, leaving the state of IA32_XSS unchanged (i.e., set to zero, as its not written with other values, and its default value is zero out of RESET per section 13.3 of the arch manual).
Then, the next time XRSTORS is used to restore supervisor state, it will fail with #GP (because the RFBM has zero for all supervisor features, which does not match the XCOMP_BV field). In particular, XFEATURE_MASK_FPSTATE includes supervisor features, so setting up the FPU will cause a #GP. This results in a call to fpu_reset_from_exception_fixup, which by the same process results in another #GP. Eventually this causes the kernel to run out of stack space and #DF.
Cute.
Fixes: d72c87018d00 ("x86/fpu/xstate: Move remaining xfeature helpers to core")
This is not the culprit/
Cc: stable@vger.kernel.org
Signed-off-by: Mitchell Levy levymitchell0@gmail.com
arch/x86/kernel/fpu/xstate.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h index 2ee0b9c53dcc..574d2c2ea227 100644 --- a/arch/x86/kernel/fpu/xstate.h +++ b/arch/x86/kernel/fpu/xstate.h @@ -61,7 +61,8 @@ static inline u64 xfeatures_mask_supervisor(void) static inline u64 xfeatures_mask_independent(void) {
- if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR))
- if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR) ||
(fpu_kernel_cfg.max_features & XFEATURE_MASK_LBR) != XFEATURE_MASK_LBR)
This is wrong because fpu_kernel_cfg.max_features never contains XFEATURE_MASK_LBR. It only contains the bits which are managed by the FPU subsystem.
Thanks,
tglx
On Fri, Aug 09 2024 at 11:52, Thomas Gleixner wrote:
static inline u64 xfeatures_mask_independent(void) {
- if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR))
- if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR) ||
(fpu_kernel_cfg.max_features & XFEATURE_MASK_LBR) != XFEATURE_MASK_LBR)This is wrong because fpu_kernel_cfg.max_features never contains XFEATURE_MASK_LBR. It only contains the bits which are managed by the FPU subsystem.
You want something like the uncompiled below.
The LBR bit should be probably cleared when the CPU feature is not there at some point in the boot process to avoid the whole is enabled and masking business, but that's an orthogonal issue.
Thanks,
tglx --- arch/x86/include/asm/fpu/types.h | 7 +++++++ arch/x86/kernel/fpu/xstate.c | 2 ++ arch/x86/kernel/fpu/xstate.h | 4 ++-- 3 files changed, 11 insertions(+), 2 deletions(-)
--- a/arch/x86/include/asm/fpu/types.h +++ b/arch/x86/include/asm/fpu/types.h @@ -591,6 +591,13 @@ struct fpu_state_config { * even without XSAVE support, i.e. legacy features FP + SSE */ u64 legacy_features; + /* + * @independent_features: + * + * Features which are supported by XSAVES but not managed + * by the FPU core, e.g. LBR + */ + u64 independent_features; };
/* FPU state configuration information */ --- a/arch/x86/kernel/fpu/xstate.c +++ b/arch/x86/kernel/fpu/xstate.c @@ -788,6 +788,8 @@ void __init fpu__init_system_xstate(unsi goto out_disable; }
+ fpu_kernel_cfg.independent_features = fpu_kernel_cfg.max_features & + XFEATURE_MASK_INDEPENDENT; /* * Clear XSAVE features that are disabled in the normal CPUID. */ --- a/arch/x86/kernel/fpu/xstate.h +++ b/arch/x86/kernel/fpu/xstate.h @@ -62,9 +62,9 @@ static inline u64 xfeatures_mask_supervi static inline u64 xfeatures_mask_independent(void) { if (!cpu_feature_enabled(X86_FEATURE_ARCH_LBR)) - return XFEATURE_MASK_INDEPENDENT & ~XFEATURE_MASK_LBR; + return fpu_kernel_cfg.independent_features & ~XFEATURE_MASK_LBR;
- return XFEATURE_MASK_INDEPENDENT; + return fpu_kernel_cfg.independent_features; }
/* XSAVE/XRSTOR wrapper functions */
linux-stable-mirror@lists.linaro.org
-
Mitchell Levy -
Mitchell Levy via B4 Relay -
Thomas Gleixner