* Re: [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64) [not found] <tip-de05764e0b2a3d9559e099a2e134f8cef4500fdd@git.kernel.org> @ 2014-04-20 0:28 ` Borislav Petkov 2014-04-25 11:09 ` Matt Fleming 0 siblings, 1 reply; 4+ messages in thread From: Borislav Petkov @ 2014-04-20 0:28 UTC (permalink / raw) To: linux-kernel, mingo, hpa, ricardo.neri-calderon, tglx, bp, matt.fleming Cc: linux-tip-commits On Sat, Apr 19, 2014 at 03:50:44PM -0700, tip-bot for Ricardo Neri wrote: > Commit-ID: de05764e0b2a3d9559e099a2e134f8cef4500fdd > Gitweb: http://git.kernel.org/tip/de05764e0b2a3d9559e099a2e134f8cef4500fdd > Author: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > AuthorDate: Thu, 27 Mar 2014 15:10:42 -0700 > Committer: Matt Fleming <matt.fleming@intel.com> > CommitDate: Thu, 17 Apr 2014 13:26:32 +0100 > > x86/efi: Save and restore FPU context around efi_calls (x86_64) > > Do a complete FPU context save/restore around the EFI calls. This required > as runtime EFI firmware may potentially use the FPU. > > This change covers only the x86_64 configuration. > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > Cc: Borislav Petkov <bp@suse.de> > Signed-off-by: Matt Fleming <matt.fleming@intel.com> > --- > arch/x86/include/asm/efi.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h > index 19292e7..0b19187 100644 > --- a/arch/x86/include/asm/efi.h > +++ b/arch/x86/include/asm/efi.h > @@ -1,6 +1,7 @@ > #ifndef _ASM_X86_EFI_H > #define _ASM_X86_EFI_H > > +#include <asm/i387.h> > /* > * We map the EFI regions needed for runtime services non-contiguously, > * with preserved alignment on virtual addresses starting from -4G down > @@ -54,7 +55,9 @@ extern u64 asmlinkage efi_call(void *fp, ...); > \ > efi_sync_low_kernel_mappings(); \ > preempt_disable(); \ > + __kernel_fpu_begin(); \ > __s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__); \ > + __kernel_fpu_end(); \ > preempt_enable(); \ I guess you can use the kernel_fpu_begin/end() variants here (i.e., without the "__") which disable and enable preemption and thus drop the preempt_* calls: efi_sync_low_kernel_mappings(); kernel_fpu_begin(); __s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__); kernel_fpu_end(); __s; I'm not sure about the WARN_ON_ONCE(!irq_fpu_usable()); thing in kernel_fpu_begin() though, I guess it wouldn't hurt... -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64) 2014-04-20 0:28 ` [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64) Borislav Petkov @ 2014-04-25 11:09 ` Matt Fleming 2014-04-25 11:40 ` Borislav Petkov 0 siblings, 1 reply; 4+ messages in thread From: Matt Fleming @ 2014-04-25 11:09 UTC (permalink / raw) To: Borislav Petkov Cc: linux-kernel, mingo, hpa, ricardo.neri-calderon, tglx, matt.fleming, linux-tip-commits, Seiji Aguchi On Sun, 20 Apr, at 02:28:11AM, Borislav Petkov wrote: > > I guess you can use the kernel_fpu_begin/end() variants here (i.e., > without the "__") which disable and enable preemption and thus drop the > preempt_* calls: > > efi_sync_low_kernel_mappings(); > kernel_fpu_begin(); > __s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__); > kernel_fpu_end(); > __s; > > I'm not sure about the > > WARN_ON_ONCE(!irq_fpu_usable()); > > thing in kernel_fpu_begin() though, I guess it wouldn't hurt... Hmm... note that we may call EFI runtime services from interrupt context in efi_pstore_write(), so it seems like it would be possible to trigger that WARN_ON_ONCE() there. Seiji (Cc'd) might have some opinions on this. Either way, if someone sends me a patch ontop of this one that swaps the __kernel_fpu_begin() for kernel_fpu_begin() I can try them out in my lab. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64) 2014-04-25 11:09 ` Matt Fleming @ 2014-04-25 11:40 ` Borislav Petkov 2014-04-29 10:46 ` Matt Fleming 0 siblings, 1 reply; 4+ messages in thread From: Borislav Petkov @ 2014-04-25 11:40 UTC (permalink / raw) To: Matt Fleming Cc: linux-kernel, mingo, hpa, ricardo.neri-calderon, tglx, matt.fleming, linux-tip-commits, Seiji Aguchi On Fri, Apr 25, 2014 at 12:09:36PM +0100, Matt Fleming wrote: > Hmm... note that we may call EFI runtime services from interrupt context > in efi_pstore_write(), so it seems like it would be possible to trigger > that WARN_ON_ONCE() there. > > Seiji (Cc'd) might have some opinions on this. > > Either way, if someone sends me a patch ontop of this one that swaps the > __kernel_fpu_begin() for kernel_fpu_begin() I can try them out in my > lab. Well, the more I think about it, the more I'm persuaded that you actually do *really* need that WARN_ON_ONCE check there to make sure you're not fiddling with the FPU while in an interrupt context and in an unsafe way (see interrupted_kernel_fpu_idle() and interrupted_user_mode()). And so you do need the variants without the "__" which include the check. Anyway, here it is, do give it a good run: --- From: Borislav Petkov <bp@suse.de> Date: Fri, 25 Apr 2014 13:30:21 +0200 Subject: [PATCH] efi: Check for unsafe dealing with FPU state in irq ctxt efi_call can happen in an irq context (pstore) and there we really need to make sure we're not scribbling over FPU state while we've interrupted a thread or kernel mode with a live FPU state. Therefore, use the kernel_fpu_begin/end() variants which do that check. Signed-off-by: Borislav Petkov <bp@suse.de> --- arch/x86/include/asm/efi.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 1eb5f6433ad8..f969ce8bea24 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -67,11 +67,9 @@ extern u64 asmlinkage efi_call(void *fp, ...); efi_status_t __s; \ \ efi_sync_low_kernel_mappings(); \ - preempt_disable(); \ - __kernel_fpu_begin(); \ + kernel_fpu_begin(); \ __s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__); \ - __kernel_fpu_end(); \ - preempt_enable(); \ + kernel_fpu_end(); \ __s; \ }) -- 1.9.0.258.g00eda23 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64) 2014-04-25 11:40 ` Borislav Petkov @ 2014-04-29 10:46 ` Matt Fleming 0 siblings, 0 replies; 4+ messages in thread From: Matt Fleming @ 2014-04-29 10:46 UTC (permalink / raw) To: Borislav Petkov Cc: linux-kernel, mingo, hpa, ricardo.neri-calderon, tglx, matt.fleming, linux-tip-commits, Seiji Aguchi On Fri, 25 Apr, at 01:40:10PM, Borislav Petkov wrote: > > Well, the more I think about it, the more I'm persuaded that > you actually do *really* need that WARN_ON_ONCE check there to > make sure you're not fiddling with the FPU while in an interrupt > context and in an unsafe way (see interrupted_kernel_fpu_idle() and > interrupted_user_mode()). And so you do need the variants without the > "__" which include the check. > > Anyway, here it is, do give it a good run: Thanks Borislav, I gave this a run on my machines and everything appears to be fine. I'll probably send it to tip by the end of the week, I'm just gonna let it stew on my 'next' branch for a while. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-04-29 10:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <tip-de05764e0b2a3d9559e099a2e134f8cef4500fdd@git.kernel.org>
2014-04-20 0:28 ` [tip:x86/efi] x86/efi: Save and restore FPU context around efi_calls (x86_64) Borislav Petkov
2014-04-25 11:09 ` Matt Fleming
2014-04-25 11:40 ` Borislav Petkov
2014-04-29 10:46 ` Matt Fleming
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).