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@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ad3495a8047325e7cc1f5c983d072358fc56bd01..3b2d104f035df31937e29305d378bf3898d5348a 100644 (file)
--- 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 a5e185eac64cdd8ffe3799d494640c08ec3e3a7f..8b825849d171b7a00d14f0fe11c001b6a463ec35 100644 (file)
--- 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 0dfdb088d564dc9e5cb674934869e7ca79385154..02384acdf74cb0531b1297dbedddae08149ed893 100644 (file)
--- 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 e9747f1e52126ff15f8a7664c0d418888d9d8bcb..02a71cb163cfc1b53bd54966d0fa9792dee12b51 100644 (file)
--- 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 03e9978553f610c9cb3cb83506127f5d22b28cab..aa18e74711f650d5a3cf238771af540d929573d3 100644 (file)
--- 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