|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] [PATCH v4 for 4.5 1/8] 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 <wei.liu2@xxxxxxxxxx>
---
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; i<gl.gl_pathc; i++) {
- userdata_delete(gc, gl.gl_pathv[i]);
+ if (!strstr(gl.gl_pathv[i], "domain-userdata-lock"))
+ userdata_delete(gc, gl.gl_pathv[i]);
}
globfree(&gl);
out:
@@ -1931,7 +1935,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
{
GC_INIT(ctx);
int rc;
- libxl__carefd *lock;
+ libxl__domain_userdata_lock *lock;
CTX_LOCK;
lock = libxl__lock_domain_userdata(gc, domid);
@@ -1992,7 +1996,7 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t
domid,
{
GC_INIT(ctx);
int rc;
- libxl__carefd *lock;
+ libxl__domain_userdata_lock *lock;
CTX_LOCK;
lock = libxl__lock_domain_userdata(gc, domid);
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index e9747f1..02a71cb 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -384,9 +384,10 @@ out:
/* Portability note: this lock utilises flock(2) so a proper implementation of
* flock(2) is required.
*/
-libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc, uint32_t domid)
+libxl__domain_userdata_lock *libxl__lock_domain_userdata(libxl__gc *gc,
+ uint32_t domid)
{
- libxl__carefd *carefd = NULL;
+ libxl__domain_userdata_lock *lock = NULL;
const char *lockfile;
int fd;
struct stat stab, fstab;
@@ -394,12 +395,15 @@ libxl__carefd *libxl__lock_domain_userdata(libxl__gc *gc,
uint32_t domid)
lockfile = libxl__userdata_path(gc, domid, "domain-userdata-lock", "l");
if (!lockfile) goto out;
+ lock = libxl__zalloc(NOGC, sizeof(libxl__domain_userdata_lock));
+ lock->path = 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |