[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for-4.12 V2] x86/altp2m: fix HVMOP_altp2m_set_domain_state race



On 2/8/19 4:00 PM, Razvan Cojocaru wrote:
HVMOP_altp2m_set_domain_state does not domain_pause(), presumably
on purpose (as it was originally supposed to cater to a in-guest
agent, and a domain pausing itself is not a good idea).

This can lead to domain crashes in the vmx_vmexit_handler() code
that checks if the guest has the ability to switch EPTP without an
exit. That code can __vmread() the host p2m's EPT_POINTER
(before HVMOP_altp2m_set_domain_state "for_each_vcpu()" has a
chance to run altp2m_vcpu_initialise(), but after
d->arch.altp2m_active is set).

This patch reorganizes the code so that d->arch.altp2m_active
is set to true only after all the init work has been done, and
to false before the uninit work begins. This required adding
a new bool parameter altp2m_vcpu_update_p2m(), which relied
on d->arch.altp2m_active being set before it's called.

While at it, I've changed a couple of bool_t's to bool's.

Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
---
  xen/arch/x86/hvm/hvm.c        | 17 ++++++++++++++---
  xen/arch/x86/hvm/vmx/vmx.c    |  4 ++--
  xen/arch/x86/mm/altp2m.c      |  4 ++--
  xen/arch/x86/mm/p2m.c         |  4 ++--
  xen/include/asm-x86/altp2m.h  |  2 +-
  xen/include/asm-x86/domain.h  |  2 +-
  xen/include/asm-x86/hvm/hvm.h |  6 +++---
  7 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 21944e9..4d1d13d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4525,7 +4525,7 @@ static int do_altp2m_op(
      case HVMOP_altp2m_set_domain_state:
      {
          struct vcpu *v;
-        bool_t ostate;
+        bool ostate, nstate;
if ( nestedhvm_enabled(d) )
          {
@@ -4534,12 +4534,16 @@ static int do_altp2m_op(
          }
ostate = d->arch.altp2m_active;
-        d->arch.altp2m_active = !!a.u.domain_state.state;
+        nstate = !!a.u.domain_state.state;
/* If the alternate p2m state has changed, handle appropriately */
-        if ( d->arch.altp2m_active != ostate &&
+        if ( nstate != ostate &&
               (ostate || !(rc = p2m_init_altp2m_by_id(d, 0))) )
          {
+            /* First mark altp2m as disabled, then altp2m_vcpu_destroy(). */
+            if ( ostate )
+                d->arch.altp2m_active = false;
+
              for_each_vcpu( d, v )
              {
                  if ( !ostate )
@@ -4550,7 +4554,14 @@ static int do_altp2m_op(
if ( ostate )
                  p2m_flush_altp2m(d);
+            else
+                /*
+                 * Wait until altp2m_vcpu_initialise() is done before marking
+                 * altp2m as being enabled for the domain.
+                 */
+                d->arch.altp2m_active = true;
          }
+
          break;
      }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 64af8bf..e542568 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2150,13 +2150,13 @@ static bool_t vmx_is_singlestep_supported(void)
      return !!cpu_has_monitor_trap_flag;
  }
-static void vmx_vcpu_update_eptp(struct vcpu *v)
+static void vmx_vcpu_update_eptp(struct vcpu *v, bool altp2m_enabled)
  {
      struct domain *d = v->domain;
      struct p2m_domain *p2m = NULL;
      struct ept_data *ept;
- if ( altp2m_active(d) )
+    if ( altp2m_enabled )
          p2m = p2m_get_altp2m(v);
      if ( !p2m )
          p2m = p2m_get_hostp2m(d);
diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
index 930bdc2..c51303b 100644
--- a/xen/arch/x86/mm/altp2m.c
+++ b/xen/arch/x86/mm/altp2m.c
@@ -39,7 +39,7 @@ altp2m_vcpu_initialise(struct vcpu *v)
      vcpu_altp2m(v).p2midx = 0;
      atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
- altp2m_vcpu_update_p2m(v);
+    altp2m_vcpu_update_p2m(v, true);
if ( v != current )
          vcpu_unpause(v);
@@ -58,7 +58,7 @@ altp2m_vcpu_destroy(struct vcpu *v)
altp2m_vcpu_reset(v); - altp2m_vcpu_update_p2m(v);
+    altp2m_vcpu_update_p2m(v, false);
      altp2m_vcpu_update_vmfunc_ve(v);
if ( v != current )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d14ce57..f9e96fc 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2332,7 +2332,7 @@ bool_t p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, 
unsigned int idx)
              atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
              vcpu_altp2m(v).p2midx = idx;
              atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
-            altp2m_vcpu_update_p2m(v);
+            altp2m_vcpu_update_p2m(v, altp2m_active(d));
          }
          rc = 1;
      }
@@ -2573,7 +2573,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, 
unsigned int idx)
                  atomic_dec(&p2m_get_altp2m(v)->active_vcpus);
                  vcpu_altp2m(v).p2midx = idx;
                  atomic_inc(&p2m_get_altp2m(v)->active_vcpus);
-                altp2m_vcpu_update_p2m(v);
+                altp2m_vcpu_update_p2m(v, altp2m_active(d));
              }
rc = 0;
diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h
index 3befcf6..e3df27f 100644
--- a/xen/include/asm-x86/altp2m.h
+++ b/xen/include/asm-x86/altp2m.h
@@ -33,7 +33,7 @@ static inline bool altp2m_active(const struct domain *d)
  /* Alternate p2m VCPU */
  void altp2m_vcpu_initialise(struct vcpu *v);
  void altp2m_vcpu_destroy(struct vcpu *v);
-void altp2m_vcpu_reset(struct vcpu *v);
+void altp2m_vcpu_disable_ve(struct vcpu *v);

Sorry, this is a leftover, unrelated change. I'll remove it in the next version, first I'll wait for other comments.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.