[Xen-changelog] [xen master] tools: libxl: Take the userdata lock around maxmem changes

[patchbot@xxxxxxx]

commit 5f33fa2bca6354fad1decfeda723c046311e85cc
Author:     Ian Campbell <ian.campbell@xxxxxxxxxx>
AuthorDate: Tue Jun 23 15:58:32 2015 +0100
Commit:     Ian Campbell <ian.campbell@xxxxxxxxxx>
CommitDate: Fri Jun 26 12:59:10 2015 +0100

    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@xxxxxxxxxx>
    Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
---
 tools/libxl/libxl.c          |   22 ++++++++++++++++++++++
 tools/libxl/libxl_internal.h |    1 +
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cfc2623..ed50d32 100644
--- 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 e96d6b5..5594b54 100644
--- 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);
--
generated by git-patchbot for /home/xen/git/xen.git#master

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog