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

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@entel.upc.edu>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 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

Roger Pau Monne writes ("[PATCH v3] libxl: Atomicaly check backend state
and set it to 5 at device_remove"):
> 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.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Do you want me to push it onto the end of my series or will you
repost it after mine''s applied ?

Ian.