From: George Dunlap Date: Mon, 18 Feb 2019 12:45:24 +0000 (+0100) Subject: altp2m: Prevent deadlocks when a domain performs altp2m operations on itself X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=29d28b29190ba09d53ae7e475108def84e16e363;p=people%2Fpauldu%2Fxen.git altp2m: Prevent deadlocks when a domain performs altp2m operations on itself 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 Tested-by: Razvan Cojocaru Acked-by: Jan Beulich Release-acked-by: Juergen Gross --- diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index d14ce57dd5..7232dbf71e 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 7470cd916a..32bca8dbf2 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 d633e1da70..edee52dfe4 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); /*