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

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

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

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>

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

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