Re: [PATCH] KVM: X86: Fix exception untrigger on ret to user

From: Jim Mattson <jmattson@google.com>
To: stsp <stsp2@yandex.ru>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Joerg Roedel <joro@8bytes.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	kvm@vger.kernel.org
Subject: Re: [PATCH] KVM: X86: Fix exception untrigger on ret to user
Date: Mon, 28 Jun 2021 10:44:15 -0700	[thread overview]
Message-ID: <CALMp9eSfC0LC4qCUNHv4qfvP=HhErQmVNqmnzfzebpOOMCLjeQ@mail.gmail.com> (raw)
In-Reply-To: <a6a8fe0b-1bd3-6a1a-22bb-bfc493f2a195@yandex.ru>

On Mon, Jun 28, 2021 at 10:06 AM stsp <stsp2@yandex.ru> wrote:
>
> 28.06.2021 19:19, Jim Mattson пишет:
> > This doesn't work. Kvm has no facilities for converting an injected
> > exception back into a pending exception.
>
> What is the purpose of the
> cancel_injection then?

I believe cancel_injection exists for serializing the vCPU state. If
the vCPU is saved and restored, then you don't want to lose the
'injected' event that is sitting in the VMCS.

>
> >   In particular, if the
> > exception has side effects, such as a #PF which sets CR2, those side
> > effects have already taken place. Once kvm sets the VM-entry
> > interruption-information field, the next VM-entry must deliver that
> > exception. You could arrange to back it out, but you would also have
> > to back out the changes to CR2 (for #PF) or DR6 (for #DB).
> >
> > Cancel_injection *should* leave the exception in the 'injected' state,
>
> But it removes it from VMCS, no?
> I thought "injected=true" means
> "injected to VMCS". What is the
> difference between "injected" and
> "pending" if both may or may not
> mean "already in VMCS"?

Pending events have not yet been subject to interception by L1 (if L2
is active). Their side effects have not yet been applied. When a
pending event is processed, if L2 is active, kvm checks L1's exception
bitmap to see if the event should cause an emulated VM-exit from L2 to
L1. If not, the pending event transitions to an injected event and the
side effects are applied. (At this point, the event is essentially
committed.) The event is then written to the VM-entry
interruption-information field to take advantage of hardware
assistance for vectoring through the IDT.

Perhaps 'committed' would have been a better term than 'injected.'

> > and KVM_SET_REGS *should not* clear an injected exception. (I don't
> > think it's right to clear a pending exception either, if that
> > exception happens to be a trap, but that's a different discussion).
> >
> > It seems to me that the crux of the problem here is that
> > run->ready_for_interrupt_injection returns true when it should return
> > false. That's probably where you should focus your efforts.
>
> I tried that already, and showed
> the results to you. :) Alas, you didn't
> reply to those.

I haven't had the time. I was hoping that someone else on the kvm list
would help you.

> But why do you suggest the cpu-specific
> approach? All other CPUs exit to user-space
> only when the exception is _really_ injected,
> i.e. CS/EIP points to the IDT handler.
> I don't see why it should be non-atomic
> just for one CPU. Shouldn't that be atomic
> for all CPUs?

This isn't CPU-specific. Even when using EPT, you can potentially end
up in this state after an EPT violation during IDT vectoring.