Re: [PATCH 0/5] kvm: fix latent guest entry/exit bugs

[Mark Rutland]

On Fri, Jan 14, 2022 at 01:29:46PM +0100, Christian Borntraeger wrote:
> 
> 
> Am 14.01.22 um 13:19 schrieb Mark Rutland:
> > On Thu, Jan 13, 2022 at 04:20:07PM +0100, Christian Borntraeger wrote:
> > > Am 11.01.22 um 16:35 schrieb Mark Rutland:
> > > > Several architectures have latent bugs around guest entry/exit, most
> > > > notably:
> > > > 
> > > > 1) Several architectures enable interrupts between guest_enter() and
> > > >      guest_exit(). As this period is an RCU extended quiescent state (EQS) this
> > > >      is unsound unless the irq entry code explicitly wakes RCU, which most
> > > >      architectures only do for entry from usersapce or idle.
> > > > 
> > > >      I believe this affects: arm64, riscv, s390
> > > > 
> > > >      I am not sure about powerpc.
> > > > 
> > > > 2) Several architectures permit instrumentation of code between
> > > >      guest_enter() and guest_exit(), e.g. KASAN, KCOV, KCSAN, etc. As
> > > >      instrumentation may directly o indirectly use RCU, this has the same
> > > >      problems as with interrupts.
> > > > 
> > > >      I believe this affects: arm64, mips, powerpc, riscv, s390
> > > > 
> > > > 3) Several architectures do not inform lockdep and tracing that
> > > >      interrupts are enabled during the execution of the guest, or do so in
> > > >      an incorrect order. Generally
> > > >      this means that logs will report IRQs being masked for much longer
> > > >      than is actually the case, which is not ideal for debugging. I don't
> > > >      know whether this affects the correctness of lockdep.
> > > > 
> > > >      I believe this affects: arm64, mips, powerpc, riscv, s390
> > > > 
> > > > This was previously fixed for x86 specifically in a series of commits:
> > > > 
> > > >     87fa7f3e98a1310e ("x86/kvm: Move context tracking where it belongs")
> > > >     0642391e2139a2c1 ("x86/kvm/vmx: Add hardirq tracing to guest enter/exit")
> > > >     9fc975e9efd03e57 ("x86/kvm/svm: Add hardirq tracing on guest enter/exit")
> > > >     3ebccdf373c21d86 ("x86/kvm/vmx: Move guest enter/exit into .noinstr.text")
> > > >     135961e0a7d555fc ("x86/kvm/svm: Move guest enter/exit into .noinstr.text")
> > > >     160457140187c5fb ("KVM: x86: Defer vtime accounting 'til after IRQ handling")
> > > >     bc908e091b326467 ("KVM: x86: Consolidate guest enter/exit logic to common helpers")
> > > > 
> > > > But other architectures were left broken, and the infrastructure for
> > > > handling this correctly is x86-specific.
> > > > 
> > > > This series introduces generic helper functions which can be used to
> > > > handle the problems above, and migrates architectures over to these,
> > > > fixing the latent issues.
> > > > 
> > > > I wasn't able to figure my way around powerpc and s390, so I have not
> > > 
> > > I think 2 later patches have moved the guest_enter/exit a bit out.
> > > Does this make the s390 code clearer?
> > 
> > Yes; that's much simpler to follow!
> > 
> > One major thing I wasn't sure about for s390 is the sequence:
> > 
> > 	guest_enter_irqoff();	// Enters an RCU EQS
> > 	...
> > 	local_irq_enable();
> > 	...
> > 	sie64a(...);
> > 	...
> > 	local_irq_disable();
> > 	...
> > 	guest_exit_irqoff();	// Exits an RCU EQS
> > 
> > ... since if an IRQ is taken between local_irq_{enable,disable}(), RCU won't be
> > watching, and I couldn't spot whether your regular IRQ entry logic would wake
> > RCU in this case, or whether there was something else I'm missing that saves
> > you here.
> > 
> > For other architectures, including x86 and arm64, we enter the guest with IRQs
> > masked and return from the guest with IRQs masked, and don't actually take IRQs
> > until we unmask them in the host, after the guest_exit_*() logic has woken RCU
> > and so on.
> > 
> > I wasn't able to find documentation on the semantics of SIE, so I couldn't spot
> > whether the local_irq_{enable,disable}() calls were necessary, or could be
> > removed.
> 
> We run the SIE instruction with interrupts enabled. SIE is interruptible.
> The disable/enable pairs are just because  guest_enter/exit_irqoff() require them.

What I was trying to figure out was when an interrupt is taken between
guest_enter_irqoff() and guest_exit_irqoff(), where is RCU woken? I couldn't
spot that in the s390 entry code (probably simply because I'm not familiar with
it), and so AFAICT that means IRQ code could run without RCU watching, which
would cause things to explode.

On other architectures that problem is avoided because IRQs asserted during the
guest cause a specific guest exit rather than a regular IRQ exception, and the
HW enables/disables IRQs when entering/exiting the guest, so the host can leave
IRQs masked across guest_enter_irqoff()..guest_exit_irqoff().

Am I right in understanding that SIE itself won't enable (host) interrupts
while running the guest, and so it *needs* to be run with interrupts already
enabled?

> One thing to be aware of: in our entry.S - after an interrupt - we leave SIE by
> setting the return address of the interrupt after the sie instruction so that we
> get back into this __vcpu_run loop to check for signals and so.

Just to check, that's after the IRQ handler runs, right?

Thanks,
Mark.