From a788fe0cd8bcb51ef167fc451f837066362d8c82 Mon Sep 17 00:00:00 2001 From: Wei Liu Date: Tue, 16 Sep 2014 11:01:11 +0100 Subject: [PATCH] libxl: rework domain userdata file lock 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 Acked-by: Ian Campbell --- 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 ad3495a804..3b2d104f03 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 a5e185eac6..8b825849d1 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 0dfdb088d5..02384acdf7 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 03e9978553..aa18e74711 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 -- 2.39.5