* [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
@ 2020-06-02 10:11 Borislav Petkov
2020-06-02 10:29 ` Petteri Aimonen
0 siblings, 1 reply; 13+ messages in thread
From: Borislav Petkov @ 2020-06-02 10:11 UTC (permalink / raw)
To: Petteri Aimonen
Cc: Andy Lutomirski, Dave Hansen, H. Peter Anvin, x86-ml, lkml
Switching to mail where patches are discussed.
> Subject: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
Fix formatting to:
Subject: [PATCH] x86/fpu: Reset MXCSR to default in kernel_fpu_begin()
> From: Petteri Aimonen <jpa@git.mail.kapsi.fi>
>
> Previously kernel floating point code would run with the control
> register value last set by userland code. This could affect calculation
> results or cause floating point exceptions, leading to a crash.
Please expand that paragraph here as you have it in the bugzilla so that
it is clear in the future how that can happen.
> This commit also adds a selftest for the usage of FPU code in kernel
s/This commit adds/Add/
> mode and a new FPU_CFLAGS variable in Makefile for building code that
> uses floating point.
>
> Currently only implemented for x86 (32 and 64-bit). In future kernel FPU
> usage could be unified between the different architectures supporting it.
>
> Signed-off-by: Petteri Aimonen <jpa@git.mail.kapsi.fi>
Add here:
Link: https://bugzilla.kernel.org/show_bug.cgi?id=207979
> ---
> arch/x86/Makefile | 16 ++++
> arch/x86/include/asm/fpu/internal.h | 5 ++
> arch/x86/kernel/fpu/core.c | 1 +
> lib/Kconfig.debug | 10 +++
> lib/Makefile | 5 ++
> lib/test_fpu.c | 104 +++++++++++++++++++++++++
> tools/testing/selftests/x86/Makefile | 4 +-
> tools/testing/selftests/x86/test_fpu.c | 56 +++++++++++++
> 8 files changed, 200 insertions(+), 1 deletion(-)
> create mode 100644 lib/test_fpu.c
> create mode 100644 tools/testing/selftests/x86/test_fpu.c
>
> diff --git a/arch/x86/Makefile b/arch/x86/Makefile
> index b65ec63c7..be35e240a 100644
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -149,6 +149,22 @@ ifdef CONFIG_X86_X32
> endif
> export CONFIG_X86_X32_ABI
>
> +#
> +# CFLAGS for compiling floating point code inside the kernel
> +#
> +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
> +export FPU_CFLAGS
Ok, thanks for this. Here's what I think you should do but I'm open to
better ideas:
Instead of adding that kernel module which is x86-specific
to a generic lib/ directory, it should be in, say,
tools/testing/selftests/x86/test_fpu_module.c or so and instead of
reading /proc/sys/debug/test_fpu, the user portion of the code would
simply modprobe it.
Upon load, the module would simply execute its test_fpu() function and
that's it. The user side could then test the modprobe result. And rmmod.
Rinse and repeat for the next test.
But maybe others think the proc fs file is better. Adding Andy who likes
to play with selftests. In any case, the file should not be in /proc but
in debugfs.
What is also missing is the user portion doing ldmxcsr before running
the test and the test should be causing at least one exception which
MXCSR masks off.
Perhaps then having a debugfs to control the module might make more
sense... just thinking out loud here.
Leaving in the rest for the others to see.
Thx.
> #
> # If the function graph tracer is used with mcount instead of fentry,
> # '-maccumulate-outgoing-args' is needed to prevent a GCC bug
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index 44c48e34d..00eac7f15 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -619,6 +619,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 12c708409..8d660890b 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -101,6 +101,7 @@ void kernel_fpu_begin(void)
> copy_fpregs_to_fpstate(¤t->thread.fpu);
> }
> __cpu_invalidate_fpregs_state();
> + ldmxcsr(MXCSR_DEFAULT);
> }
> EXPORT_SYMBOL_GPL(kernel_fpu_begin);
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 21d9c5f6e..f3989c43c 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2201,6 +2201,16 @@ config TEST_MEMINIT
>
> If unsure, say N.
>
> +config TEST_FPU
> + tristate "Test floating point operations inside kernel"
s/inside kernel/in kernel space/
or so.
> + depends on X86
> + help
> + Enable this option to add /proc/sys/debug/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 685aee60d..94b0373d2 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -93,6 +93,11 @@ obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o
> obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o
> obj-$(CONFIG_TEST_LOCKUP) += test_lockup.o
>
> +ifdef FPU_CFLAGS
> +obj-$(CONFIG_TEST_FPU) += test_fpu.o
> +CFLAGS_test_fpu.o += $(FPU_CFLAGS)
> +endif
> +
> 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 000000000..4aaf1d711
> --- /dev/null
> +++ b/lib/test_fpu.c
> @@ -0,0 +1,104 @@
> +// 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.
> + *
> + * To facilitate the test, this module registers file /proc/sys/debug/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 0 bytes.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.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;
> + d = a + b; /* Sets precision flag */
> + e = a + b / 2; /* Result depends on rounding mode */
> + f = b / c; /* Denormal and very large values */
> + g = a + c * f; /* Depends on denormal support */
> +
> + if (d > a && e > a && g > a)
> + return 0;
> + else
> + return -EINVAL;
> +}
> +
> +static int test_fpu_sysctl(struct ctl_table *ctl, int write,
> + void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> + int status = -EINVAL;
> +
> + kernel_fpu_begin();
> + status = test_fpu();
> + kernel_fpu_end();
> +
> + *lenp = 0;
> + return status;
> +}
> +
> +static struct ctl_table test_fpu_table[] = {
> + {
> + .procname = "test_fpu",
> + .data = NULL,
> + .maxlen = 0,
> + .mode = 0444,
> + .proc_handler = test_fpu_sysctl,
> + },
> + { }
> +};
> +
> +static struct ctl_table test_fpu_root_table[] = {
> + {
> + .procname = "debug",
> + .maxlen = 0,
> + .mode = 0555,
> + .child = test_fpu_table,
> + },
> + { }
> +};
> +
> +static struct ctl_table_header *test_fpu_header;
> +
> +static int __init test_fpu_init(void)
> +{
> + int status = -EINVAL;
> +
> + test_fpu_header = register_sysctl_table(test_fpu_root_table);
> + if (!test_fpu_header)
> + return -ENOMEM;
> +
> + /* FPU operations are tested both during the module loader and
> + * later from syscalls.
> + */
> + kernel_fpu_begin();
> + status = test_fpu();
> + kernel_fpu_end();
> +
> + return status;
> +}
> +
> +
> +static void __exit test_fpu_exit(void)
> +{
> + if (test_fpu_header)
> + unregister_sysctl_table(test_fpu_header);
> +}
> +
> +module_init(test_fpu_init);
> +module_exit(test_fpu_exit);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/tools/testing/selftests/x86/Makefile b/tools/testing/selftests/x86/Makefile
> index 5d49bfec1..e1992fb0c 100644
> --- a/tools/testing/selftests/x86/Makefile
> +++ b/tools/testing/selftests/x86/Makefile
> @@ -12,7 +12,7 @@ CAN_BUILD_WITH_NOPIE := $(shell ./check_cc.sh $(CC) trivial_program.c -no-pie)
>
> TARGETS_C_BOTHBITS := single_step_syscall sysret_ss_attrs syscall_nt test_mremap_vdso \
> check_initial_reg_state sigreturn iopl ioperm \
> - protection_keys test_vdso test_vsyscall mov_ss_trap \
> + protection_keys test_fpu test_vdso test_vsyscall mov_ss_trap \
> syscall_arg_fault
> TARGETS_C_32BIT_ONLY := entry_from_vm86 test_syscall_vdso unwind_vdso \
> test_FCMOV test_FCOMI test_FISTTP \
> @@ -98,6 +98,8 @@ endif
> $(OUTPUT)/sysret_ss_attrs_64: thunks.S
> $(OUTPUT)/ptrace_syscall_32: raw_syscall_helper_32.S
> $(OUTPUT)/test_syscall_vdso_32: thunks_32.S
> +$(OUTPUT)/test_fpu_64: -lm
> +$(OUTPUT)/test_fpu_32: -lm
>
> # check_initial_reg_state is special: it needs a custom entry, and it
> # needs to be static so that its interpreter doesn't destroy its initial
> diff --git a/tools/testing/selftests/x86/test_fpu.c b/tools/testing/selftests/x86/test_fpu.c
> new file mode 100644
> index 000000000..4c564f268
> --- /dev/null
> +++ b/tools/testing/selftests/x86/test_fpu.c
> @@ -0,0 +1,56 @@
> +// 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 <fenv.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +
> +int main(void)
> +{
> + char dummy[1];
> + int fd = open("/proc/sys/debug/test_fpu", O_RDONLY);
> +
> + if (fd < 0) {
> + printf("[SKIP]\ttest_fpu kernel module missing\n");
> + 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.25.1
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-02 10:11 [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin() Borislav Petkov
@ 2020-06-02 10:29 ` Petteri Aimonen
2020-06-02 10:56 ` Borislav Petkov
0 siblings, 1 reply; 13+ messages in thread
From: Petteri Aimonen @ 2020-06-02 10:29 UTC (permalink / raw)
To: Borislav Petkov
Cc: Andy Lutomirski, Dave Hansen, H. Peter Anvin, x86-ml, lkml
Hi,
> Instead of adding that kernel module which is x86-specific
> to a generic lib/ directory, it should be in, say,
> tools/testing/selftests/x86/test_fpu_module.c or so and instead of
The kernel module is not actually x86-specific, even though it is
currently only enabled for x86. amdgpu driver already does kernel mode
floating point operations on PPC64 also, and the same module could be
used to test the same thing there.
> reading /proc/sys/debug/test_fpu, the user portion of the code would
> simply modprobe it.
To deterministically trigger the bug, the syscall has to come from the
same thread that has modified MXCSR. Going through /usr/sbin/modprobe
won't work, and manually doing the necessary syscalls for module loading
seems too complicated.
> What is also missing is the user portion doing ldmxcsr before running
> the test and the test should be causing at least one exception which
> MXCSR masks off.
The fesetround() and feenableexcept() are the portable ways to modify
MXCSR. The test module does cause Precision Exception and Denormal
Exception if those exceptions are unmasked.
--
Petteri
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-02 10:29 ` Petteri Aimonen
@ 2020-06-02 10:56 ` Borislav Petkov
2020-06-02 17:03 ` Andy Lutomirski
0 siblings, 1 reply; 13+ messages in thread
From: Borislav Petkov @ 2020-06-02 10:56 UTC (permalink / raw)
To: Petteri Aimonen
Cc: Andy Lutomirski, Dave Hansen, H. Peter Anvin, x86-ml, lkml
Hi,
On Tue, Jun 02, 2020 at 01:29:51PM +0300, Petteri Aimonen wrote:
> The kernel module is not actually x86-specific, even though it is
> currently only enabled for x86. amdgpu driver already does kernel mode
> floating point operations on PPC64 also, and the same module could be
> used to test the same thing there.
Then make it generic please and put the user portion in, say,
tools/testing/selftests/fpu/ and we can ask ppc people to test it too.
People might wanna add more stuff to it in the future, which would be
good.
> To deterministically trigger the bug, the syscall has to come from the
> same thread that has modified MXCSR. Going through /usr/sbin/modprobe
> won't work, and manually doing the necessary syscalls for module loading
> seems too complicated.
Ok, fair enough. But put that file in debugfs pls.
> The fesetround() and feenableexcept() are the portable ways to modify
> MXCSR. The test module does cause Precision Exception and Denormal
> Exception if those exceptions are unmasked.
Ok.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-02 10:56 ` Borislav Petkov
@ 2020-06-02 17:03 ` Andy Lutomirski
2020-06-02 17:27 ` Shuah Khan
2020-06-11 7:36 ` Petteri Aimonen
0 siblings, 2 replies; 13+ messages in thread
From: Andy Lutomirski @ 2020-06-02 17:03 UTC (permalink / raw)
To: Borislav Petkov, Shuah Khan
Cc: Petteri Aimonen, Andy Lutomirski, Dave Hansen, H. Peter Anvin,
x86-ml, lkml
On Tue, Jun 2, 2020 at 3:56 AM Borislav Petkov <bp@alien8.de> wrote:
>
> Hi,
>
> On Tue, Jun 02, 2020 at 01:29:51PM +0300, Petteri Aimonen wrote:
> > The kernel module is not actually x86-specific, even though it is
> > currently only enabled for x86. amdgpu driver already does kernel mode
> > floating point operations on PPC64 also, and the same module could be
> > used to test the same thing there.
>
> Then make it generic please and put the user portion in, say,
> tools/testing/selftests/fpu/ and we can ask ppc people to test it too.
> People might wanna add more stuff to it in the future, which would be
> good.
>
> > To deterministically trigger the bug, the syscall has to come from the
> > same thread that has modified MXCSR. Going through /usr/sbin/modprobe
> > won't work, and manually doing the necessary syscalls for module loading
> > seems too complicated.
>
> Ok, fair enough. But put that file in debugfs pls.
I think I agree. While it would be delightful to have general
selftest tooling for kernel modules, we don't have that right now, and
having the test just work with an appropriately configured kernel
would be nice.
How about putting the file you frob in
/sys/kernel/debug/selftest_helpers/something_or_other. The idea would
be that /sys/kernel/debug/selftest_helpers would be a general place
for kernel helpers needed to make selftests work.
--Andy
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-02 17:03 ` Andy Lutomirski
@ 2020-06-02 17:27 ` Shuah Khan
2020-06-02 19:50 ` Andy Lutomirski
2020-06-11 7:36 ` Petteri Aimonen
1 sibling, 1 reply; 13+ messages in thread
From: Shuah Khan @ 2020-06-02 17:27 UTC (permalink / raw)
To: Andy Lutomirski, Borislav Petkov, Shuah Khan
Cc: Petteri Aimonen, Dave Hansen, H. Peter Anvin, x86-ml, lkml, skhan
On 6/2/20 11:03 AM, Andy Lutomirski wrote:
> On Tue, Jun 2, 2020 at 3:56 AM Borislav Petkov <bp@alien8.de> wrote:
>>
>> Hi,
>>
>> On Tue, Jun 02, 2020 at 01:29:51PM +0300, Petteri Aimonen wrote:
>>> The kernel module is not actually x86-specific, even though it is
>>> currently only enabled for x86. amdgpu driver already does kernel mode
>>> floating point operations on PPC64 also, and the same module could be
>>> used to test the same thing there.
>>
>> Then make it generic please and put the user portion in, say,
>> tools/testing/selftests/fpu/ and we can ask ppc people to test it too.
>> People might wanna add more stuff to it in the future, which would be
>> good.
>>
>>> To deterministically trigger the bug, the syscall has to come from the
>>> same thread that has modified MXCSR. Going through /usr/sbin/modprobe
>>> won't work, and manually doing the necessary syscalls for module loading
>>> seems too complicated.
>>
>> Ok, fair enough. But put that file in debugfs pls.
>
> I think I agree. While it would be delightful to have general
> selftest tooling for kernel modules, we don't have that right now, and
> having the test just work with an appropriately configured kernel
> would be nice.
>
Let's extend it to do what we want it to do. I will happy to take
patches. If you have some concrete ideas on what we can add, please
do a short summary of what is missing. I will find a way to get this
done.
> How about putting the file you frob in
> /sys/kernel/debug/selftest_helpers/something_or_other. The idea would
> be that /sys/kernel/debug/selftest_helpers would be a general place
> for kernel helpers needed to make selftests work.
>
Is this a workaround for the lack of selftest tooling for kernel
modules? In which case, let's us focus on fix selftest tooling.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-02 17:27 ` Shuah Khan
@ 2020-06-02 19:50 ` Andy Lutomirski
2020-06-02 20:25 ` Shuah Khan
0 siblings, 1 reply; 13+ messages in thread
From: Andy Lutomirski @ 2020-06-02 19:50 UTC (permalink / raw)
To: Shuah Khan
Cc: Andy Lutomirski, Borislav Petkov, Shuah Khan, Petteri Aimonen,
Dave Hansen, H. Peter Anvin, x86-ml, lkml
> On Jun 2, 2020, at 10:27 AM, Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 6/2/20 11:03 AM, Andy Lutomirski wrote:
>>> On Tue, Jun 2, 2020 at 3:56 AM Borislav Petkov <bp@alien8.de> wrote:
>>>
>>> Hi,
>>>
>>> On Tue, Jun 02, 2020 at 01:29:51PM +0300, Petteri Aimonen wrote:
>>>> The kernel module is not actually x86-specific, even though it is
>>>> currently only enabled for x86. amdgpu driver already does kernel mode
>>>> floating point operations on PPC64 also, and the same module could be
>>>> used to test the same thing there.
>>>
>>> Then make it generic please and put the user portion in, say,
>>> tools/testing/selftests/fpu/ and we can ask ppc people to test it too.
>>> People might wanna add more stuff to it in the future, which would be
>>> good.
>>>
>>>> To deterministically trigger the bug, the syscall has to come from the
>>>> same thread that has modified MXCSR. Going through /usr/sbin/modprobe
>>>> won't work, and manually doing the necessary syscalls for module loading
>>>> seems too complicated.
>>>
>>> Ok, fair enough. But put that file in debugfs pls.
>> I think I agree. While it would be delightful to have general
>> selftest tooling for kernel modules, we don't have that right now, and
>> having the test just work with an appropriately configured kernel
>> would be nice.
>
> Let's extend it to do what we want it to do. I will happy to take
> patches. If you have some concrete ideas on what we can add, please
> do a short summary of what is missing. I will find a way to get this
> done.
>
>> How about putting the file you frob in
>> /sys/kernel/debug/selftest_helpers/something_or_other. The idea would
>> be that /sys/kernel/debug/selftest_helpers would be a general place
>> for kernel helpers needed to make selftests work.
>
> Is this a workaround for the lack of selftest tooling for kernel
> modules? In which case, let's us focus on fix selftest tooling.
The goal here is to have a selftest that runs kernel code as part of its operation. That is, the selftest is, logically, starting in userspace:
setup_evil_state();
ret = call_kernel_helper();
check_some_other_stuff();
undo_evil_state();
And the call_kernel_helper() could be moderately specific to the test.
>
> thanks,
> -- Shuah
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-02 19:50 ` Andy Lutomirski
@ 2020-06-02 20:25 ` Shuah Khan
2020-06-03 5:19 ` Petteri Aimonen
0 siblings, 1 reply; 13+ messages in thread
From: Shuah Khan @ 2020-06-02 20:25 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, Borislav Petkov, Shuah Khan, Petteri Aimonen,
Dave Hansen, H. Peter Anvin, x86-ml, lkml, skhan
On 6/2/20 1:50 PM, Andy Lutomirski wrote:
>
>
>> On Jun 2, 2020, at 10:27 AM, Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> On 6/2/20 11:03 AM, Andy Lutomirski wrote:
>>>> On Tue, Jun 2, 2020 at 3:56 AM Borislav Petkov <bp@alien8.de> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Tue, Jun 02, 2020 at 01:29:51PM +0300, Petteri Aimonen wrote:
>>>>> The kernel module is not actually x86-specific, even though it is
>>>>> currently only enabled for x86. amdgpu driver already does kernel mode
>>>>> floating point operations on PPC64 also, and the same module could be
>>>>> used to test the same thing there.
>>>>
>>>> Then make it generic please and put the user portion in, say,
>>>> tools/testing/selftests/fpu/ and we can ask ppc people to test it too.
>>>> People might wanna add more stuff to it in the future, which would be
>>>> good.
>>>>
>>>>> To deterministically trigger the bug, the syscall has to come from the
>>>>> same thread that has modified MXCSR. Going through /usr/sbin/modprobe
>>>>> won't work, and manually doing the necessary syscalls for module loading
>>>>> seems too complicated.
>>>>
>>>> Ok, fair enough. But put that file in debugfs pls.
>>> I think I agree. While it would be delightful to have general
>>> selftest tooling for kernel modules, we don't have that right now, and
>>> having the test just work with an appropriately configured kernel
>>> would be nice.
>>
>> Let's extend it to do what we want it to do. I will happy to take
>> patches. If you have some concrete ideas on what we can add, please
>> do a short summary of what is missing. I will find a way to get this
>> done.
>>
>>> How about putting the file you frob in
>>> /sys/kernel/debug/selftest_helpers/something_or_other. The idea would
>>> be that /sys/kernel/debug/selftest_helpers would be a general place
>>> for kernel helpers needed to make selftests work.
>>
>> Is this a workaround for the lack of selftest tooling for kernel
>> modules? In which case, let's us focus on fix selftest tooling.
>
> The goal here is to have a selftest that runs kernel code as part of its operation. That is, the selftest is, logically, starting in userspace:
>
> setup_evil_state();
Is it correct to assume the stuff checked differs from test to test
and done in user-space.
> ret = call_kernel_helper();
> check_some_other_stuff();
Is it correct to assume the stuff checked differs from test to test
and done in user-space.
> undo_evil_state();
Is it correct to assume undoing evil differs from test to test
and done in user-space, provide it can be done from userspace.
>
> And the call_kernel_helper() could be moderately specific to the test.
>
The overall plan sounds good to me. I am all for adding support to
selftests so we can keep extending it.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-02 20:25 ` Shuah Khan
@ 2020-06-03 5:19 ` Petteri Aimonen
2020-06-11 14:38 ` Shuah Khan
0 siblings, 1 reply; 13+ messages in thread
From: Petteri Aimonen @ 2020-06-03 5:19 UTC (permalink / raw)
To: Shuah Khan
Cc: Andy Lutomirski, Andy Lutomirski, Borislav Petkov, Shuah Khan,
Dave Hansen, H. Peter Anvin, x86-ml, lkml
Hi,
> Is it correct to assume the stuff checked differs from test to test
> and done in user-space.
>
> > undo_evil_state();
>
> Is it correct to assume undoing evil differs from test to test
> and done in user-space, provide it can be done from userspace.
Yes, currently the test works like:
do_test_setup();
read_from_debugfs_file();
check_results();
and the middle step stays the same. But of course in general case there
could be argument passing etc, even though the test for this issue
doesn't need them.
Myself I don't see the problem with just adding a file under debugfs and
bind to its read.
--
Petteri
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-02 17:03 ` Andy Lutomirski
2020-06-02 17:27 ` Shuah Khan
@ 2020-06-11 7:36 ` Petteri Aimonen
2020-06-11 9:18 ` Borislav Petkov
1 sibling, 1 reply; 13+ messages in thread
From: Petteri Aimonen @ 2020-06-11 7:36 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Borislav Petkov, Shuah Khan, Dave Hansen, H. Peter Anvin, x86-ml, lkml
Hi,
> How about putting the file you frob in
> /sys/kernel/debug/selftest_helpers/something_or_other. The idea would
> be that /sys/kernel/debug/selftest_helpers would be a general place
> for kernel helpers needed to make selftests work.
Seems like this is the consensus for now.
Any opinions on whether the module should remove "selftest_helpers"
directory on unloading, or not?
1) Removing would break if other test modules will use the same dir.
2) Not removing will leave the directory dangling.
3) Remove only if empty is one option, though I'm unsure how to
cleanly check if debugfs directory is empty.
4) E.g. /sys/kernel/debug/x86/ is created centrally and a symbol is
exported for its dentry. But I'm not sure if it really makes sense
to add another exported symbol just for selftest_helpers.
--
Petteri
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-11 7:36 ` Petteri Aimonen
@ 2020-06-11 9:18 ` Borislav Petkov
2020-06-11 14:19 ` Andy Lutomirski
0 siblings, 1 reply; 13+ messages in thread
From: Borislav Petkov @ 2020-06-11 9:18 UTC (permalink / raw)
To: Petteri Aimonen
Cc: Andy Lutomirski, Shuah Khan, Dave Hansen, H. Peter Anvin, x86-ml, lkml
On Thu, Jun 11, 2020 at 10:36:08AM +0300, Petteri Aimonen wrote:
> Hi,
>
> > How about putting the file you frob in
> > /sys/kernel/debug/selftest_helpers/something_or_other. The idea would
> > be that /sys/kernel/debug/selftest_helpers would be a general place
> > for kernel helpers needed to make selftests work.
>
> Seems like this is the consensus for now.
>
> Any opinions on whether the module should remove "selftest_helpers"
> directory on unloading, or not?
>
> 1) Removing would break if other test modules will use the same dir.
> 2) Not removing will leave the directory dangling.
> 3) Remove only if empty is one option, though I'm unsure how to
> cleanly check if debugfs directory is empty.
> 4) E.g. /sys/kernel/debug/x86/ is created centrally and a symbol is
> exported for its dentry. But I'm not sure if it really makes sense
> to add another exported symbol just for selftest_helpers.
I'd say you do the simple thing and cleanup after you're done, i.e.,
remove the dir. When something else starts using it, then it would need
to be taught to deal with multiple users.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-11 9:18 ` Borislav Petkov
@ 2020-06-11 14:19 ` Andy Lutomirski
2020-06-11 14:50 ` Shuah Khan
0 siblings, 1 reply; 13+ messages in thread
From: Andy Lutomirski @ 2020-06-11 14:19 UTC (permalink / raw)
To: Borislav Petkov
Cc: Petteri Aimonen, Andy Lutomirski, Shuah Khan, Dave Hansen,
H. Peter Anvin, x86-ml, lkml
> On Jun 11, 2020, at 2:18 AM, Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Jun 11, 2020 at 10:36:08AM +0300, Petteri Aimonen wrote:
>> Hi,
>>
>>> How about putting the file you frob in
>>> /sys/kernel/debug/selftest_helpers/something_or_other. The idea would
>>> be that /sys/kernel/debug/selftest_helpers would be a general place
>>> for kernel helpers needed to make selftests work.
>>
>> Seems like this is the consensus for now.
>>
>> Any opinions on whether the module should remove "selftest_helpers"
>> directory on unloading, or not?
>>
>> 1) Removing would break if other test modules will use the same dir.
>> 2) Not removing will leave the directory dangling.
>> 3) Remove only if empty is one option, though I'm unsure how to
>> cleanly check if debugfs directory is empty.
>> 4) E.g. /sys/kernel/debug/x86/ is created centrally and a symbol is
>> exported for its dentry. But I'm not sure if it really makes sense
>> to add another exported symbol just for selftest_helpers.
>
> I'd say you do the simple thing and cleanup after you're done, i.e.,
> remove the dir. When something else starts using it, then it would need
> to be taught to deal with multiple users.
Seems good to me. Let’s have at least two users before we go nuts with the architecture :)
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-03 5:19 ` Petteri Aimonen
@ 2020-06-11 14:38 ` Shuah Khan
0 siblings, 0 replies; 13+ messages in thread
From: Shuah Khan @ 2020-06-11 14:38 UTC (permalink / raw)
To: Petteri Aimonen
Cc: Andy Lutomirski, Andy Lutomirski, Borislav Petkov, Shuah Khan,
Dave Hansen, H. Peter Anvin, x86-ml, lkml, Shuah Khan
On 6/2/20 11:19 PM, Petteri Aimonen wrote:
> Hi,
>
>> Is it correct to assume the stuff checked differs from test to test
>> and done in user-space.
>>
>>> undo_evil_state();
>>
>> Is it correct to assume undoing evil differs from test to test
>> and done in user-space, provide it can be done from userspace.
>
> Yes, currently the test works like:
>
> do_test_setup();
> read_from_debugfs_file();
> check_results();
>
You will need a 4th clanup step step of undo_test_setup().
> and the middle step stays the same. But of course in general case there
> could be argument passing etc, even though the test for this issue
> doesn't need them.
>
> Myself I don't see the problem with just adding a file under debugfs and
> bind to its read.
>
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin()
2020-06-11 14:19 ` Andy Lutomirski
@ 2020-06-11 14:50 ` Shuah Khan
0 siblings, 0 replies; 13+ messages in thread
From: Shuah Khan @ 2020-06-11 14:50 UTC (permalink / raw)
To: Andy Lutomirski, Borislav Petkov
Cc: Petteri Aimonen, Andy Lutomirski, Shuah Khan, Dave Hansen,
H. Peter Anvin, x86-ml, lkml, skhan
On 6/11/20 8:19 AM, Andy Lutomirski wrote:
>
>
>> On Jun 11, 2020, at 2:18 AM, Borislav Petkov <bp@alien8.de> wrote:
>>
>> On Thu, Jun 11, 2020 at 10:36:08AM +0300, Petteri Aimonen wrote:
>>> Hi,
>>>
>>>> How about putting the file you frob in
>>>> /sys/kernel/debug/selftest_helpers/something_or_other. The idea would
>>>> be that /sys/kernel/debug/selftest_helpers would be a general place
>>>> for kernel helpers needed to make selftests work.
>>>
>>> Seems like this is the consensus for now.
>>>
>>> Any opinions on whether the module should remove "selftest_helpers"
>>> directory on unloading, or not?
>>>
>>> 1) Removing would break if other test modules will use the same dir.
>>> 2) Not removing will leave the directory dangling.
>>> 3) Remove only if empty is one option, though I'm unsure how to
>>> cleanly check if debugfs directory is empty.
>>> 4) E.g. /sys/kernel/debug/x86/ is created centrally and a symbol is
>>> exported for its dentry. But I'm not sure if it really makes sense
>>> to add another exported symbol just for selftest_helpers.
>>
>> I'd say you do the simple thing and cleanup after you're done, i.e.,
>> remove the dir. When something else starts using it, then it would need
>> to be taught to deal with multiple users.
>
> Seems good to me. Let’s have at least two users before we go nuts with the architecture :)
>
I am good with this. Let's start with simpler approach and build.
Please cc linux-kselftest and add me to the patch thread.
Thanks for this work. I think this will help expand coverage.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-06-11 14:50 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-02 10:11 [PATCH] arch/x86: reset MXCSR to default in kernel_fpu_begin() Borislav Petkov
2020-06-02 10:29 ` Petteri Aimonen
2020-06-02 10:56 ` Borislav Petkov
2020-06-02 17:03 ` Andy Lutomirski
2020-06-02 17:27 ` Shuah Khan
2020-06-02 19:50 ` Andy Lutomirski
2020-06-02 20:25 ` Shuah Khan
2020-06-03 5:19 ` Petteri Aimonen
2020-06-11 14:38 ` Shuah Khan
2020-06-11 7:36 ` Petteri Aimonen
2020-06-11 9:18 ` Borislav Petkov
2020-06-11 14:19 ` Andy Lutomirski
2020-06-11 14:50 ` Shuah Khan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.