[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.cindex 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |