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

[Borislav Petkov <bp@alien8.de>]

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.

https://people.kernel.org/tglx/notes-about-netiquette