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

[Thomas Gleixner]

On Mon, May 02 2022 at 16:35, Borislav Petkov wrote:
> On Sun, May 01, 2022 at 09:31:47PM +0200, Thomas Gleixner wrote:
>>  /**
>> + * fpregs_lock - Lock FPU state for maintenance operations
>
> "maintenance"?

I meant maintenance of user thread FPU state. Let me rephrase. 

>> + *
>> + * 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?

Yes, FPU can be used legitimately in softirq processing context.

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

Right, though currently it's guaranteed that softirq processing context
can use the FPU. Quite some of the network crypto work runs in softirq
context, so this might cause a regression. If so, then this needs to be
an explicit commit on top which is easy to revert. Let me stare at it
some more.

>> @@ -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()...

Similar, but not identical and we cannot use fpregs_lock() here as we
don't want to have local_bh_disable() when in hard interrupt context.

>>  	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?

Not really, unless we drop the use FPU in softirq processing context
guarantee. See above.

Let me think about it.

Thanks,

        tglx