[PATCH v3 03/15] libxl: properly lock userdata store

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

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>

---
changed in v3:
move the lock out of libxl__userdata_destroyall. The critical section
should be
  lock
  delete all userdata
  destroy domain
  unlcok
---
 tools/libxl/libxl.c          |    8 +++++
 tools/libxl/libxl_dom.c      |   73 +++++++++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.h |   10 ++++++
 tools/libxl/libxl_types.idl  |    1 +
 4 files changed, 80 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2ae5fca..9bb1a90 100644
--- 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 5e0cb80..0dfdb08 100644
--- 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 0cedd30..bc89e79 100644
--- 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 931c9e9..95681d5 100644
--- 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", [
-- 
1.7.10.4