On Thu, Dec 27, 2018 at 11:20 AM Linux Kernel Mailing List
<[email protected]> wrote:
>
> Commit: 12209993e98c5fa1855c467f22a24e3d5b8be205
> Parent: 2f2fcc40a961ed04f0e130803fbaa868c2899310
> Refname: refs/heads/master
> Web: https://git.kernel.org/torvalds/c/12209993e98c5fa1855c467f22a24e3d5b8be205
> Author: Sebastian Andrzej Siewior <[email protected]>
> AuthorDate: Thu Nov 29 16:02:10 2018 +0100
> Committer: Borislav Petkov <[email protected]>
> CommitDate: Tue Dec 4 12:37:28 2018 +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 <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> Reviewed-by: Rik van Riel <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: "Jason A. Donenfeld" <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Nicolai Stange <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: kvm ML <[email protected]>
> Cc: linux-efi <[email protected]>
> Cc: x86-ml <[email protected]>
> Link: https://lkml.kernel.org/r/[email protected]
> ---
> 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 = ¤t->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 = ¤t->thread.fpu;
>
> @@ -122,7 +121,6 @@ void __kernel_fpu_end(void)
>
> kernel_fpu_enable();
> }
> -EXPORT_SYMBOL(__kernel_fpu_end);
>
> void kernel_fpu_begin(void)
> {
This commit removes an exported function pair that is currently used
by out of tree modules, while the replacement pair
(kernel_fpu_begin/end) is EXPORT_SYMBOL_GPL. So this is making
existing functionality GPL only, which will probably be an issue for
several out of tree modules that use the fpu.
Could kernel_fpu_begin/end be made plain EXPORT_SYMBOL?
Marc
On 2019-01-07 18:08:26 [-0400], Marc Dionne wrote:
> On Thu, Dec 27, 2018 at 11:20 AM Linux Kernel Mailing List
> <[email protected]> wrote:
> >
> > Commit: 12209993e98c5fa1855c467f22a24e3d5b8be205
> > x86/fpu: Don't export __kernel_fpu_{begin,end}()
> >
…
> > With EFI gone as the last user of __kernel_fpu_{begin|end}(), both can
> > be made static and not exported anymore.
> >
> This commit removes an exported function pair that is currently used
> by out of tree modules, while the replacement pair
> (kernel_fpu_begin/end) is EXPORT_SYMBOL_GPL. So this is making
> existing functionality GPL only, which will probably be an issue for
> several out of tree modules that use the fpu.
>
> Could kernel_fpu_begin/end be made plain EXPORT_SYMBOL?
It can be used by OOT modules as long as they are not under a
proprietary license. The change here is not for me to decide, I added
the x86 maintainers to make their decision. I can make a patch if they
say so.
On the other hand could we just drop EXPORT_SYMBOL_GPL? I doubt this
helps in any way yet please correct me if I am wrong.
The kernel is GPL and everything links to it should be GPL compatible. People
that don't specify a GPL compatible license either use wrapper around their
binary blob or use EXPORT_SYMBOL functions. The latter group complains each
time a function is not available anymore and we end up changing it to
EXPORT_SYMBOL.
So what do we gain from that EXPORT_SYMBOL_GPL?
> Marc
Sebastian
On Wed, Jan 09, 2019 at 12:19:52PM +0100, Sebastian Andrzej Siewior wrote:
> On 2019-01-07 18:08:26 [-0400], Marc Dionne wrote:
> > On Thu, Dec 27, 2018 at 11:20 AM Linux Kernel Mailing List
> > <[email protected]> wrote:
> > >
> > > Commit: 12209993e98c5fa1855c467f22a24e3d5b8be205
> > > x86/fpu: Don't export __kernel_fpu_{begin,end}()
> > >
> …
> > > With EFI gone as the last user of __kernel_fpu_{begin|end}(), both can
> > > be made static and not exported anymore.
> > >
> > This commit removes an exported function pair that is currently used
> > by out of tree modules, while the replacement pair
> > (kernel_fpu_begin/end) is EXPORT_SYMBOL_GPL. So this is making
> > existing functionality GPL only, which will probably be an issue for
> > several out of tree modules that use the fpu.
> >
> > Could kernel_fpu_begin/end be made plain EXPORT_SYMBOL?
>
> It can be used by OOT modules as long as they are not under a
> proprietary license. The change here is not for me to decide, I added
> the x86 maintainers to make their decision. I can make a patch if they
> say so.
If there are no in-kernel users, the symbols should not be exported
anymore. That's nothing new, we have always done this.
> On the other hand could we just drop EXPORT_SYMBOL_GPL? I doubt this
> helps in any way yet please correct me if I am wrong.
Yes, it helps, please leave it as-is.
thanks,
greg k-h
On 2019-01-09 17:52:35 [+0100], Greg Kroah-Hartman wrote:
> If there are no in-kernel users, the symbols should not be exported
> anymore. That's nothing new, we have always done this.
The thing is that we had
EXPORT_SYMBOL(__kernel_fpu_begin)
EXPORT_SYMBOL_GPL(kernel_fpu_begin)
and now __kernel_fpu_begin() is no longer exported and static only.
All in kernel user (including the kvm module) use kernel_fpu_begin()
which is not available to proprietary modules. Hence Marc's mail.
> > On the other hand could we just drop EXPORT_SYMBOL_GPL? I doubt this
> > helps in any way yet please correct me if I am wrong.
>
> Yes, it helps, please leave it as-is.
As you say. I only notice that certain things used to work and then no
longer do because due to $rework it somehow become EXPORT_SYMBOL_GPL
only and people complain and we tend to switch the export back to
EXPORT_SYMBOL. I'm not aware of a case where it actually helped in
anyway.
> thanks,
>
> greg k-h
Sebastian
On Wed, Jan 9, 2019 at 1:09 PM Sebastian Andrzej Siewior
<[email protected]> wrote:
>
> On 2019-01-09 17:52:35 [+0100], Greg Kroah-Hartman wrote:
> > If there are no in-kernel users, the symbols should not be exported
> > anymore. That's nothing new, we have always done this.
>
> The thing is that we had
> EXPORT_SYMBOL(__kernel_fpu_begin)
> EXPORT_SYMBOL_GPL(kernel_fpu_begin)
>
> and now __kernel_fpu_begin() is no longer exported and static only.
> All in kernel user (including the kvm module) use kernel_fpu_begin()
> which is not available to proprietary modules. Hence Marc's mail.
>
> > > On the other hand could we just drop EXPORT_SYMBOL_GPL? I doubt this
> > > helps in any way yet please correct me if I am wrong.
> >
> > Yes, it helps, please leave it as-is.
>
> As you say. I only notice that certain things used to work and then no
> longer do because due to $rework it somehow become EXPORT_SYMBOL_GPL
> only and people complain and we tend to switch the export back to
> EXPORT_SYMBOL. I'm not aware of a case where it actually helped in
> anyway.
>
> > thanks,
> >
> > greg k-h
>
> Sebastian
I would point out that there are several precedents for restoring
exports after functionality has been unintentionally made GPL only;
from a quick lookup these are some examples:
8af190958059 ("x86/paravirt: Remove GPL from pv_ops export")
31c5bda3a656 ("mm: fix exports that inadvertently make put_page()
EXPORT_SYMBOL_GPL")
1e5476815fd7 ("x86/tlb: Drop the _GPL from the cpu_tlbstate export")
b562c171cf01 ("locking/refcounts: Do not force refcount_t usage as
GPL-only export")
Marc
On Wed, Jan 09, 2019 at 06:09:35PM +0100, Sebastian Andrzej Siewior wrote:
> On 2019-01-09 17:52:35 [+0100], Greg Kroah-Hartman wrote:
> > If there are no in-kernel users, the symbols should not be exported
> > anymore. That's nothing new, we have always done this.
>
> The thing is that we had
> EXPORT_SYMBOL(__kernel_fpu_begin)
> EXPORT_SYMBOL_GPL(kernel_fpu_begin)
>
> and now __kernel_fpu_begin() is no longer exported and static only.
Ok, that is fine.
> All in kernel user (including the kvm module) use kernel_fpu_begin()
> which is not available to proprietary modules. Hence Marc's mail.
But since when did out-of-tree modules use __kernel_fpu_begin? It's an
x86-only thing, and shouldn't really be used by anyone, right?
> > > On the other hand could we just drop EXPORT_SYMBOL_GPL? I doubt this
> > > helps in any way yet please correct me if I am wrong.
> >
> > Yes, it helps, please leave it as-is.
>
> As you say. I only notice that certain things used to work and then no
> longer do because due to $rework it somehow become EXPORT_SYMBOL_GPL
> only and people complain and we tend to switch the export back to
> EXPORT_SYMBOL.
That is a different topic than the whole _GPL symbol export at all.
Please don't conflate the two.
> I'm not aware of a case where it actually helped in anyway.
The use of the _GPL symbol exports has helped in numerous places over
the years. I'd be glad to discuss details over a beverage sometime :)
thanks,
greg k-h
On Wed, Jan 09, 2019 at 01:40:14PM -0400, Marc Dionne wrote:
> On Wed, Jan 9, 2019 at 1:09 PM Sebastian Andrzej Siewior
> <[email protected]> wrote:
> >
> > On 2019-01-09 17:52:35 [+0100], Greg Kroah-Hartman wrote:
> > > If there are no in-kernel users, the symbols should not be exported
> > > anymore. That's nothing new, we have always done this.
> >
> > The thing is that we had
> > EXPORT_SYMBOL(__kernel_fpu_begin)
> > EXPORT_SYMBOL_GPL(kernel_fpu_begin)
> >
> > and now __kernel_fpu_begin() is no longer exported and static only.
> > All in kernel user (including the kvm module) use kernel_fpu_begin()
> > which is not available to proprietary modules. Hence Marc's mail.
> >
> > > > On the other hand could we just drop EXPORT_SYMBOL_GPL? I doubt this
> > > > helps in any way yet please correct me if I am wrong.
> > >
> > > Yes, it helps, please leave it as-is.
> >
> > As you say. I only notice that certain things used to work and then no
> > longer do because due to $rework it somehow become EXPORT_SYMBOL_GPL
> > only and people complain and we tend to switch the export back to
> > EXPORT_SYMBOL. I'm not aware of a case where it actually helped in
> > anyway.
> >
> > > thanks,
> > >
> > > greg k-h
> >
> > Sebastian
>
> I would point out that there are several precedents for restoring
> exports after functionality has been unintentionally made GPL only;
> from a quick lookup these are some examples:
>
> 8af190958059 ("x86/paravirt: Remove GPL from pv_ops export")
> 31c5bda3a656 ("mm: fix exports that inadvertently make put_page()
> EXPORT_SYMBOL_GPL")
> 1e5476815fd7 ("x86/tlb: Drop the _GPL from the cpu_tlbstate export")
> b562c171cf01 ("locking/refcounts: Do not force refcount_t usage as
> GPL-only export")
Yes, but this is a bit different of a thing here. The symbol you wish
to have changed was not modified at all, you just lost access to an
internal function that was fixed up and removed.
Best of luck,
greg k-h