[PATCH] fpu: xstate: Keep xfd_state always in-sync with IA32_XFD MSR

[PATCH] fpu: xstate: Keep xfd_state always in-sync with IA32_XFD MSR

[Adamos Ttofari]

Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
per_cpu variable xfd_state to keep the IA32_XFD MSR value cached. In
order to avoid unnecessary writes to the MSR.

xfd_state might not be always synced with the MSR. Eventually affecting
MSR writes. xfd_state is initialized with 0, meanwhile the MSR is
initialized with the XFEATURE_MASK_USER_DYNAMIC to make XFD fire. Then
later on reschedule to a different CPU, when a process that uses extended
xfeatures and handled the #NM (by allocating the additional space in task's
fpstate for extended xfeatures) it will skip the MSR update in
restore_fpregs_from_fpstate because the value might match to already cached
xfd_state (meanwhile it is not the same with the MSR). Eventually calling a
XRSTOR to set the new state (that caries extended xfeatures) and fire a #NM
from kernel context. The XFD is expected to fire from user-space context,
but not in this case and the kernel crashes.

To address the issue mentioned initialize xfd_state with the current MSR
value and update the XFD MSR always with xfd_update_state to avoid
un-sync cases.

Fixes: 672365477ae8 ("x86/fpu: Update XFD state where required")

Signed-off-by: Adamos Ttofari <attofari@amazon.de>
---
 arch/x86/kernel/fpu/xstate.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 0bab497c9436..36ed27ac0ecd 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -179,8 +179,14 @@ void fpu__init_cpu_xstate(void)
 	 * key as that does not work on the boot CPU. This also ensures
 	 * that any stale state is wiped out from XFD.
 	 */
-	if (cpu_feature_enabled(X86_FEATURE_XFD))
-		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
+	if (cpu_feature_enabled(X86_FEATURE_XFD)) {
+		u64 xfd;
+
+		rdmsrl(MSR_IA32_XFD, xfd);
+		__this_cpu_write(xfd_state, xfd);
+
+		xfd_update_state(&init_fpstate);
+	}
 
 	/*
 	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
@@ -915,7 +921,7 @@ void fpu__resume_cpu(void)
 	}
 
 	if (fpu_state_size_dynamic())
-		wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
+		xfd_update_state(&init_fpstate);
 }
 
 /*
-- 
2.39.2




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH] fpu: xstate: Keep xfd_state always in-sync with IA32_XFD MSR

[Dave Hansen]

On 5/11/23 08:28, Adamos Ttofari wrote:
> @@ -179,8 +179,14 @@ void fpu__init_cpu_xstate(void)
>  	 * key as that does not work on the boot CPU. This also ensures
>  	 * that any stale state is wiped out from XFD.
>  	 */
> -	if (cpu_feature_enabled(X86_FEATURE_XFD))
> -		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
> +	if (cpu_feature_enabled(X86_FEATURE_XFD)) {
> +		u64 xfd;
> +
> +		rdmsrl(MSR_IA32_XFD, xfd);
> +		__this_cpu_write(xfd_state, xfd);
> +
> +		xfd_update_state(&init_fpstate);
> +	}

The above comment didn't _quite_ make it into the context, so I'll paste
it here for your convenience:

>         /*
>          * Must happen after CR4 setup and before xsetbv() to allow KVM
>          * lazy passthrough.  Write independent of the dynamic state static
>          * key as that does not work on the boot CPU. This also ensures
>          * that any stale state is wiped out from XFD.
>          */

Translating there, "the dynamic state static key" means
'__fpu_state_size_dynamic' which is used here:

> static __always_inline __pure bool fpu_state_size_dynamic(void)
> {
>         return static_branch_unlikely(&__fpu_state_size_dynamic);
> }

You might recognize fpu_state_size_dynamic() from the first line of
xfd_update_state(), the function that you added to the above hunk.

Which brings me to ask what the point of calling xfd_update_state() is
in the first place if you're getting away with it not working on the
boot CPU.

Why not just short-circuit the (non-working) xfd_update_state() and do
this directly:

	wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
        __this_cpu_write(xfd_state, init_fpstate.xfd);

I don't think you even need to *READ* the MSR.  You're going to blow it
away anyway.

>  	/*
>  	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
> @@ -915,7 +921,7 @@ void fpu__resume_cpu(void)
>  	}
>  
>  	if (fpu_state_size_dynamic())
> -		wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
> +		xfd_update_state(&init_fpstate);
>  }
>  
>  /*

Re: [PATCH] fpu: xstate: Keep xfd_state always in-sync with IA32_XFD MSR

[Thomas Gleixner]

On Thu, May 11 2023 at 15:28, Adamos Ttofari wrote:

> Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
> commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
> per_cpu variable xfd_state to keep the IA32_XFD MSR value cached. In
> order to avoid unnecessary writes to the MSR.
>
> xfd_state might not be always synced with the MSR. Eventually affecting
> MSR writes. xfd_state is initialized with 0, meanwhile the MSR is
> initialized with the XFEATURE_MASK_USER_DYNAMIC to make XFD fire. Then
> later on reschedule to a different CPU, when a process that uses extended
> xfeatures and handled the #NM (by allocating the additional space in task's
> fpstate for extended xfeatures) it will skip the MSR update in
> restore_fpregs_from_fpstate because the value might match to already cached
> xfd_state (meanwhile it is not the same with the MSR). Eventually calling a
> XRSTOR to set the new state (that caries extended xfeatures) and fire a #NM
> from kernel context. The XFD is expected to fire from user-space context,
> but not in this case and the kernel crashes.

I'm completely confused.

So after reading the patch I think I know what you are trying to
explain:

   On CPU hotplug MSR_IA32_XFD is reset to the init_fpstate.xfd, which
   wipes out any stale state. But the per CPU cached xfd value is not
   reset, which brings them out of sync.

   As a consequence a subsequent xfd_update_state() might fail to update
   the MSR which in turn can result in XRSTOR raising a #NM in kernel
   space, which crashes the kernel.

Right?

> To address the issue mentioned initialize xfd_state with the current MSR
> value and update the XFD MSR always with xfd_update_state to avoid
> un-sync cases.
>
> Fixes: 672365477ae8 ("x86/fpu: Update XFD state where required")
>
> Signed-off-by: Adamos Ttofari <attofari@amazon.de>
> ---
>  arch/x86/kernel/fpu/xstate.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 0bab497c9436..36ed27ac0ecd 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -179,8 +179,14 @@ void fpu__init_cpu_xstate(void)
>  	 * key as that does not work on the boot CPU. This also ensures
>  	 * that any stale state is wiped out from XFD.
>  	 */
> -	if (cpu_feature_enabled(X86_FEATURE_XFD))
> -		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
> +	if (cpu_feature_enabled(X86_FEATURE_XFD)) {
> +		u64 xfd;
> +
> +		rdmsrl(MSR_IA32_XFD, xfd);
> +		__this_cpu_write(xfd_state, xfd);
> +
> +		xfd_update_state(&init_fpstate);
> +	}

This does not compile on 32bit. You want something like the uncompiled
below.

>  	/*
>  	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
> @@ -915,7 +921,7 @@ void fpu__resume_cpu(void)
>  	}
>  
>  	if (fpu_state_size_dynamic())
> -		wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
> +		xfd_update_state(&init_fpstate);

On suspend per CPU xfd_state == current->thread.fpu.fpstate->xfd so it's
correct to restore the exact state which was active _before_ suspend.
xfd_state can't be out of sync in that case, no?

Thanks,

        tglx
---
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 0bab497c9436..70785a722759 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -177,10 +177,11 @@ void fpu__init_cpu_xstate(void)
 	 * Must happen after CR4 setup and before xsetbv() to allow KVM
 	 * lazy passthrough.  Write independent of the dynamic state static
 	 * key as that does not work on the boot CPU. This also ensures
-	 * that any stale state is wiped out from XFD.
+	 * that any stale state is wiped out from XFD. Reset the per CPU
+	 * xfd cache too.
 	 */
 	if (cpu_feature_enabled(X86_FEATURE_XFD))
-		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
+		xfd_reset_state();
 
 	/*
 	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index a4ecb04d8d64..6cfaf72228f4 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -159,9 +159,16 @@ static inline void xfd_update_state(struct fpstate *fpstate)
 	}
 }
 
+static inline void xfd_reset_state(void)
+{
+	wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
+	__this_cpu_write(xfd_state, init_fpstate.xfd);
+}
+
 extern int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu);
 #else
 static inline void xfd_update_state(struct fpstate *fpstate) { }
+static inline void xfd_reset_state(void) { }
 
 static inline int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu) {
 	return -EPERM;

Re: [PATCH] fpu: xstate: Keep xfd_state always in-sync with IA32_XFD MSR

[Chang S. Bae]

On 5/11/2023 12:11 PM, Thomas Gleixner wrote:
> On Thu, May 11 2023 at 15:28, Adamos Ttofari wrote:
> 
>> @@ -915,7 +921,7 @@ void fpu__resume_cpu(void)
>>   	}
>>   
>>   	if (fpu_state_size_dynamic())
>> -		wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
>> +		xfd_update_state(&init_fpstate);

xfd_update_state() here is not a right choice as Thomas pointed out 
multiple times.

> On suspend per CPU xfd_state == current->thread.fpu.fpstate->xfd so it's
> correct to restore the exact state which was active _before_ suspend.
> xfd_state can't be out of sync in that case, no?

Indeed! But, looking around the code, writing 'xfd_state' here appears 
to be consistent and logical I suppose.

Every time writing the MSR in the kernel (except for the init), the 
value is saved in 'xfd_state'. So we don't do it on the sleep path. 
Then, on the wake-up path, perhaps it makes sense to restore the context 
with the saved.

Thanks,
Chang

[PATCH v2] x86: fpu: Keep xfd_state always in sync with MSR_IA32_XFD

[Adamos Ttofari]

Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
per CPU variable xfd_state to keep the MSR_IA32_XFD value cached. In
order to avoid unnecessary writes to the MSR.

On CPU hotplug MSR_IA32_XFD is reset to the init_fpstate.xfd, which
wipes out any stale state. But the per CPU cached xfd value is not
reset, which brings them out of sync.

As a consequence a subsequent xfd_update_state() might fail to update
the MSR which in turn can result in XRSTOR raising a #NM in kernel
space, which crashes the kernel.

To address the issue mentioned, initialize xfd_state together with
MSR_IA32_XFD and always update MSR_IA32_XFD with xfd_set_state to
avoid out of sync cases.

Fixes: 672365477ae8 ("x86/fpu: Update XFD state where required")

Signed-off-by: Adamos Ttofari <attofari@amazon.de>
---
 arch/x86/kernel/fpu/xstate.c |  8 ++++----
 arch/x86/kernel/fpu/xstate.h | 14 ++++++++++----
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 0bab497c9436..d0f151d209d4 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -177,10 +177,11 @@ void fpu__init_cpu_xstate(void)
 	 * Must happen after CR4 setup and before xsetbv() to allow KVM
 	 * lazy passthrough.  Write independent of the dynamic state static
 	 * key as that does not work on the boot CPU. This also ensures
-	 * that any stale state is wiped out from XFD.
+	 * that any stale state is wiped out from XFD. Reset the per CPU
+	 * xfd cache too.
 	 */
 	if (cpu_feature_enabled(X86_FEATURE_XFD))
-		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
+		xfd_set_state(init_fpstate.xfd);
 
 	/*
 	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
@@ -914,8 +915,7 @@ void fpu__resume_cpu(void)
 				     xfeatures_mask_independent());
 	}
 
-	if (fpu_state_size_dynamic())
-		wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
+	xfd_update_state(current->thread.fpu.fpstate);
 }
 
 /*
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index a4ecb04d8d64..d272fc214113 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -147,20 +147,26 @@ static inline void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rs
 #endif
 
 #ifdef CONFIG_X86_64
+static inline void xfd_set_state(u64 xfd)
+{
+	wrmsrl(MSR_IA32_XFD, xfd);
+	__this_cpu_write(xfd_state, xfd);
+}
+
 static inline void xfd_update_state(struct fpstate *fpstate)
 {
 	if (fpu_state_size_dynamic()) {
 		u64 xfd = fpstate->xfd;
 
-		if (__this_cpu_read(xfd_state) != xfd) {
-			wrmsrl(MSR_IA32_XFD, xfd);
-			__this_cpu_write(xfd_state, xfd);
-		}
+		if (__this_cpu_read(xfd_state) != xfd)
+			xfd_set_state(xfd);
 	}
 }
 
 extern int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu);
 #else
+static inline void xfd_set_state(u64 xfd) { }
+
 static inline void xfd_update_state(struct fpstate *fpstate) { }
 
 static inline int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu) {
-- 
2.39.2




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v2] x86: fpu: Keep xfd_state always in sync with MSR_IA32_XFD

[Thomas Gleixner]

On Fri, May 12 2023 at 11:38, Adamos Ttofari wrote:
>  	if (cpu_feature_enabled(X86_FEATURE_XFD))
> -		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
> +		xfd_set_state(init_fpstate.xfd);

> @@ -914,8 +915,7 @@ void fpu__resume_cpu(void)
>  				     xfeatures_mask_independent());
>  	}
>  
> -	if (fpu_state_size_dynamic())
> -		wrmsrl(MSR_IA32_XFD, current->thread.fpu.fpstate->xfd);
> +	xfd_update_state(current->thread.fpu.fpstate);

How is that supposed to work?

    this_cpu(xfd_state) == current->thread.fpu.fpstate->xfd

So the MSR write won't happen.

Thanks,

        tglx

[PATCH v3] x86: fpu: Keep xfd_state always in sync with MSR_IA32_XFD

[Adamos Ttofari]

Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
per CPU variable xfd_state to keep the MSR_IA32_XFD value cached. In
order to avoid unnecessary writes to the MSR.

On CPU hotplug MSR_IA32_XFD is reset to the init_fpstate.xfd, which
wipes out any stale state. But the per CPU cached xfd value is not
reset, which brings them out of sync.

As a consequence a subsequent xfd_update_state() might fail to update
the MSR which in turn can result in XRSTOR raising a #NM in kernel
space, which crashes the kernel.

To address the issue mentioned, initialize xfd_state together with
MSR_IA32_XFD.

Fixes: 672365477ae8 ("x86/fpu: Update XFD state where required")

Signed-off-by: Adamos Ttofari <attofari@amazon.de>
---
 arch/x86/kernel/fpu/xstate.c |  5 +++--
 arch/x86/kernel/fpu/xstate.h | 14 ++++++++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 0bab497c9436..9bff4f07358d 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -177,10 +177,11 @@ void fpu__init_cpu_xstate(void)
 	 * Must happen after CR4 setup and before xsetbv() to allow KVM
 	 * lazy passthrough.  Write independent of the dynamic state static
 	 * key as that does not work on the boot CPU. This also ensures
-	 * that any stale state is wiped out from XFD.
+	 * that any stale state is wiped out from XFD. Reset the per CPU
+	 * xfd cache too.
 	 */
 	if (cpu_feature_enabled(X86_FEATURE_XFD))
-		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
+		xfd_set_state(init_fpstate.xfd);
 
 	/*
 	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index a4ecb04d8d64..d272fc214113 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -147,20 +147,26 @@ static inline void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rs
 #endif
 
 #ifdef CONFIG_X86_64
+static inline void xfd_set_state(u64 xfd)
+{
+	wrmsrl(MSR_IA32_XFD, xfd);
+	__this_cpu_write(xfd_state, xfd);
+}
+
 static inline void xfd_update_state(struct fpstate *fpstate)
 {
 	if (fpu_state_size_dynamic()) {
 		u64 xfd = fpstate->xfd;
 
-		if (__this_cpu_read(xfd_state) != xfd) {
-			wrmsrl(MSR_IA32_XFD, xfd);
-			__this_cpu_write(xfd_state, xfd);
-		}
+		if (__this_cpu_read(xfd_state) != xfd)
+			xfd_set_state(xfd);
 	}
 }
 
 extern int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu);
 #else
+static inline void xfd_set_state(u64 xfd) { }
+
 static inline void xfd_update_state(struct fpstate *fpstate) { }
 
 static inline int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu) {
-- 
2.39.2




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v3] x86: fpu: Keep xfd_state always in sync with MSR_IA32_XFD

[Thomas Gleixner]

On Fri, May 19 2023 at 11:23, Adamos Ttofari wrote:
> Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
> commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
> per CPU variable xfd_state to keep the MSR_IA32_XFD value cached. In
> order to avoid unnecessary writes to the MSR.
>
> On CPU hotplug MSR_IA32_XFD is reset to the init_fpstate.xfd, which
> wipes out any stale state. But the per CPU cached xfd value is not
> reset, which brings them out of sync.
>
> As a consequence a subsequent xfd_update_state() might fail to update
> the MSR which in turn can result in XRSTOR raising a #NM in kernel
> space, which crashes the kernel.
>
> To address the issue mentioned, initialize xfd_state together with
> MSR_IA32_XFD.
>
> Fixes: 672365477ae8 ("x86/fpu: Update XFD state where required")
>
> Signed-off-by: Adamos Ttofari <attofari@amazon.de>

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

Re: [PATCH v3] x86: fpu: Keep xfd_state always in sync with MSR_IA32_XFD

[Chang S. Bae]

On 5/19/2023 4:23 AM, Adamos Ttofari wrote:
> Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
> commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
> per CPU variable xfd_state to keep the MSR_IA32_XFD value cached. In
> order to avoid unnecessary writes to the MSR.
> 
> On CPU hotplug MSR_IA32_XFD is reset to the init_fpstate.xfd, which
> wipes out any stale state. But the per CPU cached xfd value is not
> reset, which brings them out of sync.
> 
> As a consequence a subsequent xfd_update_state() might fail to update
> the MSR which in turn can result in XRSTOR raising a #NM in kernel
> space, which crashes the kernel.
> 
> To address the issue mentioned, initialize xfd_state together with
> MSR_IA32_XFD.
> 
> Fixes: 672365477ae8 ("x86/fpu: Update XFD state where required")
> 
> Signed-off-by: Adamos Ttofari <attofari@amazon.de>

Tested-by: Chang S. Bae <chang.seok.bae@intel.com>

With this test -- which I may follow up to be included the AMX selftest:
https://lore.kernel.org/lkml/6ab71997-8533-1828-7c62-717e2821f147@intel.com/

Thanks,
Chang

[PATCH] selftests/x86/amx: Add a CPU hotplug test

[Chang S. Bae]

Adamos found the issue with the cached XFD state [1]. Although the XFD
state is reset on the CPU hotplug, the per-CPU XFD cache is missing
the reset. Then, running an AMX thread there, the staled value causes
the kernel crash to kill the thread.

This is reproducible when moving an AMX thread to the hot-plugged CPU.
So, add a test case to ensure no issue with that.

It repeats the test due to possible inconsistencies. Then, along with
the hotplug cost, it will bring a noticeable runtime increase. But,
the overall test has a quick turnaround time.

Link: https://lore.kernel.org/lkml/20230519112315.30616-1-attofari@amazon.de/ [1]
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Adamos Ttofari <attofari@amazon.de>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
The overall x86 selftest via "$ make TARGETS='x86' kselftest" takes
about 3.5 -> 5.5 seconds. 'amx_64' itself took about 1.5 more seconds
over 0.x seconds.

But, this overall runtime still takes in a matter of some seconds,
which should be fine I thought.
---
 tools/testing/selftests/x86/amx.c | 133 ++++++++++++++++++++++++++++--
 1 file changed, 126 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/x86/amx.c b/tools/testing/selftests/x86/amx.c
index d884fd69dd51..6f2f0598c706 100644
--- a/tools/testing/selftests/x86/amx.c
+++ b/tools/testing/selftests/x86/amx.c
@@ -3,6 +3,7 @@
 #define _GNU_SOURCE
 #include <err.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <pthread.h>
 #include <setjmp.h>
 #include <stdio.h>
@@ -25,6 +26,8 @@
 # error This test is 64-bit only
 #endif
 
+#define BUF_LEN			1000
+
 #define XSAVE_HDR_OFFSET	512
 #define XSAVE_HDR_SIZE		64
 
@@ -239,11 +242,10 @@ static inline uint64_t get_fpx_sw_bytes_features(void *buffer)
 }
 
 /* Work around printf() being unsafe in signals: */
-#define SIGNAL_BUF_LEN 1000
-char signal_message_buffer[SIGNAL_BUF_LEN];
+char signal_message_buffer[BUF_LEN];
 void sig_print(char *msg)
 {
-	int left = SIGNAL_BUF_LEN - strlen(signal_message_buffer) - 1;
+	int left = BUF_LEN - strlen(signal_message_buffer) - 1;
 
 	strncat(signal_message_buffer, msg, left);
 }
@@ -767,15 +769,15 @@ static int create_threads(int num, struct futex_info *finfo)
 	return 0;
 }
 
-static void affinitize_cpu0(void)
+static inline void affinitize_cpu(int cpu)
 {
 	cpu_set_t cpuset;
 
 	CPU_ZERO(&cpuset);
-	CPU_SET(0, &cpuset);
+	CPU_SET(cpu, &cpuset);
 
 	if (sched_setaffinity(0, sizeof(cpuset), &cpuset) != 0)
-		fatal_error("sched_setaffinity to CPU 0");
+		fatal_error("sched_setaffinity to CPU %d", cpu);
 }
 
 static void test_context_switch(void)
@@ -784,7 +786,7 @@ static void test_context_switch(void)
 	int i;
 
 	/* Affinitize to one CPU to force context switches */
-	affinitize_cpu0();
+	affinitize_cpu(0);
 
 	req_xtiledata_perm();
 
@@ -926,6 +928,121 @@ static void test_ptrace(void)
 		err(1, "ptrace test");
 }
 
+/* CPU Hotplug test */
+
+static void __hotplug_cpu(int online, int cpu)
+{
+	char buf[BUF_LEN] = {};
+	int fd, rc;
+
+	strncat(buf, "/sys/devices/system/cpu/cpu", BUF_LEN);
+	snprintf(buf + strlen(buf), BUF_LEN - strlen(buf), "%d", cpu);
+	strncat(buf, "/online", BUF_LEN - strlen(buf));
+
+	fd = open(buf, O_RDWR);
+	if (fd == -1)
+		fatal_error("open()");
+
+	snprintf(buf, BUF_LEN, "%d", online);
+	rc = write(fd, buf, strlen(buf));
+	if (rc == -1)
+		fatal_error("write()");
+
+	rc = close(fd);
+	if (rc == -1)
+		fatal_error("close()");
+}
+
+static void offline_cpu(int cpu)
+{
+	__hotplug_cpu(0, cpu);
+}
+
+static void online_cpu(int cpu)
+{
+	__hotplug_cpu(1, cpu);
+}
+
+static jmp_buf jmpbuf;
+
+static void handle_sigsegv(int sig, siginfo_t *si, void *ctx_void)
+{
+	siglongjmp(jmpbuf, 1);
+}
+
+#define RETRY 5
+
+/*
+ * Sanity-check the hotplug CPU for its (re-)initialization.
+ *
+ * Create an AMX thread on a CPU, while the hotplug CPU went offline.
+ * Then, plug the offlined back, and move the thread to run on it.
+ *
+ * Repeat this multiple times to ensure no inconsistent failure.
+ * If something goes wrong, the thread will get a signal or killed.
+ */
+static void *switch_cpus(void *arg)
+{
+	int *result = (int *)arg;
+	int i = 0;
+
+	affinitize_cpu(0);
+	offline_cpu(1);
+	load_rand_tiledata(stashed_xsave);
+
+	sethandler(SIGSEGV, handle_sigsegv, SA_ONSTACK);
+	for (i = 0; i < RETRY; i++) {
+		if (i > 0) {
+			affinitize_cpu(0);
+			offline_cpu(1);
+		}
+		if (sigsetjmp(jmpbuf, 1) == 0) {
+			online_cpu(1);
+			affinitize_cpu(1);
+		} else {
+			*result = 1;
+			goto out;
+		}
+	}
+	*result = 0;
+out:
+	clearhandler(SIGSEGV);
+	return result;
+}
+
+static void test_cpuhp(void)
+{
+	int max_cpu_num = sysconf(_SC_NPROCESSORS_ONLN) - 1;
+	void *thread_retval;
+	pthread_t thread;
+	int result, rc;
+
+	if (!max_cpu_num) {
+		printf("[SKIP]\tThe running system has no more CPU for the hotplug test.\n");
+		return;
+	}
+
+	printf("[RUN]\tTest AMX use with the CPU hotplug.\n");
+
+	if (pthread_create(&thread, NULL, switch_cpus, &result))
+		fatal_error("pthread_create()");
+
+	rc = pthread_join(thread, &thread_retval);
+
+	if (rc)
+		fatal_error("pthread_join()");
+
+	/*
+	 * Either an invalid retval or a failed result indicates
+	 * the test failure.
+	 */
+	if (thread_retval != &result || result != 0)
+		printf("[FAIL]\tThe AMX thread had an issue (%s).\n",
+		       thread_retval != &result ? "killed" : "signaled");
+	else
+		printf("[OK]\tThe AMX thread had no issue.\n");
+}
+
 int main(void)
 {
 	/* Check hardware availability at first */
@@ -948,6 +1065,8 @@ int main(void)
 
 	test_ptrace();
 
+	test_cpuhp();
+
 	clearhandler(SIGILL);
 	free_stashed_xsave();
 

base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4
-- 
2.17.1

[PATCH v4] x86/fpu: Keep xfd_state always in sync with MSR_IA32_XFD

[Chang S. Bae]

From: Adamos Ttofari <attofari@amazon.de>

Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
per CPU variable xfd_state to keep the MSR_IA32_XFD value cached. In
order to avoid unnecessary writes to the MSR.

On CPU hotplug MSR_IA32_XFD is reset to the init_fpstate.xfd, which
wipes out any stale state. But the per CPU cached xfd value is not
reset, which brings them out of sync.

As a consequence a subsequent xfd_update_state() might fail to update
the MSR which in turn can result in XRSTOR raising a #NM in kernel
space, which crashes the kernel.

To address the issue mentioned, initialize xfd_state together with
MSR_IA32_XFD.

Fixes: 672365477ae8 ("x86/fpu: Update XFD state where required")
Closes: https://lore.kernel.org/lkml/20230511152818.13839-1-attofari@amazon.de
Signed-off-by: Adamos Ttofari <attofari@amazon.de>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
---
V4 <- V3: No code changes, just a minor tweak in the changelog:
* Adjust the patch subject prefix to align with the preferred format for
  'tip' [1], 'x86: fpu:' -> 'x86/fpu:'
* Add a few more commit tags.

It appears that the issue is predominantly exposed during the execution
of synthetic test cases, such as concurrent AMX operations with CPU
hotplug tests (e.g., 'stress-ng --cpu-online' [2]). While this might be
seen as a corner case, it has been a recurring failure during hardware
validation, leading to confusion regarding the source of the problem.

I considered updating the AMX test like this [3]. But, the change
introduced side effects, adding about 2 more seconds to run the test.
Also, running AMX concurrently with aggressively on-/off-lining of CPUs
doesn't seem to align well typical usage scenarios that warrant testing.
Nonetheless, I'm open to add this test if it's deemed necessary.

[1]: https://www.kernel.org/doc/html/next/process/maintainer-tip.html#patch-subject
[2]: https://github.com/ColinIanKing/stress-ng/blob/master/stress-cpu-online.c#L27
[3]: https://lore.kernel.org/lkml/20230603152455.12444-1-chang.seok.bae@intel.com/
---
 arch/x86/kernel/fpu/xstate.c |  5 +++--
 arch/x86/kernel/fpu/xstate.h | 14 ++++++++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 117e74c44e75..33a214b1a4ce 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -178,10 +178,11 @@ void fpu__init_cpu_xstate(void)
 	 * Must happen after CR4 setup and before xsetbv() to allow KVM
 	 * lazy passthrough.  Write independent of the dynamic state static
 	 * key as that does not work on the boot CPU. This also ensures
-	 * that any stale state is wiped out from XFD.
+	 * that any stale state is wiped out from XFD. Reset the per CPU
+	 * xfd cache too.
 	 */
 	if (cpu_feature_enabled(X86_FEATURE_XFD))
-		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
+		xfd_set_state(init_fpstate.xfd);
 
 	/*
 	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 3518fb26d06b..19ca623ffa2a 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -148,20 +148,26 @@ static inline void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rs
 #endif
 
 #ifdef CONFIG_X86_64
+static inline void xfd_set_state(u64 xfd)
+{
+	wrmsrl(MSR_IA32_XFD, xfd);
+	__this_cpu_write(xfd_state, xfd);
+}
+
 static inline void xfd_update_state(struct fpstate *fpstate)
 {
 	if (fpu_state_size_dynamic()) {
 		u64 xfd = fpstate->xfd;
 
-		if (__this_cpu_read(xfd_state) != xfd) {
-			wrmsrl(MSR_IA32_XFD, xfd);
-			__this_cpu_write(xfd_state, xfd);
-		}
+		if (__this_cpu_read(xfd_state) != xfd)
+			xfd_set_state(xfd);
 	}
 }
 
 extern int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu);
 #else
+static inline void xfd_set_state(u64 xfd) { }
+
 static inline void xfd_update_state(struct fpstate *fpstate) { }
 
 static inline int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu) {
-- 
2.40.1

[tip: x86/urgent] x86/fpu: Keep xfd_state in sync with MSR_IA32_XFD

[tip-bot2 for Adamos Ttofari]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     10e4b5166df9ff7a2d5316138ca668b42d004422
Gitweb:        https://git.kernel.org/tip/10e4b5166df9ff7a2d5316138ca668b42d004422
Author:        Adamos Ttofari <attofari@amazon.de>
AuthorDate:    Fri, 22 Mar 2024 16:04:39 -07:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Sun, 24 Mar 2024 04:03:54 +01:00

x86/fpu: Keep xfd_state in sync with MSR_IA32_XFD

Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
per CPU variable xfd_state to keep the MSR_IA32_XFD value cached, in
order to avoid unnecessary writes to the MSR.

On CPU hotplug MSR_IA32_XFD is reset to the init_fpstate.xfd, which
wipes out any stale state. But the per CPU cached xfd value is not
reset, which brings them out of sync.

As a consequence a subsequent xfd_update_state() might fail to update
the MSR which in turn can result in XRSTOR raising a #NM in kernel
space, which crashes the kernel.

To fix this, introduce xfd_set_state() to write xfd_state together
with MSR_IA32_XFD, and use it in all places that set MSR_IA32_XFD.

Fixes: 672365477ae8 ("x86/fpu: Update XFD state where required")
Signed-off-by: Adamos Ttofari <attofari@amazon.de>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20240322230439.456571-1-chang.seok.bae@intel.com

Closes: https://lore.kernel.org/lkml/20230511152818.13839-1-attofari@amazon.de
---
 arch/x86/kernel/fpu/xstate.c |  5 +++--
 arch/x86/kernel/fpu/xstate.h | 14 ++++++++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 117e74c..33a214b 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -178,10 +178,11 @@ void fpu__init_cpu_xstate(void)
 	 * Must happen after CR4 setup and before xsetbv() to allow KVM
 	 * lazy passthrough.  Write independent of the dynamic state static
 	 * key as that does not work on the boot CPU. This also ensures
-	 * that any stale state is wiped out from XFD.
+	 * that any stale state is wiped out from XFD. Reset the per CPU
+	 * xfd cache too.
 	 */
 	if (cpu_feature_enabled(X86_FEATURE_XFD))
-		wrmsrl(MSR_IA32_XFD, init_fpstate.xfd);
+		xfd_set_state(init_fpstate.xfd);
 
 	/*
 	 * XCR_XFEATURE_ENABLED_MASK (aka. XCR0) sets user features
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 3518fb2..19ca623 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -148,20 +148,26 @@ static inline void xfd_validate_state(struct fpstate *fpstate, u64 mask, bool rs
 #endif
 
 #ifdef CONFIG_X86_64
+static inline void xfd_set_state(u64 xfd)
+{
+	wrmsrl(MSR_IA32_XFD, xfd);
+	__this_cpu_write(xfd_state, xfd);
+}
+
 static inline void xfd_update_state(struct fpstate *fpstate)
 {
 	if (fpu_state_size_dynamic()) {
 		u64 xfd = fpstate->xfd;
 
-		if (__this_cpu_read(xfd_state) != xfd) {
-			wrmsrl(MSR_IA32_XFD, xfd);
-			__this_cpu_write(xfd_state, xfd);
-		}
+		if (__this_cpu_read(xfd_state) != xfd)
+			xfd_set_state(xfd);
 	}
 }
 
 extern int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu);
 #else
+static inline void xfd_set_state(u64 xfd) { }
+
 static inline void xfd_update_state(struct fpstate *fpstate) { }
 
 static inline int __xfd_enable_feature(u64 which, struct fpu_guest *guest_fpu) {

Re: [PATCH v2] x86: fpu: Keep xfd_state always in sync with MSR_IA32_XFD

[Chang S. Bae]

On 5/12/2023 4:38 AM, Adamos Ttofari wrote:
> Commit 672365477ae8 ("x86/fpu: Update XFD state where required") and
> commit 8bf26758ca96 ("x86/fpu: Add XFD state to fpstate") introduced a
> per CPU variable xfd_state to keep the MSR_IA32_XFD value cached. In
> order to avoid unnecessary writes to the MSR.
> 
> On CPU hotplug MSR_IA32_XFD is reset to the init_fpstate.xfd, which
> wipes out any stale state. But the per CPU cached xfd value is not
> reset, which brings them out of sync.
> 
> As a consequence a subsequent xfd_update_state() might fail to update
> the MSR which in turn can result in XRSTOR raising a #NM in kernel
> space, which crashes the kernel.

I have drafted this to reproduce and test the issue in the amx selftest:

diff --git a/tools/testing/selftests/x86/amx.c 
b/tools/testing/selftests/x86/amx.c
index d884fd69dd51..c773de1f3864 100644
--- a/tools/testing/selftests/x86/amx.c
+++ b/tools/testing/selftests/x86/amx.c
@@ -767,15 +767,15 @@ static int create_threads(int num, struct 
futex_info *finfo)
         return 0;
  }

-static void affinitize_cpu0(void)
+static inline void affinitize_cpu(int cpu)
  {
         cpu_set_t cpuset;

         CPU_ZERO(&cpuset);
-       CPU_SET(0, &cpuset);
+       CPU_SET(cpu, &cpuset);

         if (sched_setaffinity(0, sizeof(cpuset), &cpuset) != 0)
-               fatal_error("sched_setaffinity to CPU 0");
+               fatal_error("sched_setaffinity to CPU %d", cpu);
  }

  static void test_context_switch(void)
@@ -784,7 +784,7 @@ static void test_context_switch(void)
         int i;

         /* Affinitize to one CPU to force context switches */
-       affinitize_cpu0();
+       affinitize_cpu(0);

         req_xtiledata_perm();

@@ -926,6 +926,120 @@ static void test_ptrace(void)
                 err(1, "ptrace test");
  }

+/* CPU Hotplug test */
+
+#define STRING_BUF_LEN 1024
+
+void __hotplug_cpu(int online, int cpu)
+{
+       char buf[STRING_BUF_LEN] = {};
+       int ret = 0;
+
+       strncat(buf, "echo ", STRING_BUF_LEN - strlen(buf));
+       snprintf(buf + strlen(buf), STRING_BUF_LEN - strlen(buf), "%d", 
online);
+       strncat(buf, " > /sys/devices/system/cpu/cpu", STRING_BUF_LEN - 
strlen(buf));
+       snprintf(buf + strlen(buf), STRING_BUF_LEN - strlen(buf), "%d", 
cpu);
+       strncat(buf, "/online", STRING_BUF_LEN - strlen(buf));
+
+       ret = system(buf);
+       if (ret)
+               err(1, "%s\n", buf);
+}
+
+void offline_cpu(int cpu)
+{
+       __hotplug_cpu(0, cpu);
+}
+
+void online_cpu(int cpu)
+{
+       __hotplug_cpu(1, cpu);
+}
+
+static jmp_buf jmpbuf;
+
+static void handle_sigsegv(int sig, siginfo_t *si, void *ctx_void)
+{
+       siglongjmp(jmpbuf, 1);
+}
+
+#define RETRY 5
+
+/*
+ * Sanity checks the hotplug CPU for its (re-)initialization.
+ *
+ * An AMX thread is created on a CPU while the other one went offline.
+ * Then, plug the offline CPU, and migrate the thread. Repeat this
+ * on/off switches multiple times to ensure no inconsistent failure.
+ * If something goes wrong, the thread gets a signal or is just
+ * killed.
+ */
+void *switch_cpus(void *arg)
+{
+       unsigned int altstack_size = getauxval(AT_MINSIGSTKSZ) + SIGSTKSZ;
+       int *result = (int *)arg;
+       void *altstack;
+       int i = 0;
+
+       altstack = alloc_altstack(altstack_size);
+       setup_altstack(altstack, altstack_size, SUCCESS_EXPECTED);
+
+       affinitize_cpu(0);
+       offline_cpu(1);
+       load_rand_tiledata(stashed_xsave);
+
+       sethandler(SIGSEGV, handle_sigsegv, SA_ONSTACK);
+       for (i = 0;i < RETRY;i++) {
+               if (i > 0) {
+                       affinitize_cpu(0);
+                       offline_cpu(1);
+               }
+               if (sigsetjmp(jmpbuf, 1) == 0) {
+                       online_cpu(1);
+                       affinitize_cpu(1);
+               } else {
+                       *result = 1;
+                       goto out;
+               }
+       }
+       *result = 0;
+out:
+       clearhandler(SIGSEGV);
+       return result;
+}
+
+void test_cpuhp(void)
+{
+       int max_cpu_num = sysconf(_SC_NPROCESSORS_ONLN) - 1;
+       void *thread_retval;
+       pthread_t thread;
+       int result, rc;
+
+       if (!max_cpu_num) {
+               printf("[SKIP]\tThe running system do not have any spare 
CPU for the hotplug\n");
+               return;
+       }
+
+       printf("[RUN]\tTest AMX state use with CPU hotplug\n");
+
+       if (pthread_create(&thread, NULL, switch_cpus, &result))
+               fatal_error("pthread_creat()\n");
+
+       rc = pthread_join(thread, &thread_retval);
+
+       if (rc)
+               fatal_error("pthread_join()\n");
+
+       /*
+        * Either an invalid retval or a failed result indicates
+        * the test failure.
+        */
+       if (thread_retval != &result || result != 0)
+               printf("[FAIL]\tThe AMX thread had an issue with the CPU 
hotplug.\n");
+       else
+               printf("[OK]\tThe AMX thread has no issue with the CPU 
hotplug.\n");
+}
+
  int main(void)
  {
         /* Check hardware availability at first */
@@ -948,6 +1062,8 @@ int main(void)

         test_ptrace();

+       test_cpuhp();
+
         clearhandler(SIGILL);
         free_stashed_xsave();

Thanks,
Chang