Messages in this thread | | | Date | Mon, 2 May 2022 16:35:10 +0200 | From | Borislav Petkov <> | Subject | Re: [patch 3/3] x86/fpu: Make FPU protection more robust |
| |
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
| |