Re: [PATCH 3/3] x86/fpu: Don't support kernel-mode FPU when irqs_disabled()

[Ingo Molnar <mingo@kernel.org>]

* Ard Biesheuvel <ardb@kernel.org> wrote:

> On Mon, 19 May 2025 at 14:57, Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > > On Mon, 19 May 2025 at 10:06, Ingo Molnar <mingo@kernel.org> wrote:
> > > >
> > > >
> > > > * Eric Biggers <ebiggers@kernel.org> wrote:
> > > >
> > > > > > # echo PANIC > /sys/kernel/debug/provoke-crash/DIRECT
> > > > > >
> > > > > > Another case that likely executes with IRQs disabled (but I 
> > > > > > haven't double checked) is reset_system(), which may return 
> > > > > > with an error, or reboot/poweroff the machine and never 
> > > > > > return.
> > > > >
> > > > > That makes sense to me.  preempt_disable() and 
> > > > > preempt_enable() are already allowed when IRQs are disabled, 
> > > > > and I'm not sure why local_bh_disable() and local_bh_enable() 
> > > > > are different.
> > > >
> > > > Because local_bh_enable() may run softirq handlers immediately 
> > > > if there's pending softirqs, which shouldn't be done in hardirq 
> > > > context.
> > >
> > > Sure, but why is that mandatory?
> > >
> > >
> > > preempt_disable() has preempt_enable() and 
> > > preempt_enable_no_resched() counterparts.
> >
> > > [...] Could we have a local_bh_enable_no_xxx() version that
> > > re-enables async softirq processing on the current CPU but does not
> > > kick off a synchronous processing run?
> >
> > Yes, that's what __local_bh_enable() does, but if used it for
> > kernel_fpu_end() we'd be introducing random softirq processing
> > latencies. The softirq execution model is for softirqs to be
> > immediately executed after local_bh_enable(), and various networking
> > code is tuned to that behavior.
> >
> 
> All of that only applies when re-enabling softirqs with IRQs enabled.

Yes, of course. BHs in the networking code are typically 
disabled/enabled when IRQs are on. It's the whole *point* of the 
facility: it was written as a 'lightweight' IRQs-on/off facility 
originally, back in the days when local_irq_save() was very expensive, 
especially on non-x86 platforms.

> I don't see why we'd need all of that.
> 
> Conceptually, kernel_fpu_end() would do
> 
> if (irqs_disabled())
>    local_bh_enable_no_xxx();
> else
>    local_bh_enable();

In normal kernel code local_bh_enable() obviously cannot be done with 
IRQs off, and local_bh_disable()/__local_bh_enable() is highly frowned 
upon because it's generally pointless: if irqs are off to begin with, 
why disable any BHs?

What you probably mean is to only disable BHs if IRQs are not off 
(because hardirqs-off state already disables BHs):

  kernel_fpu_begin()
	if (!irqs_disabled)
		local_bh_disable();

  kernel_fpu_end()
	if (!irqs_disabled())
		local_bh_enable();

... which is basically what the current code does:

        if (!irqs_disabled())
                fpregs_lock();

	...

        if (!irqs_disabled())
                fpregs_unlock();

BTW., maybe we should add a lockdep check to make sure we never enable 
hardirqs while kernel FPU handling is ongoing. It should be relatively 
straightforward and cheap.

But that brings is far away from the original question:

> > > > > preempt_disable() and preempt_enable() are already allowed 
> > > > > when IRQs are disabled, and I'm not sure why 
> > > > > local_bh_disable() and local_bh_enable() are different.
> > > >
> > > > Because local_bh_enable() may run softirq handlers immediately 
> > > > if there's pending softirqs, which shouldn't be done in hardirq 
> > > > context.

To rephrase my answer: local_bh_disable()/enable() are part of the 
softirq execution mechanism whose primary historical purpose was to be 
a lighter-weight replacement hardirq disable/enable critical sections, 
combined with a relaxation of how long a softirq could run versus a 
hardirq. It makes little sense to try to nest BH handling primitives to 
within hardirq critical sections.

Thanks,

	Ingo