linux-kernel - Re: [PATCH v3 1/2] KVM: Disable IRQs in kvm_online_cpu()/kvm_offline

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250917131151.GA100623@k08j02272.eu95sqa>
Date: Wed, 17 Sep 2025 21:11:51 +0800
From: Hou Wenlong <houwenlong.hwl@...group.com>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>,
	"Gao, Chao" <chao.gao@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] KVM: Disable IRQs in
 kvm_online_cpu()/kvm_offline_cpu()

On Tue, Sep 16, 2025 at 10:12:03AM +0000, Huang, Kai wrote:
> On Tue, 2025-09-16 at 14:07 +0800, Hou Wenlong wrote:
> > After the commit aaf12a7b4323 ("KVM: Rename and move
> > CPUHP_AP_KVM_STARTING to ONLINE section"), KVM's hotplug callbacks have
> > been moved into the ONLINE section, where IRQs and preemption are
> > enabled according to the documentation. However, if IRQs are not
> > guaranteed to be disabled, it could theoretically be a bug, because
> > virtualization_enabled may be stale (with respect to the actual state of
> > the hardware) when read from IRQ context, making the callback
> > potentially reentrant. Therefore, disable IRQs in kvm_online_cpu() and
> > kvm_offline_cpu() to ensure that all paths for
> > kvm_enable_virtualization_cpu() and kvm_disable_virtualization_cpu() are
> > in an IRQ-disabled state.
> 
> Reading the v1 thread [*], IIUC the "virtualization_enabled being stale
> when read from IRQ context" is referring to the case where
> kvm_disable_virtualization_cpu() got interrupted by IRQ and re-entered.
> 
> But IIUC this shouldn't happen.  If I am not missing anything, the
> syscore_shutdown() (from which KVM sends IRQ to call
> kvm_disable_virtualization_cpu()) is always called after
> migrate_to_reboot_cpu(), which internally waits for currently running CPU
> hotplug to complete (if any) and disables future CPU hotplug.  Therefore
> it shouldn't be possible that kvm_disable_virtualization_cpu() could be
> interrupted and re-entered via IRQ.
> 
Yes, you are right. The syscore_ops are exclusive hotplug callbacks, so
there are actually no bugs, and I didn't add a Fix tag here. The above
description is just an assumption in case it is interrupted, as the
callback is not in an IRQ-disabled state. Sorry, I forgot to include the
important part you mentioned in my commit message.

> I don't oppose this code change, but I think this should somehow
> documented in the changelog, if I am not missing anything?
> 
> [*]: https://lore.kernel.org/kvm/aMirvo9Xly5fVmbY@google.com/
> > 
> > Suggested-by: Sean Christopherson <seanjc@...gle.com>
> > Signed-off-by: Hou Wenlong <houwenlong.hwl@...group.com>
> > ---
> >  virt/kvm/kvm_main.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 18f29ef93543..cf8dddeed37e 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -5580,6 +5580,8 @@ __weak void kvm_arch_disable_virtualization(void)
> >  
> >  static int kvm_enable_virtualization_cpu(void)
> >  {
> > +	lockdep_assert_irqs_disabled();
> > +
> >  	if (__this_cpu_read(virtualization_enabled))
> >  		return 0;
> >  
> > @@ -5595,6 +5597,8 @@ static int kvm_enable_virtualization_cpu(void)
> >  
> >  static int kvm_online_cpu(unsigned int cpu)
> >  {
> > +	guard(irqsave)();
> > +
> >  	/*
> >  	 * Abort the CPU online process if hardware virtualization cannot
> >  	 * be enabled. Otherwise running VMs would encounter unrecoverable
> > @@ -5605,6 +5609,8 @@ static int kvm_online_cpu(unsigned int cpu)
> >  
> >  static void kvm_disable_virtualization_cpu(void *ign)
> >  {
> > +	lockdep_assert_irqs_disabled();
> > +
> >  	if (!__this_cpu_read(virtualization_enabled))
> >  		return;
> >  
> > @@ -5615,6 +5621,8 @@ static void kvm_disable_virtualization_cpu(void *ign)
> >  
> >  static int kvm_offline_cpu(unsigned int cpu)
> >  {
> > +	guard(irqsave)();
> > +
> >  	kvm_disable_virtualization_cpu(NULL);
> >  	return 0;
> >  }
> > @@ -5648,7 +5656,6 @@ static int kvm_suspend(void)
> >  	 * dropped all locks (userspace tasks are frozen via a fake signal).
> >  	 */
> >  	lockdep_assert_not_held(&kvm_usage_lock);
> > -	lockdep_assert_irqs_disabled();
> >  
> >  	kvm_disable_virtualization_cpu(NULL);
> >  	return 0;
> > @@ -5657,7 +5664,6 @@ static int kvm_suspend(void)
> >  static void kvm_resume(void)
> >  {
> >  	lockdep_assert_not_held(&kvm_usage_lock);
> > -	lockdep_assert_irqs_disabled();
> >  
> >  	WARN_ON_ONCE(kvm_enable_virtualization_cpu());
> >  }
> > 
> > base-commit: a6ad54137af92535cfe32e19e5f3bc1bb7dbd383