From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] VMX: don't crash processing 'd' debug key Date: Thu, 7 Nov 2013 14:34:31 +0000 Message-ID: <527BA4F7.60500@citrix.com> References: <527B7D3802000078001008A4@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7857619385033484985==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VeQfL-0001EL-Bd for xen-devel@lists.xenproject.org; Thu, 07 Nov 2013 14:34:35 +0000 In-Reply-To: <527B7D3802000078001008A4@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Eddie Dong , Jun Nakajima List-Id: xen-devel@lists.xenproject.org --===============7857619385033484985== Content-Type: multipart/alternative; boundary="------------090709090501040101040504" --------------090709090501040101040504 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 07/11/13 10:44, Jan Beulich wrote: > There's a window during scheduling where "current" and the active VMCS > may disagree: The former gets set much earlier than the latter. Since > both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the > subject vCPU is "current", accessing VMCS fields would, depending on > whether there is any currently active VMCS, either read wrong data, or > cause a crash. > > Going forward we might want to consider reducing the window during > which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when > v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu > == -1), but that would add complexities (acquiring and - more > importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't > look worthwhile adding right now. > > Signed-off-by: Jan Beulich > > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -601,16 +601,16 @@ struct foreign_vmcs { > }; > static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs); > > -void vmx_vmcs_enter(struct vcpu *v) > +bool_t vmx_vmcs_enter(struct vcpu *v) > { > struct foreign_vmcs *fv; > > /* > * NB. We must *always* run an HVM VCPU on its own VMCS, except for > - * vmx_vmcs_enter/exit critical regions. > + * vmx_vmcs_enter/exit and scheduling tail critical regions. > */ > if ( likely(v == current) ) > - return; > + return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs); > > fv = &this_cpu(foreign_vmcs); > > @@ -633,6 +633,8 @@ void vmx_vmcs_enter(struct vcpu *v) > } > > fv->count++; > + > + return 1; > } > > void vmx_vmcs_exit(struct vcpu *v) > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp > { > unsigned long attr = 0, sel = 0, limit; > > - vmx_vmcs_enter(v); > + /* > + * We may get here in the context of dump_execstate(), which may have > + * interrupted context switching between setting "current" and > + * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make > + * all the VMREADs below fail if we don't bail right away. > + */ > + if ( unlikely(!vmx_vmcs_enter(v)) ) > + { > + memset(reg, 0, sizeof(*reg)); > + return; > + } What are the implications of this? All callers unconditionally expect this to succeed, and use the results straight as-are. On the other hand, I am not certain how we could go about dealing with the error. ~Andrew > > switch ( seg ) > { > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h > @@ -143,7 +143,7 @@ struct arch_vmx_struct { > > int vmx_create_vmcs(struct vcpu *v); > void vmx_destroy_vmcs(struct vcpu *v); > -void vmx_vmcs_enter(struct vcpu *v); > +bool_t vmx_vmcs_enter(struct vcpu *v); > void vmx_vmcs_exit(struct vcpu *v); > > #define CPU_BASED_VIRTUAL_INTR_PENDING 0x00000004 > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------090709090501040101040504 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 07/11/13 10:44, Jan Beulich wrote:
There's a window during scheduling where "current" and the active VMCS
may disagree: The former gets set much earlier than the latter. Since
both vmx_vmcs_enter() and vmx_vmcs_exit() immediately return when the
subject vCPU is "current", accessing VMCS fields would, depending on
whether there is any currently active VMCS, either read wrong data, or
cause a crash.

Going forward we might want to consider reducing the window during
which vmx_vmcs_enter() might fail (e.g. doing a plain __vmptrld() when
v->arch.hvm_vmx.vmcs != this_cpu(current_vmcs) but arch_vmx->active_cpu
== -1), but that would add complexities (acquiring and - more
importantly - properly dropping v->arch.hvm_vmx.vmcs_lock) that don't
look worthwhile adding right now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -601,16 +601,16 @@ struct foreign_vmcs {
 };
 static DEFINE_PER_CPU(struct foreign_vmcs, foreign_vmcs);
 
-void vmx_vmcs_enter(struct vcpu *v)
+bool_t vmx_vmcs_enter(struct vcpu *v)
 {
     struct foreign_vmcs *fv;
 
     /*
      * NB. We must *always* run an HVM VCPU on its own VMCS, except for
-     * vmx_vmcs_enter/exit critical regions.
+     * vmx_vmcs_enter/exit and scheduling tail critical regions.
      */
     if ( likely(v == current) )
-        return;
+        return v->arch.hvm_vmx.vmcs == this_cpu(current_vmcs);
 
     fv = &this_cpu(foreign_vmcs);
 
@@ -633,6 +633,8 @@ void vmx_vmcs_enter(struct vcpu *v)
     }
 
     fv->count++;
+
+    return 1;
 }
 
 void vmx_vmcs_exit(struct vcpu *v)
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
 {
     unsigned long attr = 0, sel = 0, limit;
 
-    vmx_vmcs_enter(v);
+    /*
+     * We may get here in the context of dump_execstate(), which may have
+     * interrupted context switching between setting "current" and
+     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
+     * all the VMREADs below fail if we don't bail right away.
+     */
+    if ( unlikely(!vmx_vmcs_enter(v)) )
+    {
+        memset(reg, 0, sizeof(*reg));
+        return;
+    }

What are the implications of this?  All callers unconditionally expect this to succeed, and use the results straight as-are.

On the other hand, I am not certain how we could go about dealing with the error.

~Andrew

 
     switch ( seg )
     {
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
@@ -143,7 +143,7 @@ struct arch_vmx_struct {
 
 int vmx_create_vmcs(struct vcpu *v);
 void vmx_destroy_vmcs(struct vcpu *v);
-void vmx_vmcs_enter(struct vcpu *v);
+bool_t vmx_vmcs_enter(struct vcpu *v);
 void vmx_vmcs_exit(struct vcpu *v);
 
 #define CPU_BASED_VIRTUAL_INTR_PENDING        0x00000004






_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--------------090709090501040101040504-- --===============7857619385033484985== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============7857619385033484985==--