[Xen-devel] [PATCH] tools: libxl: Take the userdata lock around maxmem changes

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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>
---
This applies on top of Wei's revert of "libxl_set_memory_target:
retain the same maxmem offset on top of the current target"

I couldn't quite rule out some race (for transaction=EAGAIN, for
!EAGAIN there are obvious ones) which resulted in the incorrect state
being in place after both entities exit, but couldn't construct an
actual case.
---
 tools/libxl/libxl.c          |   22 ++++++++++++++++++++++
 tools/libxl/libxl_internal.h |    1 +
 2 files changed, 23 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e3e63b5..0559e47 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;
 }
@@ -4745,6 +4756,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);
@@ -4866,6 +4886,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);
-- 
1.7.10.4


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