Re: [Xen-devel] [PATCH] VMX: don't crash processing 'd' debug key

[Tim Deegan <tim@xxxxxxx>]

At 10:44 +0000 on 07 Nov (1383817496), 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@xxxxxxxx>
> 
> --- 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;

It would be nice to print something here, at least on the first
instance.  Otherwise someone looking at bizarre debugkey output would
have to know (and remember) about this path.

I'd also be inclined to ASSERT that, e.g. interrupts are disabled here
-- if for any reason this function ever starts corrupting register
state on other paths, we'll want to know about it quickly!

Cheers,

Tim.

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