[PATCH 0/2] x86/FPU: FPU sanitization for in-kernel use

[PATCH 0/2] x86/FPU: FPU sanitization for in-kernel use

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Hi all,

here's a proper submission of the work started by Petteri. I think I've
addressed all the feedback so far. I've added the preparation work for
the test to run, to a script run_test_fpu.sh which does some basic
checks, loads the module and runs the test for 1000 times on all CPUs,
by default. Thought this is a sane default, feel free to prove me wrong
and I'll change it.

Thx.

Petteri Aimonen (2):
  x86/fpu: Reset MXCSR to default in kernel_fpu_begin()
  selftests/fpu: Add an FPU selftest

 arch/x86/include/asm/fpu/internal.h         |  5 ++
 arch/x86/kernel/fpu/core.c                  |  6 ++
 lib/Kconfig.debug                           | 11 +++
 lib/Makefile                                | 20 +++++
 lib/test_fpu.c                              | 89 +++++++++++++++++++++
 tools/testing/selftests/Makefile            |  1 +
 tools/testing/selftests/fpu/.gitignore      |  2 +
 tools/testing/selftests/fpu/Makefile        |  9 +++
 tools/testing/selftests/fpu/run_test_fpu.sh | 46 +++++++++++
 tools/testing/selftests/fpu/test_fpu.c      | 61 ++++++++++++++
 10 files changed, 250 insertions(+)
 create mode 100644 lib/test_fpu.c
 create mode 100644 tools/testing/selftests/fpu/.gitignore
 create mode 100644 tools/testing/selftests/fpu/Makefile
 create mode 100755 tools/testing/selftests/fpu/run_test_fpu.sh
 create mode 100644 tools/testing/selftests/fpu/test_fpu.c

-- 
2.21.0

[PATCH 1/2] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Borislav Petkov]

From: Petteri Aimonen <jpa@git.mail.kapsi.fi>

Previously, kernel floating point code would run with the MXCSR control
register value last set by userland code by the thread that was active
on the CPU core just before kernel call. This could affect calculation
results if rounding mode was changed, or a crash if a FPU/SIMD exception
was unmasked.

Restore MXCSR to the kernel's default value.

 [ bp: Carve out from a bigger patch by Petteri, add feature check, add
   FNINIT call too (amluto). ]

Signed-off-by: Petteri Aimonen <jpa@git.mail.kapsi.fi>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=207979
---
 arch/x86/include/asm/fpu/internal.h | 5 +++++
 arch/x86/kernel/fpu/core.c          | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 42159f45bf9c..845e7481ab77 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -623,6 +623,11 @@ static inline void switch_fpu_finish(struct fpu *new_fpu)
  * MXCSR and XCR definitions:
  */
 
+static inline void ldmxcsr(u32 mxcsr)
+{
+	asm volatile("ldmxcsr %0" :: "m" (mxcsr));
+}
+
 extern unsigned int mxcsr_feature_mask;
 
 #define XCR_XFEATURE_ENABLED_MASK	0x00000000
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 06c818967bb6..15247b96c6ea 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -101,6 +101,12 @@ void kernel_fpu_begin(void)
 		copy_fpregs_to_fpstate(&current->thread.fpu);
 	}
 	__cpu_invalidate_fpregs_state();
+
+	if (boot_cpu_has(X86_FEATURE_XMM))
+		ldmxcsr(MXCSR_DEFAULT);
+
+	if (boot_cpu_has(X86_FEATURE_FPU))
+		asm volatile ("fninit");
 }
 EXPORT_SYMBOL_GPL(kernel_fpu_begin);
 
-- 
2.21.0

[PATCH 2/2] selftests/fpu: Add an FPU selftest

[Borislav Petkov]

From: Petteri Aimonen <jpa@git.mail.kapsi.fi>

Add a selftest for the usage of FPU code in kernel mode.

Currently only implemented for x86. In the future, kernel FPU testing
could be unified between the different architectures supporting it.

 [ bp: Split out from a conglomerate patch, put comments over statements,
   run the test only on debugfs write. Add bare-minimum run_test_fpu.sh,
   run 1000 iterations on all CPUs by default. ]

Signed-off-by: Petteri Aimonen <jpa@git.mail.kapsi.fi>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 lib/Kconfig.debug                           | 11 +++
 lib/Makefile                                | 20 +++++
 lib/test_fpu.c                              | 89 +++++++++++++++++++++
 tools/testing/selftests/Makefile            |  1 +
 tools/testing/selftests/fpu/.gitignore      |  2 +
 tools/testing/selftests/fpu/Makefile        |  9 +++
 tools/testing/selftests/fpu/run_test_fpu.sh | 46 +++++++++++
 tools/testing/selftests/fpu/test_fpu.c      | 61 ++++++++++++++
 8 files changed, 239 insertions(+)
 create mode 100644 lib/test_fpu.c
 create mode 100644 tools/testing/selftests/fpu/.gitignore
 create mode 100644 tools/testing/selftests/fpu/Makefile
 create mode 100755 tools/testing/selftests/fpu/run_test_fpu.sh
 create mode 100644 tools/testing/selftests/fpu/test_fpu.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d74ac0fd6b2d..a1963a493920 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2308,6 +2308,17 @@ config TEST_HMM
 
 	  If unsure, say N.
 
+config TEST_FPU
+	tristate "Test floating point operations in kernel space"
+	depends on X86
+	help
+	  Enable this option to add /sys/kernel/debug/selftest_helpers/test_fpu
+	  which will trigger a sequence of floating point operations. This is used
+	  for self-testing floating point control register setting in
+	  kernel_fpu_begin().
+
+	  If unsure, say N.
+
 endif # RUNTIME_TESTING_MENU
 
 config MEMTEST
diff --git a/lib/Makefile b/lib/Makefile
index b1c42c10073b..2bb3164a0dbb 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -99,6 +99,26 @@ obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
 obj-$(CONFIG_TEST_LOCKUP) += test_lockup.o
 obj-$(CONFIG_TEST_HMM) += test_hmm.o
 
+#
+# CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns
+# off the generation of FPU/SSE* instructions for kernel proper but FPU_FLAGS
+# get appended last to CFLAGS and thus override those previous compiler options.
+#
+FPU_CFLAGS += -mhard-float -msse
+ifdef CONFIG_CC_IS_GCC
+  ifeq ($(call cc-ifversion, -lt, 0701, y), y)
+    # Stack alignment mismatch, proceed with caution.
+    # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
+    # (8B stack alignment).
+    FPU_CFLAGS += -mpreferred-stack-boundary=4
+  else
+    FPU_CFLAGS += -msse2
+  endif
+endif
+
+obj-$(CONFIG_TEST_FPU) += test_fpu.o
+CFLAGS_test_fpu.o += $(FPU_CFLAGS)
+
 obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
 
 obj-$(CONFIG_KUNIT) += kunit/
diff --git a/lib/test_fpu.c b/lib/test_fpu.c
new file mode 100644
index 000000000000..c33764aa3eb8
--- /dev/null
+++ b/lib/test_fpu.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test cases for using floating point operations inside a kernel module.
+ *
+ * This tests kernel_fpu_begin() and kernel_fpu_end() functions, especially
+ * when userland has modified the floating point control registers. The kernel
+ * state might depend on the state set by the userland thread that was active
+ * before a syscall.
+ *
+ * To facilitate the test, this module registers file
+ * /sys/kernel/debug/selftest_helpers/test_fpu, which when read causes a
+ * sequence of floating point operations. If the operations fail, either the
+ * read returns error status or the kernel crashes.
+ * If the operations succeed, the read returns "1\n".
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/debugfs.h>
+#include <asm/fpu/api.h>
+
+static int test_fpu(void)
+{
+	/*
+	 * This sequence of operations tests that rounding mode is
+	 * to nearest and that denormal numbers are supported.
+	 * Volatile variables are used to avoid compiler optimizing
+	 * the calculations away.
+	 */
+	volatile double a, b, c, d, e, f, g;
+
+	a = 4.0;
+	b = 1e-15;
+	c = 1e-310;
+
+	/* Sets precision flag */
+	d = a + b;
+
+	/* Result depends on rounding mode */
+	e = a + b / 2;
+
+	/* Denormal and very large values */
+	f = b / c;
+
+	/* Depends on denormal support */
+	g = a + c * f;
+
+	if (d > a && e > a && g > a)
+		return 0;
+	else
+		return -EINVAL;
+}
+
+static int test_fpu_get(void *data, u64 *val)
+{
+	int status = -EINVAL;
+
+	kernel_fpu_begin();
+	status = test_fpu();
+	kernel_fpu_end();
+
+	*val = 1;
+	return status;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(test_fpu_fops, test_fpu_get, NULL, "%lld\n");
+static struct dentry *selftest_dir;
+
+static int __init test_fpu_init(void)
+{
+	selftest_dir = debugfs_create_dir("selftest_helpers", NULL);
+	if (!selftest_dir)
+		return -ENOMEM;
+
+	debugfs_create_file("test_fpu", 0444, selftest_dir, NULL,
+			    &test_fpu_fops);
+
+	return 0;
+}
+
+static void __exit test_fpu_exit(void)
+{
+	debugfs_remove(selftest_dir);
+}
+
+module_init(test_fpu_init);
+module_exit(test_fpu_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 1195bd85af38..227ca78a5b7f 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -15,6 +15,7 @@ TARGETS += filesystems
 TARGETS += filesystems/binderfs
 TARGETS += filesystems/epoll
 TARGETS += firmware
+TARGETS += fpu
 TARGETS += ftrace
 TARGETS += futex
 TARGETS += gpio
diff --git a/tools/testing/selftests/fpu/.gitignore b/tools/testing/selftests/fpu/.gitignore
new file mode 100644
index 000000000000..d6d12ac1d9c3
--- /dev/null
+++ b/tools/testing/selftests/fpu/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0+
+test_fpu
diff --git a/tools/testing/selftests/fpu/Makefile b/tools/testing/selftests/fpu/Makefile
new file mode 100644
index 000000000000..ea62c176ede7
--- /dev/null
+++ b/tools/testing/selftests/fpu/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+LDLIBS := -lm
+
+TEST_GEN_PROGS := test_fpu
+
+TEST_PROGS := run_test_fpu.sh
+
+include ../lib.mk
diff --git a/tools/testing/selftests/fpu/run_test_fpu.sh b/tools/testing/selftests/fpu/run_test_fpu.sh
new file mode 100755
index 000000000000..d77be93ec139
--- /dev/null
+++ b/tools/testing/selftests/fpu/run_test_fpu.sh
@@ -0,0 +1,46 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Load kernel module for FPU tests
+
+uid=$(id -u)
+if [ $uid -ne 0 ]; then
+	echo "$0: Must be run as root"
+	exit 1
+fi
+
+if ! which modprobe > /dev/null 2>&1; then
+	echo "$0: You need modprobe installed"
+        exit 4
+fi
+
+if ! modinfo test_fpu > /dev/null 2>&1; then
+	echo "$0: You must have the following enabled in your kernel:"
+	echo "CONFIG_TEST_FPU=m"
+	exit 4
+fi
+
+NR_CPUS=$(getconf _NPROCESSORS_ONLN)
+if [ ! $NR_CPUS ]; then
+	NR_CPUS=1
+fi
+
+modprobe test_fpu
+
+if [ ! -e /sys/kernel/debug/selftest_helpers/test_fpu ]; then
+	mount -t debugfs none /sys/kernel/debug
+
+	if [ ! -e /sys/kernel/debug/selftest_helpers/test_fpu ]; then
+		echo "$0: Error mounting debugfs"
+		exit 4
+	fi
+fi
+
+echo "Running 1000 iterations on all CPUs... "
+for i in $(seq 1 1000); do
+	for c in $(seq 1 $NR_CPUS); do
+		./test_fpu &
+	done
+done
+
+rmmod test_fpu
diff --git a/tools/testing/selftests/fpu/test_fpu.c b/tools/testing/selftests/fpu/test_fpu.c
new file mode 100644
index 000000000000..200238522a9d
--- /dev/null
+++ b/tools/testing/selftests/fpu/test_fpu.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* This testcase operates with the test_fpu kernel driver.
+ * It modifies the FPU control register in user mode and calls the kernel
+ * module to perform floating point operations in the kernel. The control
+ * register value should be independent between kernel and user mode.
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <errno.h>
+#include <string.h>
+#include <fenv.h>
+#include <unistd.h>
+#include <fcntl.h>
+
+const char *test_fpu_path = "/sys/kernel/debug/selftest_helpers/test_fpu";
+
+int main(void)
+{
+	char dummy[1];
+	int fd = open(test_fpu_path, O_RDONLY);
+
+	if (fd < 0) {
+		printf("[SKIP]\tcan't access %s: %s\n",
+		       test_fpu_path, strerror(errno));
+		return 0;
+	}
+
+	if (read(fd, dummy, 1) < 0) {
+		printf("[FAIL]\taccess with default rounding mode failed\n");
+		return 1;
+	}
+
+	fesetround(FE_DOWNWARD);
+	if (read(fd, dummy, 1) < 0) {
+		printf("[FAIL]\taccess with downward rounding mode failed\n");
+		return 2;
+	}
+	if (fegetround() != FE_DOWNWARD) {
+		printf("[FAIL]\tusermode rounding mode clobbered\n");
+		return 3;
+	}
+
+	/* Note: the tests up to this point are quite safe and will only return
+	 * an error. But the exception mask setting can cause misbehaving kernel
+	 * to crash.
+	 */
+	feclearexcept(FE_ALL_EXCEPT);
+	feenableexcept(FE_ALL_EXCEPT);
+	if (read(fd, dummy, 1) < 0) {
+		printf("[FAIL]\taccess with fpu exceptions unmasked failed\n");
+		return 4;
+	}
+	if (fegetexcept() != FE_ALL_EXCEPT) {
+		printf("[FAIL]\tusermode fpu exception mask clobbered\n");
+		return 5;
+	}
+
+	printf("[OK]\ttest_fpu\n");
+	return 0;
+}
-- 
2.21.0

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Andy Lutomirski]

On Fri, Jun 19, 2020 at 10:41 AM Borislav Petkov <bp@alien8.de> wrote:
>
> From: Petteri Aimonen <jpa@git.mail.kapsi.fi>
>
> Add a selftest for the usage of FPU code in kernel mode.
>
> Currently only implemented for x86. In the future, kernel FPU testing
> could be unified between the different architectures supporting it.

Acked-by: Andy Lutomirski <luto@kernel.org>

except:

> +#
> +# CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns
> +# off the generation of FPU/SSE* instructions for kernel proper but FPU_FLAGS
> +# get appended last to CFLAGS and thus override those previous compiler options.
> +#
> +FPU_CFLAGS += -mhard-float -msse
> +ifdef CONFIG_CC_IS_GCC
> +  ifeq ($(call cc-ifversion, -lt, 0701, y), y)
> +    # Stack alignment mismatch, proceed with caution.
> +    # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
> +    # (8B stack alignment).
> +    FPU_CFLAGS += -mpreferred-stack-boundary=4
> +  else
> +    FPU_CFLAGS += -msse2
> +  endif
> +endif

This should be cc-option, not cc-ifversion, I think.  But maybe we
should consider dropping the problematic GCC version instead?  The old
GCC versions with stack alignment problems are seriously problematic
for x86 kernels, and I don't really trust kernels built with them.

Re: [PATCH 1/2] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Andy Lutomirski]

On Fri, Jun 19, 2020 at 10:41 AM Borislav Petkov <bp@alien8.de> wrote:
>
> From: Petteri Aimonen <jpa@git.mail.kapsi.fi>
>
> Previously, kernel floating point code would run with the MXCSR control
> register value last set by userland code by the thread that was active
> on the CPU core just before kernel call. This could affect calculation
> results if rounding mode was changed, or a crash if a FPU/SIMD exception
> was unmasked.
>
> Restore MXCSR to the kernel's default value.
>
>  [ bp: Carve out from a bigger patch by Petteri, add feature check, add
>    FNINIT call too (amluto). ]

Acked-by: Andy Lutomirski <luto@kernel.org>

but:

shouldn't kernel_fpu_begin() end with a barrier()?

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

Hi Borislav,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kselftest/next]
[also build test ERROR on tip/auto-latest linus/master v5.8-rc1 next-20200618]
[cannot apply to tip/x86/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Borislav-Petkov/x86-FPU-FPU-sanitization-for-in-kernel-use/20200620-014453
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
config: x86_64-allmodconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 487ca07fcc75d52755c9fe2ee05bcb3b6eeeec44)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__gtdf2" [lib/test_fpu.ko] undefined!
>> ERROR: modpost: "__divdf3" [lib/test_fpu.ko] undefined!
>> ERROR: modpost: "__muldf3" [lib/test_fpu.ko] undefined!
>> ERROR: modpost: "__adddf3" [lib/test_fpu.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75906 bytes --]

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Borislav Petkov]

On Sun, Jun 21, 2020 at 03:55:00AM +0800, kernel test robot wrote:
> Hi Borislav,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on kselftest/next]
> [also build test ERROR on tip/auto-latest linus/master v5.8-rc1 next-20200618]
> [cannot apply to tip/x86/core]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use  as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Borislav-Petkov/x86-FPU-FPU-sanitization-for-in-kernel-use/20200620-014453
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
> config: x86_64-allmodconfig (attached as .config)
> compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 487ca07fcc75d52755c9fe2ee05bcb3b6eeeec44)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install x86_64 cross compiling tool for clang build
>         # apt-get install binutils-x86-64-linux-gnu
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>, old ones prefixed by <<):
> 
> >> ERROR: modpost: "__gtdf2" [lib/test_fpu.ko] undefined!
> >> ERROR: modpost: "__divdf3" [lib/test_fpu.ko] undefined!
> >> ERROR: modpost: "__muldf3" [lib/test_fpu.ko] undefined!
> >> ERROR: modpost: "__adddf3" [lib/test_fpu.ko] undefined!

Hmm, the point here is to actually have SSE* code in a kernel module for
testing. And gcc does the right thing, see below.

LLVM folks, how can I make llvm not call those library functions and
actually spit out SSE* code?

Full build command is:

  /mnt/smr/share/src/llvm/tc-build/install/bin/clang-11 -Wp,-MMD,lib/.test_fpu.o.d  -nostdinc -isystem /mnt/smr/share/src/llvm/tc-build/install/lib/clang/11.0.0/include -I./arch/x86/include -I./arch/x86/include/generated  -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -Qunused-arguments -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Wno-format-security -std=gnu89 -no-integrated-as -Werror=unknown-warning-option -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -m64 -mno-80387 -mstack-alignment=8 -march=k8 -mno-red-zone -mcmodel=kernel -Wno-sign-compare -fno-asynchronous-unwind-tables -mretpoline-external-thunk -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector -Wno-format-invalid-specifier -Wno-gnu -mno-global-merge -Wno-unused-const-variable -g -DCC_USING_FENTRY -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wno-array-bounds -fno-strict-overflow -fno-merge-all-constants -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -fmacro-prefix-map=./= -fcf-protection=none -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-format-zero-length -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -mhard-float -msse  -DMODULE  -DKBUILD_BASENAME='"test_fpu"' -DKBUILD_MODNAME='"test_fpu"' -c -o lib/test_fpu.o lib/test_fpu.c

Thx.

--- /tmp/test_fpu.s.gcc	2020-06-22 18:51:25.828615337 +0200
+++ /tmp/test_fpu.s.llvm-11	2020-06-22 18:50:35.080614534 +0200
@@ -1,74 +1,49 @@
-test_fpu_get:
-	pushq	%r12	#
-	pushq	%rbx	#
-	movq	%rsi, %rbx	# tmp119, val
-	subq	$56, %rsp	#,
-# lib/test_fpu.c:58: 	kernel_fpu_begin();
-	call	kernel_fpu_begin	#
-# lib/test_fpu.c:32: 	a = 4.0;
-	movlpd	.LC1(%rip), %xmm0	#, tmp106
-	movsd	%xmm0, (%rsp)	# tmp106, a
-# lib/test_fpu.c:33: 	b = 1e-15;
-	movlpd	.LC2(%rip), %xmm0	#, tmp107
-	movsd	%xmm0, 8(%rsp)	# tmp107, b
-# lib/test_fpu.c:34: 	c = 1e-310;
-	movlpd	.LC3(%rip), %xmm0	#, tmp108
-	movsd	%xmm0, 16(%rsp)	# tmp108, c
-# lib/test_fpu.c:37: 	d = a + b;
-	movlpd	(%rsp), %xmm0	# a, a.2_8
-	movlpd	8(%rsp), %xmm1	# b, b.3_9
-	addsd	%xmm1, %xmm0	# b.3_9, _10
-# lib/test_fpu.c:37: 	d = a + b;
-	movsd	%xmm0, 24(%rsp)	# _10, d
-# lib/test_fpu.c:40: 	e = a + b / 2;
-	movlpd	8(%rsp), %xmm0	# b, b.4_11
-# lib/test_fpu.c:40: 	e = a + b / 2;
-	movlpd	(%rsp), %xmm1	# a, a.5_13
-# lib/test_fpu.c:40: 	e = a + b / 2;
-	mulsd	.LC4(%rip), %xmm0	#, tmp109
-# lib/test_fpu.c:40: 	e = a + b / 2;
-	addsd	%xmm1, %xmm0	# a.5_13, _14
-# lib/test_fpu.c:40: 	e = a + b / 2;
-	movsd	%xmm0, 32(%rsp)	# _14, e
-# lib/test_fpu.c:43: 	f = b / c;
-	movlpd	8(%rsp), %xmm0	# b, b.6_15
-	movlpd	16(%rsp), %xmm1	# c, c.7_16
-	divsd	%xmm1, %xmm0	# c.7_16, _17
-# lib/test_fpu.c:43: 	f = b / c;
-	movsd	%xmm0, 40(%rsp)	# _17, f
-# lib/test_fpu.c:46: 	g = a + c * f;
-	movlpd	16(%rsp), %xmm0	# c, c.8_18
-	movlpd	40(%rsp), %xmm2	# f, f.9_19
-# lib/test_fpu.c:46: 	g = a + c * f;
-	movlpd	(%rsp), %xmm1	# a, a.10_21
-# lib/test_fpu.c:46: 	g = a + c * f;
-	mulsd	%xmm2, %xmm0	# f.9_19, tmp111
-# lib/test_fpu.c:46: 	g = a + c * f;
-	addsd	%xmm1, %xmm0	# a.10_21, _22
-# lib/test_fpu.c:46: 	g = a + c * f;
-	movsd	%xmm0, 48(%rsp)	# _22, g
-# lib/test_fpu.c:48: 	if (d > a && e > a && g > a)
-	movlpd	24(%rsp), %xmm1	# d, d.11_23
-	movlpd	(%rsp), %xmm0	# a, a.12_24
-# lib/test_fpu.c:48: 	if (d > a && e > a && g > a)
-	comisd	%xmm0, %xmm1	# a.12_24, d.11_23
-	jbe	.L15	#,
-# lib/test_fpu.c:48: 	if (d > a && e > a && g > a)
-	movlpd	32(%rsp), %xmm1	# e, e.13_25
-	movlpd	(%rsp), %xmm0	# a, a.14_26
-# lib/test_fpu.c:48: 	if (d > a && e > a && g > a)
-	comisd	%xmm0, %xmm1	# a.14_26, e.13_25
-	jbe	.L15	#,
-# lib/test_fpu.c:48: 	if (d > a && e > a && g > a)
-	movlpd	48(%rsp), %xmm1	# g, g.15_27
-# lib/test_fpu.c:51: 		return -EINVAL;
-	movl	$0, %r12d	#, tmp117
-	movl	$-22, %eax	#, tmp118
-# lib/test_fpu.c:48: 	if (d > a && e > a && g > a)
-	movlpd	(%rsp), %xmm0	# a, a.16_28
-# lib/test_fpu.c:51: 		return -EINVAL;
-	comisd	%xmm0, %xmm1	# a.16_28, g.15_27
-	cmovbe	%eax, %r12d	# tmp117,, tmp118, <retval>
-.L4:
-# lib/test_fpu.c:60: 	kernel_fpu_end();
-	call	kernel_fpu_end	#
+test_fpu_get:                           # @test_fpu_get
+# %bb.0:
+	pushq	%rbp
+	pushq	%rbx
+	subq	$56, %rsp
+	movq	%rsi, %rbx
+	callq	kernel_fpu_begin
+	movabsq	$4616189618054758400, %rax # imm = 0x4010000000000000
+	movq	%rax, (%rsp)
+	movabsq	$4382569440205035030, %rax # imm = 0x3CD203AF9EE75616
+	movq	%rax, 8(%rsp)
+	movabsq	$20240225330731, %rax   # imm = 0x12688B70E62B
+	movq	%rax, 16(%rsp)
+	movq	(%rsp), %rdi
+	movq	8(%rsp), %rsi
+	callq	__adddf3
+	movq	%rax, 48(%rsp)
+	movq	(%rsp), %rbp
+	movq	8(%rsp), %rdi
+	movabsq	$4602678819172646912, %rsi # imm = 0x3FE0000000000000
+	callq	__muldf3
+	movq	%rbp, %rdi
+	movq	%rax, %rsi
+	callq	__adddf3
+	movq	%rax, 40(%rsp)
+	movq	8(%rsp), %rdi
+	movq	16(%rsp), %rsi
+	callq	__divdf3
+	movq	%rax, 32(%rsp)
+	movq	(%rsp), %rbp
+	movq	16(%rsp), %rdi
+	movq	32(%rsp), %rsi
+	callq	__muldf3
+	movq	%rbp, %rdi
+	movq	%rax, %rsi
+	callq	__adddf3
+	movq	%rax, 24(%rsp)
+	movq	48(%rsp), %rdi
+	movq	(%rsp), %rsi
+	callq	__gtdf2
+	testl	%eax, %eax
+	jle	.LBB3_3
+# %bb.1:
+	movq	40(%rsp), %rdi
+	movq	(%rsp), %rsi
+	callq	__gtdf2
+	testl	%eax, %eax
+	jle	.LBB3_3
+# %bb.2:

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/2] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Borislav Petkov]

On Fri, Jun 19, 2020 at 11:01:44AM -0700, Andy Lutomirski wrote:
> On Fri, Jun 19, 2020 at 10:41 AM Borislav Petkov <bp@alien8.de> wrote:
> >
> > From: Petteri Aimonen <jpa@git.mail.kapsi.fi>
> >
> > Previously, kernel floating point code would run with the MXCSR control
> > register value last set by userland code by the thread that was active
> > on the CPU core just before kernel call. This could affect calculation
> > results if rounding mode was changed, or a crash if a FPU/SIMD exception
> > was unmasked.
> >
> > Restore MXCSR to the kernel's default value.
> >
> >  [ bp: Carve out from a bigger patch by Petteri, add feature check, add
> >    FNINIT call too (amluto). ]
> 
> Acked-by: Andy Lutomirski <luto@kernel.org>
> 
> but:
> 
> shouldn't kernel_fpu_begin() end with a barrier()?

the "fninit" thing is already asm volatile or do you want the explicit
memory clobber of barrier?

If so, why?

The LDMXCSR and FNINIT have effect only on hardware state...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Borislav Petkov]

On Fri, Jun 19, 2020 at 11:00:28AM -0700, Andy Lutomirski wrote:
> This should be cc-option, not cc-ifversion, I think.

Why?

> But maybe we should consider dropping the problematic GCC version
> instead? The old GCC versions with stack alignment problems are
> seriously problematic for x86 kernels, and I don't really trust
> kernels built with them.

Can't - we just upped min gcc version to 4.8:

5429ef62bcf3 ("compiler/gcc: Raise minimum GCC version for kernel builds to 4.8")

I mean we could but everytime we do that, it is all a big bikeshedding
discussion. Even though almost everyone is using gcc9 or so to build...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Nick Desaulniers]

On Mon, Jun 22, 2020 at 10:04 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Sun, Jun 21, 2020 at 03:55:00AM +0800, kernel test robot wrote:
> > Hi Borislav,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on kselftest/next]
> > [also build test ERROR on tip/auto-latest linus/master v5.8-rc1 next-20200618]
> > [cannot apply to tip/x86/core]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use  as documented in
> > https://git-scm.com/docs/git-format-patch]
> >
> > url:    https://github.com/0day-ci/linux/commits/Borislav-Petkov/x86-FPU-FPU-sanitization-for-in-kernel-use/20200620-014453
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
> > config: x86_64-allmodconfig (attached as .config)
> > compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 487ca07fcc75d52755c9fe2ee05bcb3b6eeeec44)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # install x86_64 cross compiling tool for clang build
> >         # apt-get install binutils-x86-64-linux-gnu
> >         # save the attached .config to linux build tree
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All errors (new ones prefixed by >>, old ones prefixed by <<):
> >
> > >> ERROR: modpost: "__gtdf2" [lib/test_fpu.ko] undefined!
> > >> ERROR: modpost: "__divdf3" [lib/test_fpu.ko] undefined!
> > >> ERROR: modpost: "__muldf3" [lib/test_fpu.ko] undefined!
> > >> ERROR: modpost: "__adddf3" [lib/test_fpu.ko] undefined!
>
> Hmm, the point here is to actually have SSE* code in a kernel module for
> testing. And gcc does the right thing, see below.
>
> LLVM folks, how can I make llvm not call those library functions and
> actually spit out SSE* code?
>
> Full build command is:
>
>   /mnt/smr/share/src/llvm/tc-build/install/bin/clang-11 -Wp,-MMD,lib/.test_fpu.o.d  -nostdinc -isystem /mnt/smr/share/src/llvm/tc-build/install/lib/clang/11.0.0/include -I./arch/x86/include -I./arch/x86/include/generated  -I./include -I./arch/x86/include/uapi -I./arch/x86/include/generated/uapi -I./include/uapi -I./include/generated/uapi -include ./include/linux/kconfig.h -include ./include/linux/compiler_types.h -D__KERNEL__ -Qunused-arguments -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE -Werror=implicit-function-declaration -Werror=implicit-int -Wno-format-security -std=gnu89 -no-integrated-as -Werror=unknown-warning-option -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -m64 -mno-80387 -mstack-alignment=8 -march=k8 -mno-red-zone -mcmodel=kernel -Wno-sign-compare -fno-asynchronous-unwind-tables -mretpoline-external-thunk -fno-delete-null-pointer-checks -Wno-frame-address -Wno-address-of-packed-member -O2 -Wframe-larger-than=2048 -fstack-protector -Wno-format-invalid-specifier -Wno-gnu -mno-global-merge -Wno-unused-const-variable -g -DCC_USING_FENTRY -Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Wno-array-bounds -fno-strict-overflow -fno-merge-all-constants -fno-stack-check -Werror=date-time -Werror=incompatible-pointer-types -fmacro-prefix-map=./= -fcf-protection=none -Wno-initializer-overrides -Wno-format -Wno-sign-compare -Wno-format-zero-length -Wno-pointer-to-enum-cast -Wno-tautological-constant-out-of-range-compare -mhard-float -msse  -DMODULE  -DKBUILD_BASENAME='"test_fpu"' -DKBUILD_MODNAME='"test_fpu"' -c -o lib/test_fpu.o lib/test_fpu.c

Add -msse2.  See also:
commit e8a170ff9a35 ("drm/amdgpu: enable -msse2 for GCC 7.1+ users")
potentially relevant, though less so:
commit 00db297106e8 ("drm/amdgpu: fix stack alignment ABI mismatch for
GCC 7.1+")
commit c868868f6b6a ("drm/amdgpu: fix stack alignment ABI mismatch for Clang")
and may your stacks always share the same alignment.

>
> Thx.
>
> --- /tmp/test_fpu.s.gcc 2020-06-22 18:51:25.828615337 +0200
> +++ /tmp/test_fpu.s.llvm-11     2020-06-22 18:50:35.080614534 +0200
> @@ -1,74 +1,49 @@
> -test_fpu_get:
> -       pushq   %r12    #
> -       pushq   %rbx    #
> -       movq    %rsi, %rbx      # tmp119, val
> -       subq    $56, %rsp       #,
> -# lib/test_fpu.c:58:   kernel_fpu_begin();
> -       call    kernel_fpu_begin        #
> -# lib/test_fpu.c:32:   a = 4.0;
> -       movlpd  .LC1(%rip), %xmm0       #, tmp106
> -       movsd   %xmm0, (%rsp)   # tmp106, a
> -# lib/test_fpu.c:33:   b = 1e-15;
> -       movlpd  .LC2(%rip), %xmm0       #, tmp107
> -       movsd   %xmm0, 8(%rsp)  # tmp107, b
> -# lib/test_fpu.c:34:   c = 1e-310;
> -       movlpd  .LC3(%rip), %xmm0       #, tmp108
> -       movsd   %xmm0, 16(%rsp) # tmp108, c
> -# lib/test_fpu.c:37:   d = a + b;
> -       movlpd  (%rsp), %xmm0   # a, a.2_8
> -       movlpd  8(%rsp), %xmm1  # b, b.3_9
> -       addsd   %xmm1, %xmm0    # b.3_9, _10
> -# lib/test_fpu.c:37:   d = a + b;
> -       movsd   %xmm0, 24(%rsp) # _10, d
> -# lib/test_fpu.c:40:   e = a + b / 2;
> -       movlpd  8(%rsp), %xmm0  # b, b.4_11
> -# lib/test_fpu.c:40:   e = a + b / 2;
> -       movlpd  (%rsp), %xmm1   # a, a.5_13
> -# lib/test_fpu.c:40:   e = a + b / 2;
> -       mulsd   .LC4(%rip), %xmm0       #, tmp109
> -# lib/test_fpu.c:40:   e = a + b / 2;
> -       addsd   %xmm1, %xmm0    # a.5_13, _14
> -# lib/test_fpu.c:40:   e = a + b / 2;
> -       movsd   %xmm0, 32(%rsp) # _14, e
> -# lib/test_fpu.c:43:   f = b / c;
> -       movlpd  8(%rsp), %xmm0  # b, b.6_15
> -       movlpd  16(%rsp), %xmm1 # c, c.7_16
> -       divsd   %xmm1, %xmm0    # c.7_16, _17
> -# lib/test_fpu.c:43:   f = b / c;
> -       movsd   %xmm0, 40(%rsp) # _17, f
> -# lib/test_fpu.c:46:   g = a + c * f;
> -       movlpd  16(%rsp), %xmm0 # c, c.8_18
> -       movlpd  40(%rsp), %xmm2 # f, f.9_19
> -# lib/test_fpu.c:46:   g = a + c * f;
> -       movlpd  (%rsp), %xmm1   # a, a.10_21
> -# lib/test_fpu.c:46:   g = a + c * f;
> -       mulsd   %xmm2, %xmm0    # f.9_19, tmp111
> -# lib/test_fpu.c:46:   g = a + c * f;
> -       addsd   %xmm1, %xmm0    # a.10_21, _22
> -# lib/test_fpu.c:46:   g = a + c * f;
> -       movsd   %xmm0, 48(%rsp) # _22, g
> -# lib/test_fpu.c:48:   if (d > a && e > a && g > a)
> -       movlpd  24(%rsp), %xmm1 # d, d.11_23
> -       movlpd  (%rsp), %xmm0   # a, a.12_24
> -# lib/test_fpu.c:48:   if (d > a && e > a && g > a)
> -       comisd  %xmm0, %xmm1    # a.12_24, d.11_23
> -       jbe     .L15    #,
> -# lib/test_fpu.c:48:   if (d > a && e > a && g > a)
> -       movlpd  32(%rsp), %xmm1 # e, e.13_25
> -       movlpd  (%rsp), %xmm0   # a, a.14_26
> -# lib/test_fpu.c:48:   if (d > a && e > a && g > a)
> -       comisd  %xmm0, %xmm1    # a.14_26, e.13_25
> -       jbe     .L15    #,
> -# lib/test_fpu.c:48:   if (d > a && e > a && g > a)
> -       movlpd  48(%rsp), %xmm1 # g, g.15_27
> -# lib/test_fpu.c:51:           return -EINVAL;
> -       movl    $0, %r12d       #, tmp117
> -       movl    $-22, %eax      #, tmp118
> -# lib/test_fpu.c:48:   if (d > a && e > a && g > a)
> -       movlpd  (%rsp), %xmm0   # a, a.16_28
> -# lib/test_fpu.c:51:           return -EINVAL;
> -       comisd  %xmm0, %xmm1    # a.16_28, g.15_27
> -       cmovbe  %eax, %r12d     # tmp117,, tmp118, <retval>
> -.L4:
> -# lib/test_fpu.c:60:   kernel_fpu_end();
> -       call    kernel_fpu_end  #
> +test_fpu_get:                           # @test_fpu_get
> +# %bb.0:
> +       pushq   %rbp
> +       pushq   %rbx
> +       subq    $56, %rsp
> +       movq    %rsi, %rbx
> +       callq   kernel_fpu_begin
> +       movabsq $4616189618054758400, %rax # imm = 0x4010000000000000
> +       movq    %rax, (%rsp)
> +       movabsq $4382569440205035030, %rax # imm = 0x3CD203AF9EE75616
> +       movq    %rax, 8(%rsp)
> +       movabsq $20240225330731, %rax   # imm = 0x12688B70E62B
> +       movq    %rax, 16(%rsp)
> +       movq    (%rsp), %rdi
> +       movq    8(%rsp), %rsi
> +       callq   __adddf3
> +       movq    %rax, 48(%rsp)
> +       movq    (%rsp), %rbp
> +       movq    8(%rsp), %rdi
> +       movabsq $4602678819172646912, %rsi # imm = 0x3FE0000000000000
> +       callq   __muldf3
> +       movq    %rbp, %rdi
> +       movq    %rax, %rsi
> +       callq   __adddf3
> +       movq    %rax, 40(%rsp)
> +       movq    8(%rsp), %rdi
> +       movq    16(%rsp), %rsi
> +       callq   __divdf3
> +       movq    %rax, 32(%rsp)
> +       movq    (%rsp), %rbp
> +       movq    16(%rsp), %rdi
> +       movq    32(%rsp), %rsi
> +       callq   __muldf3
> +       movq    %rbp, %rdi
> +       movq    %rax, %rsi
> +       callq   __adddf3
> +       movq    %rax, 24(%rsp)
> +       movq    48(%rsp), %rdi
> +       movq    (%rsp), %rsi
> +       callq   __gtdf2
> +       testl   %eax, %eax
> +       jle     .LBB3_3
> +# %bb.1:
> +       movq    40(%rsp), %rdi
> +       movq    (%rsp), %rsi
> +       callq   __gtdf2
> +       testl   %eax, %eax
> +       jle     .LBB3_3
> +# %bb.2:
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200622170446.GG32200%40zn.tnic.



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Andy Lutomirski]

On Mon, Jun 22, 2020 at 10:12 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Fri, Jun 19, 2020 at 11:00:28AM -0700, Andy Lutomirski wrote:
> > This should be cc-option, not cc-ifversion, I think.
>
> Why?

For all the ridiculous distro gcc versions out there.  Also, it seems
less fragile, since it tests for what you actually care about

>
> > But maybe we should consider dropping the problematic GCC version
> > instead? The old GCC versions with stack alignment problems are
> > seriously problematic for x86 kernels, and I don't really trust
> > kernels built with them.
>
> Can't - we just upped min gcc version to 4.8:
>
> 5429ef62bcf3 ("compiler/gcc: Raise minimum GCC version for kernel builds to 4.8")
>
> I mean we could but everytime we do that, it is all a big bikeshedding
> discussion. Even though almost everyone is using gcc9 or so to build...

Phooey.

>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Andy Lutomirski]

On Mon, Jun 22, 2020 at 10:09 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Fri, Jun 19, 2020 at 11:01:44AM -0700, Andy Lutomirski wrote:
> > On Fri, Jun 19, 2020 at 10:41 AM Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > From: Petteri Aimonen <jpa@git.mail.kapsi.fi>
> > >
> > > Previously, kernel floating point code would run with the MXCSR control
> > > register value last set by userland code by the thread that was active
> > > on the CPU core just before kernel call. This could affect calculation
> > > results if rounding mode was changed, or a crash if a FPU/SIMD exception
> > > was unmasked.
> > >
> > > Restore MXCSR to the kernel's default value.
> > >
> > >  [ bp: Carve out from a bigger patch by Petteri, add feature check, add
> > >    FNINIT call too (amluto). ]
> >
> > Acked-by: Andy Lutomirski <luto@kernel.org>
> >
> > but:
> >
> > shouldn't kernel_fpu_begin() end with a barrier()?
>
> the "fninit" thing is already asm volatile or do you want the explicit
> memory clobber of barrier?
>
> If so, why?
>
> The LDMXCSR and FNINIT have effect only on hardware state...
>

Suppose you do:

double x = 1.0;

kernel_fpu_begin();

x += 2.0;

We want to make sure that GCC puts things in the right order.  I
suppose that even a memory clobber is insufficient here, though.

Re: [PATCH 1/2] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Petteri Aimonen]

> We want to make sure that GCC puts things in the right order.  I
> suppose that even a memory clobber is insufficient here, though.

amdgpu worked around that by using a noinline function:
https://github.com/torvalds/linux/commit/59dfb0c64d3853d20dc84f4561f28d4f5a2ddc7d#diff-a82b8ab0e6b4f9abfd3344d1427d765f

If there is something that would help, it would have to be an
attribute added to kernel_fpu_begin() in the header. Any barriers
inside the function will be invisible to code generation in other
compilation units.

--
Petteri

Re: [PATCH 1/2] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Dave Hansen]

On 6/22/20 11:33 AM, Andy Lutomirski wrote:
> Suppose you do:
> 
> double x = 1.0;
> 
> kernel_fpu_begin();
> 
> x += 2.0;
> 
> We want to make sure that GCC puts things in the right order.  I
> suppose that even a memory clobber is insufficient here, though.

Even with CONFIG_PREEMPT disabled, we still have:

	#define preempt_disable()                       barrier()

I don't see us supporting preemptible kernel_fpu regions any time soon,
so shouldn't this be sufficient now and for a long time?

Re: [PATCH 1/2] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Andy Lutomirski]

On Mon, Jun 22, 2020 at 11:38 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 6/22/20 11:33 AM, Andy Lutomirski wrote:
> > Suppose you do:
> >
> > double x = 1.0;
> >
> > kernel_fpu_begin();
> >
> > x += 2.0;
> >
> > We want to make sure that GCC puts things in the right order.  I
> > suppose that even a memory clobber is insufficient here, though.
>
> Even with CONFIG_PREEMPT disabled, we still have:
>
>         #define preempt_disable()                       barrier()
>
> I don't see us supporting preemptible kernel_fpu regions any time soon,
> so shouldn't this be sufficient now and for a long time?

That's on the wrong end of the function.  It'sL

preempt_disable();
LDMXCSR;
<-- some kind of barrier here might be nice

--Andy

Re: [PATCH 1/2] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()

[Borislav Petkov]

On Mon, Jun 22, 2020 at 11:40:38AM -0700, Andy Lutomirski wrote:
> On Mon, Jun 22, 2020 at 11:38 AM Dave Hansen <dave.hansen@intel.com> wrote:
> >
> > On 6/22/20 11:33 AM, Andy Lutomirski wrote:
> > > Suppose you do:
> > >
> > > double x = 1.0;
> > >
> > > kernel_fpu_begin();
> > >
> > > x += 2.0;
> > >
> > > We want to make sure that GCC puts things in the right order.  I
> > > suppose that even a memory clobber is insufficient here, though.
> >
> > Even with CONFIG_PREEMPT disabled, we still have:
> >
> >         #define preempt_disable()                       barrier()
> >
> > I don't see us supporting preemptible kernel_fpu regions any time soon,
> > so shouldn't this be sufficient now and for a long time?
> 
> That's on the wrong end of the function.  It'sL
> 
> preempt_disable();
> LDMXCSR;
> <-- some kind of barrier here might be nice

For what? LDMXCSR loads the MXCSR *hardware* register with a u32 from
memory.

In any case, I'm still unconvinced we should preemptively fix issues
that might potentially happen when someone uses FPU in the kernel. And
even if, this doesn't belong in this patch but in a separate one with a
commit message explaining what is being fixed.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Borislav Petkov]

On Mon, Jun 22, 2020 at 11:26:50AM -0700, Andy Lutomirski wrote:
> For all the ridiculous distro gcc versions out there.  Also, it seems
> less fragile, since it tests for what you actually care about

Let's cross that bridge when we get there.

Do you know of an actual compiler not supporting
-mpreferred-stack-boundary?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Borislav Petkov]

On Mon, Jun 22, 2020 at 12:37:12PM -0700, Andy Lutomirski wrote:
> It’s this whole mess:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53383

It really is a mess because our option detection doesn't really work. I did:

ifdef CONFIG_CC_IS_GCC
  ifeq ($(call cc-option-yn,-mpreferred-stack-boundary=3),y)
        dummy := $(error "gcc supports -mpreferred-stack-boundary=3")
  endif
  ...

and doing:

$ make CC=gcc-4.8 HOSTCC=gcc-4.8 V=1 lib/test_fpu.ko

gives

lib/Makefile:110: *** "gcc supports -mpreferred-stack-boundary=3".  Stop.

That same compiler, however, says:

$ gcc-4.8 -mpreferred-stack-boundary=3 -c /tmp/foo.c
/tmp/foo.c:1:0: error: -mpreferred-stack-boundary=3 is not between 4 and 12

so I need to dig deep into cc-option* fun.

Or, I can be lazy and simply cut off all compiler versions to >= 8
and be done with it. If people wanna run selftests, they can upgrade
toolchain too...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Andy Lutomirski]

On Tue, Jun 23, 2020 at 3:28 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Jun 22, 2020 at 12:37:12PM -0700, Andy Lutomirski wrote:
> > It’s this whole mess:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53383
>
> It really is a mess because our option detection doesn't really work. I did:
>
> ifdef CONFIG_CC_IS_GCC
>   ifeq ($(call cc-option-yn,-mpreferred-stack-boundary=3),y)
>         dummy := $(error "gcc supports -mpreferred-stack-boundary=3")
>   endif
>   ...
>
> and doing:
>
> $ make CC=gcc-4.8 HOSTCC=gcc-4.8 V=1 lib/test_fpu.ko
>
> gives
>
> lib/Makefile:110: *** "gcc supports -mpreferred-stack-boundary=3".  Stop.
>
> That same compiler, however, says:
>
> $ gcc-4.8 -mpreferred-stack-boundary=3 -c /tmp/foo.c
> /tmp/foo.c:1:0: error: -mpreferred-stack-boundary=3 is not between 4 and 12
>
> so I need to dig deep into cc-option* fun.

See that same atrocious bug report.  It's the insane interaction
between -mno-sse2 and -mpreferred-stack-boundary.  So you need to
cc-option them both?  Or just stick with a compiler version check, I
guess.

--Andy

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Borislav Petkov]

On Tue, Jun 23, 2020 at 11:22:53AM -0700, Andy Lutomirski wrote:
> See that same atrocious bug report.  It's the insane interaction
> between -mno-sse2 and -mpreferred-stack-boundary.  So you need to
> cc-option them both?  Or just stick with a compiler version check, I
> guess.

Yes, it was the interaction. This below seems to work. Note the "-msse"
in the first argument of cc-option which causes the compiler to error
out with

/dev/null:1:0: error: -mpreferred-stack-boundary=3 is not between 4 and 12

Adding Nick for the clang side.

@Nick: I'm simply going to add -msse2 with cc-option.

Anyway, lemme test this thing a bit more.

Thx.

---

# CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns
# off the generation of FPU/SSE* instructions for kernel proper but FPU_FLAGS
# get appended last to CFLAGS and thus override those previous compiler options.
#
FPU_CFLAGS := -mhard-float -msse
FPU_CFLAGS += $(call cc-option,-msse2,)
ifdef CONFIG_CC_IS_GCC
# Stack alignment mismatch, proceed with caution.
# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
# (8B stack alignment).
# See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53383
FPU_CFLAGS += $(call cc-option,-msse -mpreferred-stack-boundary=3,-mpreferred-stack-boundary=4)
endif

obj-$(CONFIG_TEST_FPU) += test_fpu.o
CFLAGS_test_fpu.o += $(FPU_CFLAGS)

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Nick Desaulniers]

On Tue, Jun 23, 2020 at 11:47 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Jun 23, 2020 at 11:22:53AM -0700, Andy Lutomirski wrote:
> > See that same atrocious bug report.  It's the insane interaction
> > between -mno-sse2 and -mpreferred-stack-boundary.  So you need to
> > cc-option them both?  Or just stick with a compiler version check, I
> > guess.

Yeah, IIRC, this was fixed in GCC 7.1. See also
c868868f6b6a5272350781f9a19b3a5ba1c00b02
00db297106e81770e7c4319014a67896053b5a22 (this one for details about GCC 7.1)
e8a170ff9a3576730e43c0dbdd27b7cd3dc56848

>
> Yes, it was the interaction. This below seems to work. Note the "-msse"
> in the first argument of cc-option which causes the compiler to error
> out with
>
> /dev/null:1:0: error: -mpreferred-stack-boundary=3 is not between 4 and 12
>
> Adding Nick for the clang side.
>
> @Nick: I'm simply going to add -msse2 with cc-option.

You already have a conditional below for CC_IS_GCC; just add an else
and unconditionally add -msse2.  You *should* use -msse2 for GCC 7.1+
IMO.

Note that Clang has -mstack-alignment=8 whereas GCC has
-mpreferred-stack-boundary=3.  (Clang is a value in bytes, GCC is 2^X
bytes)

I recommend a version check for GCC < 7.1, or simply disabling the
self test if the version of GCC used is older than 7.1.  Mixing stack
alignments is a recipe for GPF with SSE when the caller has a smaller
stack alignment than the callee and is also unspecified behavior.  It
might work today, but as soon as someone perturbs the stack of the
call chain, you'll likely see a GPF due to non-16B aligned stack when
using SSE with stack operands in the caller.

>
> Anyway, lemme test this thing a bit more.
>
> Thx.
>
> ---
>
> # CFLAGS for compiling floating point code inside the kernel. x86/Makefile turns
> # off the generation of FPU/SSE* instructions for kernel proper but FPU_FLAGS
> # get appended last to CFLAGS and thus override those previous compiler options.
> #
> FPU_CFLAGS := -mhard-float -msse
> FPU_CFLAGS += $(call cc-option,-msse2,)
> ifdef CONFIG_CC_IS_GCC
> # Stack alignment mismatch, proceed with caution.
> # GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
> # (8B stack alignment).

^ looks familiar ;)

> # See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53383
> FPU_CFLAGS += $(call cc-option,-msse -mpreferred-stack-boundary=3,-mpreferred-stack-boundary=4)
> endif
>
> obj-$(CONFIG_TEST_FPU) += test_fpu.o
> CFLAGS_test_fpu.o += $(FPU_CFLAGS)
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Borislav Petkov]

On Tue, Jun 23, 2020 at 12:07:13PM -0700, Nick Desaulniers wrote:
> You already have a conditional below for CC_IS_GCC; just add an else
> and unconditionally add -msse2.  You *should* use -msse2 for GCC 7.1+
> IMO.

Why if one can write it more compact with cc-option?

FPU_CFLAGS += $(call cc-option,-msse2,)

> Note that Clang has -mstack-alignment=8 whereas GCC has
> -mpreferred-stack-boundary=3.  (Clang is a value in bytes, GCC is 2^X
> bytes)

Sure, the stack boundary setting is GCC only.

> I recommend a version check for GCC < 7.1, or simply disabling the
> self test if the version of GCC used is older than 7.1.

See Andy's suggestion upthread.

And I agree too that using cc-option is better than simply tying it to a
compiler version. Who knows what compiler has what backported. In such
cases a version number means nothing.

> ^ looks familiar ;)

It has been pasted around the kernel, I came to realize today. :-)

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Nick Desaulniers]

On Tue, Jun 23, 2020 at 12:13 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Jun 23, 2020 at 12:07:13PM -0700, Nick Desaulniers wrote:
> > You already have a conditional below for CC_IS_GCC; just add an else
> > and unconditionally add -msse2.  You *should* use -msse2 for GCC 7.1+
> > IMO.
>
> Why if one can write it more compact with cc-option?
>
> FPU_CFLAGS += $(call cc-option,-msse2,)

This will always be true though, right? For both compilers.  So why
test for it via cc-option when you can just do:

FPU_CFLAGS += -msse2

Though we might only want -msse2 if CC_IS_CLAG...I don't remember now
if GCC will select SSE instructions that require 16B operands at
-msse2, which will be problematic for the -mpreferred-stack-boundary=4
case.

My point was more so about avoiding needless cc-option checks when
they're tautological.

> > I recommend a version check for GCC < 7.1, or simply disabling the
> > self test if the version of GCC used is older than 7.1.
>
> See Andy's suggestion upthread.

Thread for other travelers:
https://lore.kernel.org/lkml/CALCETrXXzt8WZMs3dsReCJ5wdF3zhxFmUtGnmdCgV7_exFUKKQ@mail.gmail.com/

When Andy says "consider dropping the problematic GCC version" I
wonder if it was meant *just for this selftest* as I suggested, or
outright (which is untenable IMO, as it's a large jump to GCC 7.1+).

> And I agree too that using cc-option is better than simply tying it to a
> compiler version.

I agree that will differentiate better than a version check; but it's
still dangerous IMO to mix and match stack alignments.

> Who knows what compiler has what backported. In such
> cases a version number means nothing.

I'm not sure I agree, but I'll take feature detection any day over
version detection.

>
> > ^ looks familiar ;)
>
> It has been pasted around the kernel, I came to realize today. :-)

Guilty, your honor. :P

Maybe the feature test should be copy+pasta'd to those other places in
the kernel, rather than the version check?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 2/2] selftests/fpu: Add an FPU selftest

[Borislav Petkov]

On Tue, Jun 23, 2020 at 01:58:15PM -0700, Nick Desaulniers wrote:
> My point was more so about avoiding needless cc-option checks when
> they're tautological.

No, you're right. The oldest gcc we support is 4.8 and that eats -msse2
just fine.

> When Andy says "consider dropping the problematic GCC version" I
> wonder if it was meant *just for this selftest* as I suggested, or
> outright (which is untenable IMO, as it's a large jump to GCC 7.1+).

Yeah, he's been schooled now :)

> I'm not sure I agree, but I'll take feature detection any day over
> version detection.

Yeah, we had another issue recently with a funky compiler on some funky
distro:

https://lkml.kernel.org/r/CAEJqkgi3w%2BzvMkRBP4VtAewX1UJxrVNRQ03MtRN_yH-PwOOScQ@mail.gmail.com

and I have to say, I'm not at all happy about such subtle issues
stemming from how a distro builds its compiler and that having an impact
on the kernel build. But I guess this is the new reality now since we're
getting more and more married to the compiler.

> Guilty, your honor. :P
> 
> Maybe the feature test should be copy+pasta'd to those other places in
> the kernel, rather than the version check?

"pasta'd"- mmhmhm, tasty noodles. Good idea for lunch tomorrow, thanks!
:-)

But sure, if you feel bored. Let it soak a while first, though, until
all the builders and bots have seen it and haven't triggered moar funky
build errors.

Thx.

-- 
Regards/Gruss,
    Boris.

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