From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kouya Shimura Subject: [PATCH] x86/hap: fix race condition between ENABLE_LOGDIRTY and track_dirty_vram hypercall Date: Thu, 29 Nov 2012 16:02:21 +0900 Message-ID: <50B7087D.20407@jp.fujitsu.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------010100060105050609020200" Return-path: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --------------010100060105050609020200 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit There is a race condition between XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY and HVMOP_track_dirty_vram hypercall. Although HVMOP_track_dirty_vram is called many times from qemu-dm which is connected via VNC, XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY is called only once from a migration process (e.g. xc_save, libxl-save-helper). So the race seldom happens, but the following cases are possible. ======================================================================== [case-1] XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY hypercall -> paging_enable_logdirty() -> hap_logdirty_init() -> paging_log_dirty_disable() dirty_vram = NULL -> paging_log_dirty_init(d, hap_enable_log_dirty, ...) ---> (A) -> paging_log_dirty_enable() ************************************************************************** /* <--- (B) */ -> hap_enable_vram_tracking() // should be hap_enable_log_dirty() !!! return -EINVAL <- return -EINVAL // live-migration failure!!! ************************************************************************** HVMOP_track_dirty_vram -> hap_track_dirty_vram() /* (!paging_mode_log_dirty(d) && !dirty_vram) */ -> hap_vram_tracking_init() /* <--- (A) */ -> paging_log_dirty_init(d, hap_enable_vram_tracking, ...) ---> (B) -> paging_log_dirty_enable() ======================================================================== [case-2] XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY hypercall -> paging_enable_logdirty() -> hap_logdirty_init() -> paging_log_dirty_disable() /* d->arch.hvm_domain.dirty_vram != NULL */ d->arch.paging.mode &= ~PG_log_dirty; ---> (C) /* d->arch.hvm_domain.dirty_vram is freed */ dereference dirty_vram // access to freed memory !!! <--- (D) HVMOP_track_dirty_vram -> hap_track_dirty_vram() /* (!paging_mode_log_dirty(d) && dirty_vram) */ <---(C) rc = -EINVAL xfree(dirty_vram); ---> (D) dirty_vram = d->arch.hvm_domain.dirty_vram = NULL; return rc; ======================================================================== Actually I encountered the xen crash by null pointer exception in xen-3.4. FYI, in xen-4.x, c/s 19738:8dd5c3cae086 mitigated it. I'm not sure why paging_lock() is used partially in hap_XXX_vram_tracking functions. Thus, this patch introduces a new lock. It would be better to use paging_lock() instead of the new lock since shadow paging mode (not HAP mode) uses paging_lock to avoid this race condition. Thanks, Kouya Signed-off-by: Kouya Shimura --------------010100060105050609020200 Content-Type: text/x-patch; name="hap_dirty_vram.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="hap_dirty_vram.patch" diff -r d1d05cb59a76 xen/arch/x86/hvm/hvm.c --- a/xen/arch/x86/hvm/hvm.c Wed Nov 14 16:27:58 2012 +0000 +++ b/xen/arch/x86/hvm/hvm.c Wed Nov 28 16:04:19 2012 +0900 @@ -513,6 +513,7 @@ int hvm_domain_initialise(struct domain spin_lock_init(&d->arch.hvm_domain.pbuf_lock); spin_lock_init(&d->arch.hvm_domain.irq_lock); spin_lock_init(&d->arch.hvm_domain.uc_lock); + spin_lock_init(&d->arch.hvm_domain.dirty_vram_lock); INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list); spin_lock_init(&d->arch.hvm_domain.msixtbl_list_lock); diff -r d1d05cb59a76 xen/arch/x86/mm/hap/hap.c --- a/xen/arch/x86/mm/hap/hap.c Wed Nov 14 16:27:58 2012 +0000 +++ b/xen/arch/x86/mm/hap/hap.c Wed Nov 28 16:04:19 2012 +0900 @@ -122,7 +122,12 @@ int hap_track_dirty_vram(struct domain * XEN_GUEST_HANDLE_64(uint8) dirty_bitmap) { long rc = 0; - struct sh_dirty_vram *dirty_vram = d->arch.hvm_domain.dirty_vram; + struct sh_dirty_vram *dirty_vram; + + if ( !spin_trylock(&d->arch.hvm_domain.dirty_vram_lock) ) + return -ENODATA; + + dirty_vram = d->arch.hvm_domain.dirty_vram; if ( nr ) { @@ -174,6 +179,7 @@ int hap_track_dirty_vram(struct domain * rc = 0; } + spin_unlock(&d->arch.hvm_domain.dirty_vram_lock); return rc; param_fail: @@ -182,6 +188,8 @@ param_fail: xfree(dirty_vram); dirty_vram = d->arch.hvm_domain.dirty_vram = NULL; } + + spin_unlock(&d->arch.hvm_domain.dirty_vram_lock); return rc; } diff -r d1d05cb59a76 xen/arch/x86/mm/paging.c --- a/xen/arch/x86/mm/paging.c Wed Nov 14 16:27:58 2012 +0000 +++ b/xen/arch/x86/mm/paging.c Wed Nov 28 16:04:19 2012 +0900 @@ -697,14 +697,21 @@ int paging_domctl(struct domain *d, xen_ break; /* Else fall through... */ case XEN_DOMCTL_SHADOW_OP_ENABLE_LOGDIRTY: + spin_lock(&d->arch.hvm_domain.dirty_vram_lock); if ( hap_enabled(d) ) hap_logdirty_init(d); - return paging_log_dirty_enable(d); + rc = paging_log_dirty_enable(d); + spin_unlock(&d->arch.hvm_domain.dirty_vram_lock); + return rc; case XEN_DOMCTL_SHADOW_OP_OFF: + spin_lock(&d->arch.hvm_domain.dirty_vram_lock); + rc = 0; if ( paging_mode_log_dirty(d) ) - if ( (rc = paging_log_dirty_disable(d)) != 0 ) - return rc; + rc = paging_log_dirty_disable(d); + spin_unlock(&d->arch.hvm_domain.dirty_vram_lock); + if ( rc != 0 ) + return rc; break; case XEN_DOMCTL_SHADOW_OP_CLEAN: diff -r d1d05cb59a76 xen/include/asm-x86/hvm/domain.h --- a/xen/include/asm-x86/hvm/domain.h Wed Nov 14 16:27:58 2012 +0000 +++ b/xen/include/asm-x86/hvm/domain.h Wed Nov 28 16:04:19 2012 +0900 @@ -75,6 +75,7 @@ struct hvm_domain { /* VRAM dirty support. */ struct sh_dirty_vram *dirty_vram; + spinlock_t dirty_vram_lock; /* If one of vcpus of this domain is in no_fill_mode or * mtrr/pat between vcpus is not the same, set is_in_uc_mode --------------010100060105050609020200 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------010100060105050609020200--