tools: libxl: Take the userdata lock around maxmem changes

There is an issue in libxl_set_memory_target whereby the target and
the max mem can get out of sync, this is because the call the
xc_domain_setmaxmem is not tied in any way to the xenstore transaction
which controls updates to the xenstore side of things.

Consider a domain with 1M of RAM (==target and maxmem for the sake of
argument) and two simultaneous calls to libxl_set_memory_target, both
with relative=0 and enforce=1, one with target=3 and the other with
target=5.

target=5 call                   target=3 call

transaction start
                                transaction start
write target=5 to xenstore
                                write target=3 to xenstore
setmaxmem(5)
                                setmaxmem(3)

transaction commit = success
                                transaction commit = EAGAIN

At this point maxmem=3 while target=5.

In reality the target=3 case will the retry and eventually (hopefully)
succeed with target=maxmem=3, however the bad state will persist for
some window which is undesirable. On failure other than EAGAIN all
bets are off anyway, but in that case we will likely stick in the bad
state until someone else sets the memory).

To fix this we slightly abuse the userdata lock which is used to
protect updates to the domain's json configuration. Abused because
maxmem is not actually stored in there, but is kept by Xen. However
the lock protects some semantically similar things and is convenient
to use here too.

libxl_domain_setmaxmem also takes the lock, since it reads
memory/target from xenstore before calling xc_domain_setmaxmem there
is a small (but perhaps not very interesting) race there too.

There is on more use of xc_domain_setmaxmem in libxl__build_pre.
However taking a lock around this would be tricky since the xenstore
parts are not done until libxl__build_post. I think this one could be
argued to be OK since the domid is not "public" yet, that is it has
not been returned to the application yet (as the result of the create
operation). Toolstacks which go round fiddling with random domid's
which they find lying on the floor should be taught to do better.

Add a doc note that taking the userdata lock requires the CTX_LOCK to
be held.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cfc2623273f4177e9d82748894b752905679659e..ed50d32572c7a2e33c888ac1404b7525e2e630b0 100644 (file)
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4621,6 +4621,15 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t max_memkb)
     uint32_t memorykb;
     char *dompath = libxl__xs_get_dompath(gc, domid);
     int rc = 1;
+    libxl__domain_userdata_lock *lock = NULL;
+
+    CTX_LOCK;
+
+    lock = libxl__lock_domain_userdata(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out;
+    }
 
     mem = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/memory/target", dompath));
     if (!mem) {
@@ -4647,6 +4656,8 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t max_memkb)
 
     rc = 0;
 out:
+    if (lock) libxl__unlock_domain_userdata(lock);
+    CTX_UNLOCK;
     GC_FREE;
     return rc;
 }
@@ -4746,6 +4757,15 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
     libxl_dominfo ptr;
     char *uuid;
     xs_transaction_t t;
+    libxl__domain_userdata_lock *lock;
+
+    CTX_LOCK;
+
+    lock = libxl__lock_domain_userdata(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL;
+        goto out_no_transaction;
+    }
 
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
@@ -4867,6 +4887,8 @@ out:
             goto retry_transaction;
 
 out_no_transaction:
+    if (lock) libxl__unlock_domain_userdata(lock);
+    CTX_UNLOCK;
     GC_FREE;
     return rc;
 }

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e96d6b57a522659aedce51e10382f8925e5a9aba..5594b54b6937451b61452a559d38fd6a53fee7d2 100644 (file)
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3600,6 +3600,7 @@ typedef struct {
     libxl__carefd *carefd;
     char *path; /* path of the lock file itself */
 } libxl__domain_userdata_lock;
+/* The CTX_LOCK must be held around uses of this 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);