Created attachment 289417 [details] Userland program for setting mxcsr While debugging crash with amdgpu driver ( https://gitlab.freedesktop.org/drm/amd/-/issues/1154 ), I appear to have found a more general bug in kernel_fpu_begin(). What seems to happen is the kernel_fpu_begin() does not reset the mxcsr value, instead leaving it at whatever value the user mode thread has been using. This has the potential to disturb any floating point operations in the kernel and causing FPU/SIMD exceptions if the mask bits have been changed. I have been able to reproduce the problem as follows: 1. Have a kernel module that uses floating point operations. In my case, amdgpu. 2. Compile attached mxcsr_test.c: gcc -Wall mxcsr_test.c 3. Run one copy per CPU thread, using a normal unprivileged user account: ./a.out & ./a.out & ... & ./a.out 4. Do something that causes floating point operations in kernel, in my case graphic activity caused by browsing with Firefox works best. 5. Within an hour, the kernel will crash with "simd exception". In 0001-kernel-traps-print-mxcsr.txt I have added extra debug to print the MXCSR value. In dmesg_5.7.0_mxcsr_crash.txt it can be seen that the MXCSR value in kernel is the same as is set by mxcsr_test.c.
Created attachment 289419 [details] Dmesg from crash, with MXCSR debug info
Created attachment 289421 [details] Patch for printing extra debug info in simd exception
Created attachment 289423 [details] Possible fix, by setting mxcsr to default value
(In reply to Petteri Aimonen from comment #3) > Possible fix, by setting mxcsr to default value Pretty much. But with some saving and restoring of the MXCSR. Pls try the following one. Thx.
Created attachment 289447 [details] save/restore mxcsr
I can try that patch. But is the separate save & restore of MXCSR necessary, considering copy_fpregs_to_fpstate(¤t->thread.fpu); uses fxsaveq that already saves mxcsr? It seems to me the difference would be only in the MXCSR value that is active between kernel_fpu_end() and the context switch back to userland. There shouldn't be any float operations between those, and if there accidentally are, using the kernel MXCSR value would be safer.
Well, depends. It can use XSAVE too: if (likely(use_xsave())) { copy_xregs_to_kernel(&fpu->state.xsave); not that it matters tho - all XSAVE* save MXCSR too. Lemme add two more gents to check. Dave, Sebastian, do you guys think that it would be enough to unconditionally overwrite MXCSR in kernel_fpu_begin() if userspace has unmasked the SIMD exception bits and thus kernel code using SIMD insns can end up in an SIMD exception? Thx.
I agree that we can just "clobber" the bad user MXCSR value with the good kernel value and leave it. All the rest of the XSAVE/FPU state is clobbered there anyway, and gets restored before going back to userspace, so I think MXCSR can just do the same thing. It would also be nice to go double-check the SDM for anything else that can cause SIMD/FP faults that we might have missed. MXCSR is certainly the worst offender, but I also wonder if there are any others we might have missed. This also seems like something we need a selftest for.
This would seem to be the only sensible option to me. If it was *only* the trap bits that would be one thing (the #XF handler could detect that case and adjust MXCSR at trap time, saving a few cycles), but user space could have mucked with the various silent bits as well, changing the semantics of kernel code.
Ok, Petteri you wanna give producing a proper patch a try, along with a selftest and explaining the issue properly in the commit message? Thx.
@Borislav I can certainly try :)
Ok, if you have any questions, feel free to switch to mail so that we don't pollute bugzilla with modalities and I'll answer them if I can. Thx.
Created attachment 289467 [details] Proposed patch with selftest Here is a properly formatted patch, including a selftest. Compiling floating point code for kernel modules requires extra CFLAGS. I have chosen to add FPU_CFLAGS define to arch/x86/Makefile, using the same logic as amdgpu currently uses in multiple Makefiles. Eventually amdgpu could be changed to use the global FPU_CFLAGS also. I did a cursory check if the Intel SDM for other potentially problematic registers: - MPX bound registers: MPX support has been purposely removed from kernel. - SSE2 EFLAGS register: is not retained between function calls, so code will not expect any particular value - x87 legacy control word register: for pre-SSE CPUs, this is initialized to sane default by FNSAVE instruction. Potentially problematic if kernel code would use legacy x87 instructions on a cpu with X86_FEATURE_XSAVE.
Created attachment 289635 [details] Proposed patch with selftest, v2 Here is an updated version of the patch, incorporating the feedback from mail chain: - Actual fix is unchanged. - Moved the test module trigger file under debugfs. - Moved self-test userland part out of x86 folder as it is portable. - Minor formatting fixes. - Updated patch to apply cleanly against newest upstream.
Please send it to lkml and Cc all the people on the thread so that it can get discussed properly. bugzilla is not for dealing with patches. Thx.
Sorry, I'm unfamiliar with the kernel patch process and this is getting more complex than I have time available currently. As such, I will just leave the patch here and run a custom kernel for now. I hope the fix will eventually make in, but this is as far as I can take this now.
Ok, no problem. I'll take care of it and Cc you. Thanks for taking the time and responding to feedback.
Boris, are you still planning to send this out?
7ad816762f9b ("x86/fpu: Reset MXCSR to default in kernel_fpu_begin()") is already in v5.8-rc4.
*** Bug 206987 has been marked as a duplicate of this bug. ***
I am still hitting this problem even with the patch (7ad816762f9b) applied. For details please see my comments in bug 206987 here: https://bugzilla.kernel.org/show_bug.cgi?id=206987#c33
(In reply to krakopo from comment #21) > I am still hitting this problem even with the patch (7ad816762f9b) applied. > For details please see my comments in bug 206987 here: > https://bugzilla.kernel.org/show_bug.cgi?id=206987#c33 Does this always happen only when you run a KVM guest and not otherwise?
@Borislav Yes that was the case. I have recently moved up to kernel 5.8.5 and so far I have not been able to reproduce this even when using KVM.
Interesting. Ok, if you can reproduce it, do let me know how so that I can try it here. Thx.
Looks forgotten. Feel free to reopen if still of interest.