[Xen-devel] [PATCH for-4.12 V5 1/2] altp2m: Prevent deadlocks when a domain performs altp2m operations on itself

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

domain_pause_except_self() was introduced to allow a domain to pause
itself while doing altp2m operations.  However, as written, it has a
risk fo deadlock if two vcpus enter the loop at the same time.

Luckily, there's already a solution for this: Attempt to call domain's
hypercall_deadlock_mutex, and restart the entire hypercall if you
fail.

Make domain_pause_except_self() attempt to grab this mutex when
pausing itself, returning -ERESTART if it fails.  Have the callers
check for errors and pass the value up.  In both cases, the top-level
do_hvm_op() should DTRT when -ERESTART is returned.

The (necessary) reuse of the hypercall deadlock mutex poses the risk
of getting called from a context where the lock was already acquired
(e.g. someone may (say) call domctl_lock(), then afterwards call
domain_pause_except_self()). However, in the interest of not
overcomplicating things, no changes are made here to the mutex.
Attempted nesting of this lock isn't a security issue, because all
that will happen is that the vcpu will livelock taking continuations.

Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
Tested-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
---
Release exception justification:
- Fixes a bug in the current code
- Lays the foundation of another fix
- Only affects altp2m, which isn't a supported feature

NB two possible further improvements to put on the list for 4.13:
 - Replace send-interrupt-and-wait-for-each-one loop with
   hvmop_flush_tlb_all()'s more efficient
   send-all-the-interrupts-then-wait loop.
 - Modify hvmop_flush_tlb_all() to use domain_pause_except_self() to
   avoid code duplication
---
CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
CC: Jan Beulich <jbeulich@xxxxxxxx>
CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>
CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
CC: Tim Deegan <tim@xxxxxxx>
---
 xen/arch/x86/mm/p2m.c   | 10 ++++++++--
 xen/common/domain.c     |  8 +++++++-
 xen/include/xen/sched.h |  2 +-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d14ce57..7232dbf 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2530,8 +2530,11 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned 
int idx)
     if ( !idx || idx >= MAX_ALTP2M )
         return rc;
 
-    domain_pause_except_self(d);
+    rc = domain_pause_except_self(d);
+    if ( rc )
+        return rc;
 
+    rc = -EBUSY;
     altp2m_list_lock(d);
 
     if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
@@ -2561,8 +2564,11 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, 
unsigned int idx)
     if ( idx >= MAX_ALTP2M )
         return rc;
 
-    domain_pause_except_self(d);
+    rc = domain_pause_except_self(d);
+    if ( rc )
+        return rc;
 
+    rc = -EINVAL;
     altp2m_list_lock(d);
 
     if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 7470cd9..32bca8d 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1134,18 +1134,24 @@ int domain_unpause_by_systemcontroller(struct domain *d)
     return 0;
 }
 
-void domain_pause_except_self(struct domain *d)
+int domain_pause_except_self(struct domain *d)
 {
     struct vcpu *v, *curr = current;
 
     if ( curr->domain == d )
     {
+        /* Avoid racing with other vcpus which may want to be pausing us */
+        if ( !spin_trylock(&d->hypercall_deadlock_mutex) )
+            return -ERESTART;
         for_each_vcpu( d, v )
             if ( likely(v != curr) )
                 vcpu_pause(v);
+        spin_unlock(&d->hypercall_deadlock_mutex);
     }
     else
         domain_pause(d);
+
+    return 0;
 }
 
 void domain_unpause_except_self(struct domain *d)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index d633e1d..edee52d 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -839,7 +839,7 @@ static inline int 
domain_pause_by_systemcontroller_nosync(struct domain *d)
 }
 
 /* domain_pause() but safe against trying to pause current. */
-void domain_pause_except_self(struct domain *d);
+int __must_check domain_pause_except_self(struct domain *d);
 void domain_unpause_except_self(struct domain *d);
 
 /*
-- 
2.7.4


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