Message-ID: <20250518200114.GA1764@sol>
Date: Sun, 18 May 2025 13:01:14 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: Ingo Molnar <mingo@...nel.org>, x86@...nel.org,
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
	linux-pm@...r.kernel.org, Borislav Petkov <bp@...en8.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ayush Jain <Ayush.Jain3@....com>,
	Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH 3/3] x86/fpu: Don't support kernel-mode FPU when
 irqs_disabled()

On Sun, May 18, 2025 at 03:18:58PM +0200, Ard Biesheuvel wrote:
> On Sun, 18 May 2025 at 08:34, Ingo Molnar <mingo@...nel.org> wrote:
> >
> >
> > * Eric Biggers <ebiggers@...nel.org> wrote:
> >
> > > > Alternatively we could set in_kernel_fpu during CPU bootstrap, and
> > > > clear it once we know the FPU is usable? This is only a relatively
> > > > short early boot period, with no scheduling, right?
> > >
> > > Yes, if there isn't agreement on this approach we can do that
> > > instead.  Say:
> > >
> > > - Replace in_kernel_fpu with kernel_fpu_supported, with the opposite
> > >   meaning (so that the initial value of false means "unsupported")
> >
> > I'm not against simplifying the x86 FPU model to exclude IRQs-off
> > context (especially if it also micro-optimizes some of the key runtime
> > kernel-FPU primitives), but it has to be a full solution and we'll have
> > to see how complicated the EFI changes get.
> >
> > Ie. without seeing the full cost-benefit balance it's hard to call this
> > in advance. Mind sending a full series that addresses the EFI case too?
> >
> 
> EFI services are only called with IRQs disabled in exceptional cases,
> so it would be unfortunate if it prevents us from make meaningful
> improvements here. In ordinary cases, they are called from a
> workqueue, and I'd prefer it if we can address this without calling
> all EFI services with interrupts disabled either.
> 
> AIUI, the reason we cannot tolerate IRQs being disabled is because
> re-enabling softirqs will complain if IRQs are disabled, due to the
> fact that handling softirqs should not be attempted at that point?
> 
> I don't know the history here, but I wonder if that isn't overly
> pedantic? Disabling softirqs could be avoided entirely when IRQs are
> off, given that they are disabled implicitly already. But why then is
> it not permitted to disable and re-enable softirqs under this
> condition, given that it makes no difference? Or perhaps I'm missing
> something here.
> 
> A good way to trigger such an exceptional case is running a kernel
> with efi-pstore and lkdtm built-in under QEMU with OVMF, and do
> 
> # 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.  local_bh_enable() already uses
local_irq_save(flags) instead of local_irq_disable(), so it seems it's sort of
intended to work when IRQs are disabled, despite the
lockdep_assert_irqs_enabled().

Anyway, that would point to continuing to support kernel-mode FPU when IRQs are
disabled.  But also EFI needs it anyway, unless we refactor it to use
kernel_fpu_begin() and kernel_fpu_end() only when irq_fpu_usable() and otherwise
use different code, analogous what arm64 does.

So for now I've sent
https://lore.kernel.org/lkml/20250518193212.1822-1-ebiggers@kernel.org which
implements the other possible fix, where we just start keeping track of whether
the FPU has been initialized or not.

- Eric

Powered by blists - more mailing lists