libxl: properly lock userdata store

Originally libxl user data store didn't have lock at all. There could be
such race condition as mentioned by Ian Jackson:

  Task 1                                 Task 2
  Creating the domain                    Trying to shut down

    actually create domain
                                           observe domid
                                           start domain destruction
                                           delete all userdata
                                           destroy domain
    store the userdata
      *** forbidden state created: userdata exists but domain doesn't
      *** userdata has been leaked
    [ would now bomb out ]

This patch adds in proper locking to libxl user data store. The lock is
associated with a specific domain (i.e. a per-domain lock).

As for locking hierachy, we first take CTX lock (which is implemented
with pthread recursive mutex so even if the application has taken it
we're fine), then take the file lock. These locks are released in
reversed order.

A new libxl error code ERROR_LOCK_FAIL is introduced to describe failure
to acquire locks.

Also factor out libxl__userdata_{retrieve,store}, so that other
functions that already hold the lock can call them to manipulate
user data.

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 2ae5fcaa7aab9b4ad3e1c3db4baa013dd123e59c..9bb1a90bbe82ae52609a261b3c58016cad3bcffc 100644 (file)
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1530,6 +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;
 
     dom_path = libxl__xs_get_dompath(gc, domid);
     if (!dom_path) {
@@ -1555,6 +1556,12 @@ static void devices_destroy_cb(libxl__egc *egc,
     xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(gc,
                                 "/local/domain/%d/hvmloader", domid));
 
+    /* This is async operation, we already hold CTX lock */
+    lock = libxl__lock_domain_userdata(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
     libxl__userdata_destroyall(gc, domid);
 
     rc = xc_domain_destroy(ctx->xch, domid);
@@ -1566,6 +1573,7 @@ static void devices_destroy_cb(libxl__egc *egc,
     rc = 0;
 
 out:
+    if (lock) libxl__unlock_domain_userdata(lock);
     dis->callback(egc, dis, rc);
     return;
 }

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 5e0cb80f81f6beadec75c59701b5925ae1cdbd7f..0dfdb088d564dc9e5cb674934869e7ca79385154 100644 (file)
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1865,11 +1865,10 @@ out:
     return;
 }
 
-int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
-                              const char *userdata_userid,
-                              const uint8_t *data, int datalen)
+int libxl__userdata_store(libxl__gc *gc, uint32_t domid,
+                          const char *userdata_userid,
+                          const uint8_t *data, int datalen)
 {
-    GC_INIT(ctx);
     const char *filename;
     const char *newfilename;
     int e, rc;
@@ -1898,7 +1897,7 @@ int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
     if (fd < 0)
         goto err;
 
-    if (libxl_write_exactly(ctx, fd, data, datalen, "userdata", newfilename))
+    if (libxl_write_exactly(CTX, fd, data, datalen, "userdata", newfilename))
         goto err;
 
     if (close(fd) < 0) {
@@ -1920,18 +1919,42 @@ err:
     }
 
     if (rc)
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "cannot write/rename %s for %s",
+        LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR, "cannot write/rename %s for %s",
                  newfilename, filename);
 out:
-    GC_FREE;
     return rc;
 }
 
-int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
-                                 const char *userdata_userid,
-                                 uint8_t **data_r, int *datalen_r)
+int libxl_userdata_store(libxl_ctx *ctx, uint32_t domid,
+                              const char *userdata_userid,
+                              const uint8_t *data, int datalen)
 {
     GC_INIT(ctx);
+    int rc;
+    libxl__carefd *lock;
+
+    CTX_LOCK;
+    lock = libxl__lock_domain_userdata(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
+    rc = libxl__userdata_store(gc, domid, userdata_userid,
+                               data, datalen);
+
+    libxl__unlock_domain_userdata(lock);
+
+out:
+    CTX_UNLOCK;
+    GC_FREE;
+    return rc;
+}
+
+int libxl__userdata_retrieve(libxl__gc *gc, uint32_t domid,
+                             const char *userdata_userid,
+                             uint8_t **data_r, int *datalen_r)
+{
     const char *filename;
     int e, rc;
     int datalen = 0;
@@ -1943,13 +1966,13 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
         goto out;
     }
 
-    e = libxl_read_file_contents(ctx, filename, data_r ? &data : 0, &datalen);
+    e = libxl_read_file_contents(CTX, filename, data_r ? &data : 0, &datalen);
     if (e && errno != ENOENT) {
         rc = ERROR_FAIL;
         goto out;
     }
     if (!e && !datalen) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "userdata file %s is empty", filename);
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "userdata file %s is empty", filename);
         if (data_r) assert(!*data_r);
         rc = ERROR_FAIL;
         goto out;
@@ -1958,7 +1981,33 @@ int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
     if (data_r) *data_r = data;
     if (datalen_r) *datalen_r = datalen;
     rc = 0;
+
+out:
+    return rc;
+}
+
+int libxl_userdata_retrieve(libxl_ctx *ctx, uint32_t domid,
+                                 const char *userdata_userid,
+                                 uint8_t **data_r, int *datalen_r)
+{
+    GC_INIT(ctx);
+    int rc;
+    libxl__carefd *lock;
+
+    CTX_LOCK;
+    lock = libxl__lock_domain_userdata(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
+    rc = libxl__userdata_retrieve(gc, domid, userdata_userid,
+                                  data_r, datalen_r);
+
+
+    libxl__unlock_domain_userdata(lock);
 out:
+    CTX_UNLOCK;
     GC_FREE;
     return rc;
 }

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 0cedd3092cae581da548eac6b1d35331bcefd6e3..bc89e79e87f1968dac1a4ffb49edd93ebc13ae23 100644 (file)
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -997,6 +997,16 @@ _hidden const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid,
                                          const char *userdata_userid,
                                          const char *wh);
 _hidden void libxl__userdata_destroyall(libxl__gc *gc, uint32_t domid);
+/* Caller must hold userdata store lock before calling
+ * libxl__userdata_{retrieve,store}
+ * See libxl__{un,}lock_domain_userdata.
+ */
+_hidden int libxl__userdata_retrieve(libxl__gc *gc, uint32_t domid,
+                                     const char *userdata_userid,
+                                     uint8_t **data_r, int *datalen_r);
+_hidden int libxl__userdata_store(libxl__gc *gc, uint32_t domid,
+                                  const char *userdata_userid,
+                                  const uint8_t *data, int datalen);
 
 _hidden int libxl__domain_resume(libxl__gc *gc, uint32_t domid,
                                  int suspend_cancel);

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 931c9e96d0791dfc3fca571390ba266d3ff8f14e..95681d5c91981b790f40308904b648ce4c4c6b94 100644 (file)
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -58,6 +58,7 @@ libxl_error = Enumeration("error", [
     (-12, "OSEVENT_REG_FAIL"),
     (-13, "BUFFERFULL"),
     (-14, "UNKNOWN_CHILD"),
+    (-15, "LOCK_FAIL"),
     ], value_namespace = "")
 
 libxl_domain_type = Enumeration("domain_type", [