[tip:x86/fpu] x86/fpu: Don't export __kernel_fpu_{begin,end}()

[tip:x86/fpu] x86/fpu: Don't export __kernel_fpu_{begin,end}()

[tip-bot for Sebastian Andrzej Siewior]

Commit-ID:  7d79adb87fa79e4a4c59424fd5b5a922861fc5f6
Gitweb:     https://git.kernel.org/tip/7d79adb87fa79e4a4c59424fd5b5a922861fc5f6
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Thu, 29 Nov 2018 16:02:10 +0100
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Mon, 3 Dec 2018 19:37:27 +0100

x86/fpu: Don't export __kernel_fpu_{begin,end}()

There is one user of __kernel_fpu_begin() and before invoking it,
it invokes preempt_disable(). So it could invoke kernel_fpu_begin()
right away. The 32bit version of arch_efi_call_virt_setup() and
arch_efi_call_virt_teardown() does this already.

The comment above *kernel_fpu*() claims that before invoking
__kernel_fpu_begin() preemption should be disabled and that KVM is a
good example of doing it. Well, KVM doesn't do that since commit

  f775b13eedee2 ("x86,kvm: move qemu/guest FPU switching out to vcpu_run")

so it is not an example anymore.

With EFI gone as the last user of __kernel_fpu_{begin|end}(), both can
be made static and not exported anymore.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Rik van Riel <riel@surriel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Nicolai Stange <nstange@suse.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: kvm ML <kvm@vger.kernel.org>
Cc: linux-efi <linux-efi@vger.kernel.org>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20181129150210.2k4mawt37ow6c2vq@linutronix.de
---
 arch/x86/include/asm/efi.h     |  6 ++----
 arch/x86/include/asm/fpu/api.h | 16 ++++++----------
 arch/x86/kernel/fpu/core.c     |  6 ++----
 3 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index eea40d52ca78..45864898f7e5 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -82,8 +82,7 @@ struct efi_scratch {
 #define arch_efi_call_virt_setup()					\
 ({									\
 	efi_sync_low_kernel_mappings();					\
-	preempt_disable();						\
-	__kernel_fpu_begin();						\
+	kernel_fpu_begin();						\
 	firmware_restrict_branch_speculation_start();			\
 									\
 	if (!efi_enabled(EFI_OLD_MEMMAP))				\
@@ -99,8 +98,7 @@ struct efi_scratch {
 		efi_switch_mm(efi_scratch.prev_mm);			\
 									\
 	firmware_restrict_branch_speculation_end();			\
-	__kernel_fpu_end();						\
-	preempt_enable();						\
+	kernel_fpu_end();						\
 })
 
 extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index a9caac9d4a72..e368d8d94ca6 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -12,17 +12,13 @@
 #define _ASM_X86_FPU_API_H
 
 /*
- * Careful: __kernel_fpu_begin/end() must be called with preempt disabled
- * and they don't touch the preempt state on their own.
- * If you enable preemption after __kernel_fpu_begin(), preempt notifier
- * should call the __kernel_fpu_end() to prevent the kernel/user FPU
- * state from getting corrupted. KVM for example uses this model.
- *
- * All other cases use kernel_fpu_begin/end() which disable preemption
- * during kernel FPU usage.
+ * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
+ * disables preemption so be careful if you intend to use it for long periods
+ * of time.
+ * If you intend to use the FPU in softirq you need to check first with
+ * irq_fpu_usable() if it is possible.
+ * Using the FPU in hardirq is not allowed.
  */
-extern void __kernel_fpu_begin(void);
-extern void __kernel_fpu_end(void);
 extern void kernel_fpu_begin(void);
 extern void kernel_fpu_end(void);
 extern bool irq_fpu_usable(void);
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 2ea85b32421a..2e5003fef51a 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -93,7 +93,7 @@ bool irq_fpu_usable(void)
 }
 EXPORT_SYMBOL(irq_fpu_usable);
 
-void __kernel_fpu_begin(void)
+static void __kernel_fpu_begin(void)
 {
 	struct fpu *fpu = &current->thread.fpu;
 
@@ -111,9 +111,8 @@ void __kernel_fpu_begin(void)
 		__cpu_invalidate_fpregs_state();
 	}
 }
-EXPORT_SYMBOL(__kernel_fpu_begin);
 
-void __kernel_fpu_end(void)
+static void __kernel_fpu_end(void)
 {
 	struct fpu *fpu = &current->thread.fpu;
 
@@ -122,7 +121,6 @@ void __kernel_fpu_end(void)
 
 	kernel_fpu_enable();
 }
-EXPORT_SYMBOL(__kernel_fpu_end);
 
 void kernel_fpu_begin(void)
 {

Re: [tip:x86/fpu] x86/fpu: Don't export __kernel_fpu_{begin,end}()

[Ard Biesheuvel]

On Mon, 3 Dec 2018 at 22:04, tip-bot for Sebastian Andrzej Siewior
<tipbot@zytor.com> wrote:
>
> Commit-ID:  7d79adb87fa79e4a4c59424fd5b5a922861fc5f6
> Gitweb:     https://git.kernel.org/tip/7d79adb87fa79e4a4c59424fd5b5a922861fc5f6
> Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> AuthorDate: Thu, 29 Nov 2018 16:02:10 +0100
> Committer:  Borislav Petkov <bp@suse.de>
> CommitDate: Mon, 3 Dec 2018 19:37:27 +0100
>
> x86/fpu: Don't export __kernel_fpu_{begin,end}()
>
> There is one user of __kernel_fpu_begin() and before invoking it,
> it invokes preempt_disable(). So it could invoke kernel_fpu_begin()
> right away. The 32bit version of arch_efi_call_virt_setup() and
> arch_efi_call_virt_teardown() does this already.
>
> The comment above *kernel_fpu*() claims that before invoking
> __kernel_fpu_begin() preemption should be disabled and that KVM is a
> good example of doing it. Well, KVM doesn't do that since commit
>
>   f775b13eedee2 ("x86,kvm: move qemu/guest FPU switching out to vcpu_run")
>
> so it is not an example anymore.
>
> With EFI gone as the last user of __kernel_fpu_{begin|end}(), both can
> be made static and not exported anymore.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Reviewed-by: Rik van Riel <riel@surriel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Nicolai Stange <nstange@suse.de>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: kvm ML <kvm@vger.kernel.org>
> Cc: linux-efi <linux-efi@vger.kernel.org>
> Cc: x86-ml <x86@kernel.org>
> Link: https://lkml.kernel.org/r/20181129150210.2k4mawt37ow6c2vq@linutronix.de
> ---
>  arch/x86/include/asm/efi.h     |  6 ++----
>  arch/x86/include/asm/fpu/api.h | 16 ++++++----------
>  arch/x86/kernel/fpu/core.c     |  6 ++----
>  3 files changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index eea40d52ca78..45864898f7e5 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -82,8 +82,7 @@ struct efi_scratch {
>  #define arch_efi_call_virt_setup()                                     \
>  ({                                                                     \
>         efi_sync_low_kernel_mappings();                                 \
> -       preempt_disable();                                              \
> -       __kernel_fpu_begin();                                           \
> +       kernel_fpu_begin();                                             \
>         firmware_restrict_branch_speculation_start();                   \
>                                                                         \
>         if (!efi_enabled(EFI_OLD_MEMMAP))                               \
> @@ -99,8 +98,7 @@ struct efi_scratch {
>                 efi_switch_mm(efi_scratch.prev_mm);                     \
>                                                                         \
>         firmware_restrict_branch_speculation_end();                     \
> -       __kernel_fpu_end();                                             \
> -       preempt_enable();                                               \
> +       kernel_fpu_end();                                               \
>  })
>
>  extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
> diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
> index a9caac9d4a72..e368d8d94ca6 100644
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -12,17 +12,13 @@
>  #define _ASM_X86_FPU_API_H
>
>  /*
> - * Careful: __kernel_fpu_begin/end() must be called with preempt disabled
> - * and they don't touch the preempt state on their own.
> - * If you enable preemption after __kernel_fpu_begin(), preempt notifier
> - * should call the __kernel_fpu_end() to prevent the kernel/user FPU
> - * state from getting corrupted. KVM for example uses this model.
> - *
> - * All other cases use kernel_fpu_begin/end() which disable preemption
> - * during kernel FPU usage.
> + * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
> + * disables preemption so be careful if you intend to use it for long periods
> + * of time.
> + * If you intend to use the FPU in softirq you need to check first with
> + * irq_fpu_usable() if it is possible.
> + * Using the FPU in hardirq is not allowed.

According to the documentation in x86/kernel/fpu/core.c, this is not
true. So which one is accurate?

>   */
> -extern void __kernel_fpu_begin(void);
> -extern void __kernel_fpu_end(void);
>  extern void kernel_fpu_begin(void);
>  extern void kernel_fpu_end(void);
>  extern bool irq_fpu_usable(void);
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index 2ea85b32421a..2e5003fef51a 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -93,7 +93,7 @@ bool irq_fpu_usable(void)
>  }
>  EXPORT_SYMBOL(irq_fpu_usable);
>
> -void __kernel_fpu_begin(void)
> +static void __kernel_fpu_begin(void)
>  {
>         struct fpu *fpu = &current->thread.fpu;
>
> @@ -111,9 +111,8 @@ void __kernel_fpu_begin(void)
>                 __cpu_invalidate_fpregs_state();
>         }
>  }
> -EXPORT_SYMBOL(__kernel_fpu_begin);
>
> -void __kernel_fpu_end(void)
> +static void __kernel_fpu_end(void)
>  {
>         struct fpu *fpu = &current->thread.fpu;
>
> @@ -122,7 +121,6 @@ void __kernel_fpu_end(void)
>
>         kernel_fpu_enable();
>  }
> -EXPORT_SYMBOL(__kernel_fpu_end);
>
>  void kernel_fpu_begin(void)
>  {

Re: [tip:x86/fpu] x86/fpu: Don't export __kernel_fpu_{begin,end}()

[Borislav Petkov]

On Mon, Dec 03, 2018 at 10:12:19PM +0100, Ard Biesheuvel wrote:
> > + * Using the FPU in hardirq is not allowed.
> 
> According to the documentation in x86/kernel/fpu/core.c, this is not
> true. So which one is accurate?

I think you mean the irq from user mode... Yap, we do allow that.

Sebastian?

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [tip:x86/fpu] x86/fpu: Don't export __kernel_fpu_{begin,end}()

[Borislav Petkov]

On Mon, Dec 03, 2018 at 11:08:41PM +0100, Borislav Petkov wrote:
> On Mon, Dec 03, 2018 at 10:12:19PM +0100, Ard Biesheuvel wrote:
> > > + * Using the FPU in hardirq is not allowed.
> > 
> > According to the documentation in x86/kernel/fpu/core.c, this is not
> > true. So which one is accurate?
> 
> I think you mean the irq from user mode... Yap, we do allow that.
> 
> Sebastian?

Ok, I've zapped that sentence from the comment until we clarify what
Sebastian meant.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[tip:x86/fpu] x86/fpu: Don't export __kernel_fpu_{begin,end}()

[tip-bot for Sebastian Andrzej Siewior]

Commit-ID:  12209993e98c5fa1855c467f22a24e3d5b8be205
Gitweb:     https://git.kernel.org/tip/12209993e98c5fa1855c467f22a24e3d5b8be205
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Thu, 29 Nov 2018 16:02:10 +0100
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Tue, 4 Dec 2018 12:37:28 +0100

x86/fpu: Don't export __kernel_fpu_{begin,end}()

There is one user of __kernel_fpu_begin() and before invoking it,
it invokes preempt_disable(). So it could invoke kernel_fpu_begin()
right away. The 32bit version of arch_efi_call_virt_setup() and
arch_efi_call_virt_teardown() does this already.

The comment above *kernel_fpu*() claims that before invoking
__kernel_fpu_begin() preemption should be disabled and that KVM is a
good example of doing it. Well, KVM doesn't do that since commit

  f775b13eedee2 ("x86,kvm: move qemu/guest FPU switching out to vcpu_run")

so it is not an example anymore.

With EFI gone as the last user of __kernel_fpu_{begin|end}(), both can
be made static and not exported anymore.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Rik van Riel <riel@surriel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Nicolai Stange <nstange@suse.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: kvm ML <kvm@vger.kernel.org>
Cc: linux-efi <linux-efi@vger.kernel.org>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20181129150210.2k4mawt37ow6c2vq@linutronix.de
---
 arch/x86/include/asm/efi.h     |  6 ++----
 arch/x86/include/asm/fpu/api.h | 15 +++++----------
 arch/x86/kernel/fpu/core.c     |  6 ++----
 3 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index eea40d52ca78..45864898f7e5 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -82,8 +82,7 @@ struct efi_scratch {
 #define arch_efi_call_virt_setup()					\
 ({									\
 	efi_sync_low_kernel_mappings();					\
-	preempt_disable();						\
-	__kernel_fpu_begin();						\
+	kernel_fpu_begin();						\
 	firmware_restrict_branch_speculation_start();			\
 									\
 	if (!efi_enabled(EFI_OLD_MEMMAP))				\
@@ -99,8 +98,7 @@ struct efi_scratch {
 		efi_switch_mm(efi_scratch.prev_mm);			\
 									\
 	firmware_restrict_branch_speculation_end();			\
-	__kernel_fpu_end();						\
-	preempt_enable();						\
+	kernel_fpu_end();						\
 })
 
 extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
index a9caac9d4a72..b56d504af654 100644
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -12,17 +12,12 @@
 #define _ASM_X86_FPU_API_H
 
 /*
- * Careful: __kernel_fpu_begin/end() must be called with preempt disabled
- * and they don't touch the preempt state on their own.
- * If you enable preemption after __kernel_fpu_begin(), preempt notifier
- * should call the __kernel_fpu_end() to prevent the kernel/user FPU
- * state from getting corrupted. KVM for example uses this model.
- *
- * All other cases use kernel_fpu_begin/end() which disable preemption
- * during kernel FPU usage.
+ * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
+ * disables preemption so be careful if you intend to use it for long periods
+ * of time.
+ * If you intend to use the FPU in softirq you need to check first with
+ * irq_fpu_usable() if it is possible.
  */
-extern void __kernel_fpu_begin(void);
-extern void __kernel_fpu_end(void);
 extern void kernel_fpu_begin(void);
 extern void kernel_fpu_end(void);
 extern bool irq_fpu_usable(void);
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 2ea85b32421a..2e5003fef51a 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -93,7 +93,7 @@ bool irq_fpu_usable(void)
 }
 EXPORT_SYMBOL(irq_fpu_usable);
 
-void __kernel_fpu_begin(void)
+static void __kernel_fpu_begin(void)
 {
 	struct fpu *fpu = &current->thread.fpu;
 
@@ -111,9 +111,8 @@ void __kernel_fpu_begin(void)
 		__cpu_invalidate_fpregs_state();
 	}
 }
-EXPORT_SYMBOL(__kernel_fpu_begin);
 
-void __kernel_fpu_end(void)
+static void __kernel_fpu_end(void)
 {
 	struct fpu *fpu = &current->thread.fpu;
 
@@ -122,7 +121,6 @@ void __kernel_fpu_end(void)
 
 	kernel_fpu_enable();
 }
-EXPORT_SYMBOL(__kernel_fpu_end);
 
 void kernel_fpu_begin(void)
 {

Re: [tip:x86/fpu] x86/fpu: Don't export __kernel_fpu_{begin,end}()

[Sebastian Andrzej Siewior]

On 2018-12-03 23:08:41 [+0100], Borislav Petkov wrote:
> On Mon, Dec 03, 2018 at 10:12:19PM +0100, Ard Biesheuvel wrote:
> > > + * Using the FPU in hardirq is not allowed.
> > 
> > According to the documentation in x86/kernel/fpu/core.c, this is not
> > true. So which one is accurate?
> 
> I think you mean the irq from user mode... Yap, we do allow that.
> 
> Sebastian?

Do you refer to
| *   - by IRQ context code to potentially use the FPU
| *     if it's unused.
  
? It is possible to use the FPU in IRQ context.
The FPU could be used in user-context surrounded by kernel_fpu_begin().
This only disables preemption so an IRQ could interrupt it. This IRQ
could then use the FPU or raise a SoftIRQ which would use it.
Therefore on x86 it is required to check with irq_fpu_usable() if the
FPU can be used. If the FPU can not be used, you have to implement
fallback code.

With the "restore FPU on return to userland" series we need to modify
the FPU in a few places. The softirq and preemption is disabled. I
didn't find any in-IRQ users.
Going forward I would like to remove the in-IRQ part and
irq_fpu_usable() and disable softirq as part of kernel_fpu_begin().

> Thx.

Sebastian

Re: [tip:x86/fpu] x86/fpu: Don't export __kernel_fpu_{begin,end}()

[Borislav Petkov]

On Tue, Dec 04, 2018 at 01:15:10PM +0100, Sebastian Andrzej Siewior wrote:
> Do you refer to
> | *   - by IRQ context code to potentially use the FPU
> | *     if it's unused.
>   
> ? It is possible to use the FPU in IRQ context.

I mean interrupted_user_mode() where we apparently can use the FPU when
handling an IRQ from user mode.

> The FPU could be used in user-context surrounded by kernel_fpu_begin().

Right, that.

> This only disables preemption so an IRQ could interrupt it. This IRQ
> could then use the FPU or raise a SoftIRQ which would use it.
> Therefore on x86 it is required to check with irq_fpu_usable() if the

Yes, and the check that thing does is:

        return !in_interrupt() ||
                interrupted_user_mode() || ...

so you're either *not* in interrupt, or you've gotten the IRQ while in
user mode.

> FPU can be used. If the FPU can not be used, you have to implement
> fallback code.
> 
> With the "restore FPU on return to userland" series we need to modify
> the FPU in a few places. The softirq and preemption is disabled. I
> didn't find any in-IRQ users.
> Going forward I would like to remove the in-IRQ part and
> irq_fpu_usable() and disable softirq as part of kernel_fpu_begin().

Right, and we should document all those new conditions prominently so
that people are aware.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.