[Xen-devel] [PATCH v2 04/18] libxl: properly lock user data store

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxxx>
---
 tools/libxl/libxl_dom.c      |   81 +++++++++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.h |    9 +++++
 tools/libxl/libxl_types.idl  |    1 +
 3 files changed, 79 insertions(+), 12 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index a41e8fd..3b9eade 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1798,6 +1798,12 @@ void libxl__userdata_destroyall(libxl__gc *gc, uint32_t 
domid)
     const char *pattern;
     glob_t gl;
     int r, i;
+    libxl__carefd *lock;
+
+    CTX_LOCK;
+    lock = libxl__lock_domain_data(gc, domid);
+    if (!lock)
+        goto out;
 
     pattern = libxl__userdata_path(gc, domid, "*", "?");
     if (!pattern)
@@ -1817,14 +1823,15 @@ void libxl__userdata_destroyall(libxl__gc *gc, uint32_t 
domid)
     }
     globfree(&gl);
 out:
+    if (lock) libxl__unlock_domain_data(lock);
+    CTX_UNLOCK;
     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;
@@ -1853,7 +1860,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) {
@@ -1875,18 +1882,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_data(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
+
+    rc = libxl__userdata_store(gc, domid, userdata_userid,
+                               data, datalen);
+
+    libxl__unlock_domain_data(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;
@@ -1898,13 +1929,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;
@@ -1913,7 +1944,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_data(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_data(lock);
 out:
+    CTX_UNLOCK;
     GC_FREE;
     return rc;
 }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 60316ff..03ed7ec 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -996,6 +996,15 @@ _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 data store lock before calling
+ * libxl__userdata_{retrieve,store}
+ */
+_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 a412f9c..17b4453 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel