From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: [PATCH v4 for 4.5 1/8] libxl: rework domain userdata file lock Date: Tue, 16 Sep 2014 11:01:11 +0100 Message-ID: <1410861678-31428-2-git-send-email-wei.liu2@citrix.com> References: <1410861678-31428-1-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1410861678-31428-1-git-send-email-wei.liu2@citrix.com> 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 Cc: Wei Liu , ian.jackson@eu.citrix.com, ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org The lock introduced in d2cd9d4f ("libxl: functions to lock / unlock libxl userdata store") has a bug that can leak the lock file when domain destruction races with other functions that try to get hold of the lock. There are several issues: 1. The lock is released too early with libxl__userdata_destroyall deletes everything in userdata store, including the lock file. 2. The check of domain existence is only done at the beginning of lock function, by the time the lock is acquired, the domain might have been gone already. The effect of this two issues is we can run into such situation: Process 1 Process 2 domain destruction # LOCK FUNCTION # LOCK FUNCTION check domain existence check domain existence acquire lock (file created) # LOCK FUNCTION destroy all files (lock file deleted, lock released) acquire lock (file created) # LOCK FUNCTION destroy domain # UNLOCK (close fd only) [ lock file leaked ] Fix this problem by deploying following changes: 1. Unlink lock file in unlock function. 2. Modify libxl__userdata_destroyall to not delete domain-userdata-lock, so that the lock remains held until unlock function is called. 3. Check domain still exists when the lock is acquired, unlock if domain is already gone. Signed-off-by: Wei Liu --- tools/libxl/libxl.c | 2 +- tools/libxl/libxl_create.c | 2 +- tools/libxl/libxl_dom.c | 10 +++++++--- tools/libxl/libxl_internal.c | 30 +++++++++++++++++++++--------- tools/libxl/libxl_internal.h | 9 +++++++-- 5 files changed, 37 insertions(+), 16 deletions(-) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index ad3495a..3b2d104 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -1530,7 +1530,7 @@ static void devices_destroy_cb(libxl__egc *egc, uint32_t domid = dis->domid; char *dom_path; char *vm_path; - libxl__carefd *lock = NULL; + libxl__domain_userdata_lock *lock = NULL; dom_path = libxl__xs_get_dompath(gc, domid); if (!dom_path) { diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index a5e185e..8b82584 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -1404,7 +1404,7 @@ static void domcreate_complete(libxl__egc *egc, rc = xc_flask_relabel_domain(CTX->xch, dcs->guest_domid, d_config->b_info.exec_ssidref); if (!rc) { - libxl__carefd *lock; + libxl__domain_userdata_lock *lock; /* Note that we hold CTX lock at this point so only need to * take data store lock diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index 0dfdb08..02384ac 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -1857,8 +1857,12 @@ void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid) if (r) LOGE(ERROR, "glob failed for %s", pattern); + /* Note: don't delete domain-userdata-lock, it will be handled by + * unlock function. + */ for (i=0; ipath = libxl__strdup(NOGC, lockfile); + while (true) { libxl__carefd_begin(); fd = open(lockfile, O_RDWR|O_CREAT, 0666); if (fd < 0) LOGE(ERROR, "cannot open lockfile %s, errno=%d", lockfile, errno); - carefd = libxl__carefd_opened(CTX, fd); + lock->lock_carefd = libxl__carefd_opened(CTX, fd); if (fd < 0) goto out; /* Lock the file in exclusive mode, wait indefinitely to @@ -434,20 +438,28 @@ libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid) break; } - libxl__carefd_close(carefd); + libxl__carefd_close(lock->lock_carefd); } - return carefd; + /* Check the domain is still there, if not we should release the + * lock and clean up. + */ + if (libxl_domain_info(CTX, NULL, domid)) + goto out; + + return lock; out: - if (carefd) libxl__carefd_close(carefd); + if (lock) libxl__unlock_domain_userdata(lock); return NULL; } -void libxl__unlock_domain_userdata(libxl__carefd *lock_carefd) +void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock) { - /* Simply closing the file descriptor releases the lock */ - libxl__carefd_close(lock_carefd); + if (lock->path) unlink(lock->path); + if (lock->lock_carefd) libxl__carefd_close(lock->lock_carefd); + free(lock->path); + free(lock); } int libxl__get_domain_configuration(libxl__gc *gc, uint32_t domid, diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 03e9978..aa18e74 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -3241,8 +3241,13 @@ static inline int libxl__key_value_list_is_empty(libxl_key_value_list *pkvl) int libxl__cpuid_policy_is_empty(libxl_cpuid_policy_list *pl); /* Portability note: a proper flock(2) implementation is required */ -libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid); -void libxl__unlock_domain_userdata(libxl__carefd *lock_carefd); +typedef struct { + libxl__carefd *lock_carefd; + char *path; /* path of the lock file itself */ +} libxl__domain_userdata_lock; +libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc, + uint32_t domid); +void libxl__unlock_domain_userdata(libxl__domain_userdata_lock *lock); /* * Retrieve / store domain configuration from / to libxl private -- 1.7.10.4