[Xen-devel] [PATCH v3] libxl: Atomicaly check backend state and set it to 5 at device_remove

[Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>]

libxl__initiate_device_remove was setting backend state to 5, which
could create a race condition, since the previous check for state != 4
and setting state to 5 was not done inside the same transaction, so
the kernel could change the state to 6 in the space between the check
for state != 4 and setting it to 5.

The state != 4 check and setting it to 5 should happen in the same
transaction, to assure that nobody is modifying it behind our back.

Changes since v2:

 * Updated to match current tree, plus Ian Jackson "libxl child
   process handling" series.

 * Moved xs_transaction_end to out_ok section.

Changes since v1:

 * Do the check and set in the same transaction, instead of removing
   the set state to 5.

Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
---
 tools/libxl/libxl_device.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index c7e057d..d5cb722 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -381,23 +381,25 @@ int libxl__initiate_device_remove(libxl__egc *egc, 
libxl__ao *ao,
 {
     AO_GC;
     libxl_ctx *ctx = libxl__gc_owner(gc);
-    xs_transaction_t t;
+    xs_transaction_t t = 0;
     char *be_path = libxl__device_backend_path(gc, dev);
     char *state_path = libxl__sprintf(gc, "%s/state", be_path);
-    char *state = libxl__xs_read(gc, XBT_NULL, state_path);
+    char *state;
     int rc = 0;
     libxl__ao_device_remove *aorm = 0;
 
-    if (!state)
+retry_transaction:
+    t = xs_transaction_start(ctx->xsh);
+    state = libxl__xs_read(gc, t, state_path);
+    if (!state) {
         goto out_ok;
+    }
     if (atoi(state) != 4) {
         libxl__device_destroy_tapdisk(gc, be_path);
         xs_rm(ctx->xsh, XBT_NULL, be_path);
         goto out_ok;
     }
 
-retry_transaction:
-    t = xs_transaction_start(ctx->xsh);
     xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/online", be_path), "0", 
strlen("0"));
     xs_write(ctx->xsh, t, state_path, "5", strlen("5"));
     if (!xs_transaction_end(ctx->xsh, t, 0)) {
@@ -408,6 +410,8 @@ retry_transaction:
             goto out_fail;
         }
     }
+    /* mark transaction as ended, to prevent double closing it on out_ok */
+    t = 0;
 
     libxl__device_destroy_tapdisk(gc, be_path);
 
@@ -428,6 +432,7 @@ retry_transaction:
     return rc;
 
  out_ok:
+    if (t) xs_transaction_end(ctx->xsh, t, 0);
     libxl__ao_complete(egc, ao, 0);
     return 0;
 }
-- 
1.7.2.5


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