Re: [patch 3/3] x86/fpu: Make FPU protection more robust

Borislav Petkov
2022-05-02T14:36:14Z

On Sun, May 01, 2022 at 09:31:47PM +0200, Thomas Gleixner wrote:
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -42,7 +42,7 @@ struct fpu_state_config fpu_user_cfg __r
> struct fpstate init_fpstate __ro_after_init;
>
> /* Track in-kernel FPU usage */
> -static DEFINE_PER_CPU(bool, in_kernel_fpu);
> +static DEFINE_PER_CPU(bool, fpu_in_use);
>
> /*
> * Track which context is using the FPU on the CPU:
> @@ -50,6 +50,50 @@ static DEFINE_PER_CPU(bool, in_kernel_fp
> DEFINE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
>
> /**
> + * fpregs_lock - Lock FPU state for maintenance operations

"maintenance"?

> + *
> + * This protects against preemption, soft interrupts and in-kernel FPU
> + * usage on both !RT and RT enabled kernels.
> + *
> + * !RT kernels use local_bh_disable() to prevent soft interrupt processing
> + * and preemption.
> + *
> + * On RT kernels local_bh_disable() is not sufficient because it only
> + * serializes soft interrupt related sections via a local lock, but stays
> + * preemptible. Disabling preemption is the right choice here as bottom
> + * half processing is always in thread context on RT kernels so it
> + * implicitly prevents bottom half processing as well.
> + */
> +void fpregs_lock(void)
> +{
> + if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> + local_bh_disable();
> + else
> + preempt_disable();

So I'm wondering: can we get rid of this distinction and simply do
preempt_disable()?

Or can FPU be used in softirq processing too so we want to block that
there?

But even if, fpu_in_use will already state that fact...

...

> @@ -410,10 +433,9 @@ void kernel_fpu_begin_mask(unsigned int
> {
> preempt_disable();
>
> - WARN_ON_FPU(!kernel_fpu_usable());
> - WARN_ON_FPU(this_cpu_read(in_kernel_fpu));
> + WARN_ON_ONCE(!kernel_fpu_usable());
>
> - this_cpu_write(in_kernel_fpu, true);
> + this_cpu_write(fpu_in_use, true);

This starts to look awfully similar to fpregs_lock()...

>
> if (!(current->flags & PF_KTHREAD) &&
> !test_thread_flag(TIF_NEED_FPU_LOAD)) {
> @@ -433,9 +455,9 @@ EXPORT_SYMBOL_GPL(kernel_fpu_begin_mask)
>
> void kernel_fpu_end(void)
> {
> - WARN_ON_FPU(!this_cpu_read(in_kernel_fpu));
> + WARN_ON_ONCE(!this_cpu_read(fpu_in_use));
>
> - this_cpu_write(in_kernel_fpu, false);
> + this_cpu_write(fpu_in_use, false);
> preempt_enable();

... and this to fpregs_unlock().

Can we use those here too instead of open-coding them mostly?

--
Regards/Gruss,
Boris.