[PATCH v4 for 4.5 1/8] libxl: rework domain userdata file lock

[Wei Liu <wei.liu2@citrix.com>]

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>
---
 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