LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Aaron Lewis <aaronlewis@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Peter Shier <pshier@google.com>
Subject: Re: [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete
Date: Wed, 8 May 2019 11:13:39 -0700	[thread overview]
Message-ID: <20190508181339.GD19656@linux.intel.com> (raw)
In-Reply-To: <CAAAPnDE0ujH4eTX=4umTTEmUMyaZ7M0B3qxWa7oUUD-Ls7Ta+A@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

On Wed, May 08, 2019 at 10:53:12AM -0700, Aaron Lewis wrote:
> nested_run_pending is also checked in
> nested_vmx_check_vmentry_postreqs
> (https://elixir.bootlin.com/linux/v5.1/source/arch/x86/kvm/vmx/nested.c#L2709)
> so I think the setting needs to be moved to just prior to that call
> with Paolo's rollback along with another for if the prereqs and
> postreqs fail.  I put a patch together below:

Gah, I missed that usage (also, it's now nested_vmx_check_guest_state()).

Side topic, I think the VM_ENTRY_LOAD_BNDCFGS check should be gated by
nested_run_pending, a la the EFER check.'

> ------------------------------------
> 
> nested_run_pending=1 implies we have successfully entered guest mode.
> Move setting from external state in vmx_set_nested_state() until after
> all other checks are complete.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> Reviewed-by: Peter Shier <pshier@google.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 6401eb7ef19c..cf1f810223d2 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5460,9 +5460,6 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>   if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE))
>   return 0;
> 
> - vmx->nested.nested_run_pending =
> - !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING);

Alternatively, it might be better to leave nested_run_pending where it
is and instead add a label to handle clearing the flag on error.  IIUC,
the real issue is that nested_run_pending is left set after a failed
vmx_set_nested_state(), not that its shouldn't be set in the shadow
VMCS handling.

Patch attached, though it's completely untested.  The KVM selftests are
broken for me right now, grrr.

> -
>   if (nested_cpu_has_shadow_vmcs(vmcs12) &&
>       vmcs12->vmcs_link_pointer != -1ull) {
>   struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
> @@ -5480,14 +5477,21 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
>   return -EINVAL;
>   }
> 
> + vmx->nested.nested_run_pending =
> + !!(kvm_state->flags & KVM_STATE_NESTED_RUN_PENDING);
> +
>   if (nested_vmx_check_vmentry_prereqs(vcpu, vmcs12) ||
> -     nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual))
> +     nested_vmx_check_vmentry_postreqs(vcpu, vmcs12, &exit_qual)) {
> +     vmx->nested.nested_run_pending = 0;
>   return -EINVAL;
> + }
> 
>   vmx->nested.dirty_vmcs12 = true;
>   ret = nested_vmx_enter_non_root_mode(vcpu, false);
> - if (ret)
> + if (ret) {
> + vmx->nested.nested_run_pending = 0;
>   return -EINVAL;
> + }
> 
>   return 0;
>  }

[-- Attachment #2: 0001-KVM-nVMX-Clear-nested_run_pending-if-setting-nested-.patch --]
[-- Type: text/x-diff, Size: 1974 bytes --]

From 279ce1be96d74aee41e93b597572e612a143cf3c Mon Sep 17 00:00:00 2001
From: Sean Christopherson <sean.j.christopherson@intel.com>
Date: Wed, 8 May 2019 11:04:32 -0700
Subject: [PATCH] KVM: nVMX: Clear nested_run_pending if setting nested state
 fails

VMX's nested_run_pending flag is subtly consumed when stuffing state to
enter guest mode, i.e. needs to be set according before KVM knows if
setting guest state is successful.  If setting guest state fails, clear
the flag as a nested run is obviously not pending.

Reported-by: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/nested.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 04b40a98f60b..1a2a2f91b7e0 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5428,29 +5428,33 @@ static int vmx_set_nested_state(struct kvm_vcpu *vcpu,
 		struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
 
 		if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
-			return -EINVAL;
+			goto error_guest_mode;
 
 		if (copy_from_user(shadow_vmcs12,
 				   user_kvm_nested_state->data + VMCS12_SIZE,
 				   sizeof(*vmcs12)))
-			return -EFAULT;
+			goto error_guest_mode;
 
 		if (shadow_vmcs12->hdr.revision_id != VMCS12_REVISION ||
 		    !shadow_vmcs12->hdr.shadow_vmcs)
-			return -EINVAL;
+			goto error_guest_mode;
 	}
 
 	if (nested_vmx_check_controls(vcpu, vmcs12) ||
 	    nested_vmx_check_host_state(vcpu, vmcs12) ||
 	    nested_vmx_check_guest_state(vcpu, vmcs12, &exit_qual))
-		return -EINVAL;
+		goto error_guest_mode;
 
 	vmx->nested.dirty_vmcs12 = true;
 	ret = nested_vmx_enter_non_root_mode(vcpu, false);
 	if (ret)
-		return -EINVAL;
+		goto error_guest_mode;
 
 	return 0;
+
+error_guest_mode:
+	vmx->nested.nested_run_pending = 0;
+	return -EINVAL;
 }
 
 void nested_vmx_vcpu_setup(void)
-- 
2.21.0


  reply	other threads:[~2019-05-08 18:13 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-08 12:16 Paolo Bonzini
2019-05-08 14:20 ` Sean Christopherson
2019-05-08 17:53   ` Aaron Lewis
2019-05-08 18:13     ` Sean Christopherson [this message]
2019-05-08 21:13       ` Aaron Lewis
2019-05-15 16:44         ` Aaron Lewis

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190508181339.GD19656@linux.intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=aaronlewis@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=pshier@google.com \
    --subject='Re: [PATCH v2] kvm: nVMX: Set nested_run_pending in vmx_set_nested_state after checks complete' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).