tools: xentoolcore_restrict_all: Do deregistration before close

Closing the fd before unhooking it from the list runs the risk that a
concurrent thread calls xentoolcore_restrict_all will operate on the
old fd value, which might refer to a new fd by then.  So we need to do
it in the other order.

Sadly this weakens the guarantee provided by xentoolcore_restrict_all
slightly, but not (I think) in a problematic way.  It would be
possible to implement the previous guarantee, but it would involve
replacing all of the close() calls in all of the individual osdep
parts of all of the individual libraries with calls to a new function
which does
   dup2("/dev/null", thing->fd);
   pthread_mutex_lock(&handles_lock);
   thing->fd = -1;
   pthread_mutex_unlock(&handles_lock);
   close(fd);
which would be terribly tedious.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Release-acked-by: Julien Grall <julien.grall@linaro.org>

diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
index b256fce98c815327d70e15dd33e7943f978abdff..f3a34009da1a004098afa25526f264c2a6c643f2 100644 (file)
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -59,8 +59,8 @@ xencall_handle *xencall_open(xentoollog_logger *logger, unsigned open_flags)
     return xcall;
 
 err:
-    osdep_xencall_close(xcall);
     xentoolcore__deregister_active_handle(&xcall->tc_ah);
+    osdep_xencall_close(xcall);
     xtl_logger_destroy(xcall->logger_tofree);
     free(xcall);
     return NULL;
@@ -73,8 +73,8 @@ int xencall_close(xencall_handle *xcall)
     if ( !xcall )
         return 0;
 
-    rc = osdep_xencall_close(xcall);
     xentoolcore__deregister_active_handle(&xcall->tc_ah);
+    rc = osdep_xencall_close(xcall);
     buffer_release_cache(xcall);
     xtl_logger_destroy(xcall->logger_tofree);
     free(xcall);

diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c
index b66d4f92948757a84b2b45e01b571b9c3c1e038b..355b7dec18a17ea3bc70980eed89f4736223656c 100644 (file)
--- a/tools/libs/devicemodel/core.c
+++ b/tools/libs/devicemodel/core.c
@@ -68,8 +68,8 @@ xendevicemodel_handle *xendevicemodel_open(xentoollog_logger *logger,
 
 err:
     xtl_logger_destroy(dmod->logger_tofree);
-    xencall_close(dmod->xcall);
     xentoolcore__deregister_active_handle(&dmod->tc_ah);
+    xencall_close(dmod->xcall);
     free(dmod);
     return NULL;
 }
@@ -83,8 +83,8 @@ int xendevicemodel_close(xendevicemodel_handle *dmod)
 
     rc = osdep_xendevicemodel_close(dmod);
 
-    xencall_close(dmod->xcall);
     xentoolcore__deregister_active_handle(&dmod->tc_ah);
+    xencall_close(dmod->xcall);
     xtl_logger_destroy(dmod->logger_tofree);
     free(dmod);
     return rc;

diff --git a/tools/libs/evtchn/core.c b/tools/libs/evtchn/core.c
index 2dba58bf0044edf1f5eb680085b80756b3eb29be..aff6ecfaa0a223e5fd2bb92bb3ed5e36dd00c933 100644 (file)
--- a/tools/libs/evtchn/core.c
+++ b/tools/libs/evtchn/core.c
@@ -55,8 +55,8 @@ xenevtchn_handle *xenevtchn_open(xentoollog_logger *logger, unsigned open_flags)
     return xce;
 
 err:
-    osdep_evtchn_close(xce);
     xentoolcore__deregister_active_handle(&xce->tc_ah);
+    osdep_evtchn_close(xce);
     xtl_logger_destroy(xce->logger_tofree);
     free(xce);
     return NULL;
@@ -69,8 +69,8 @@ int xenevtchn_close(xenevtchn_handle *xce)
     if ( !xce )
         return 0;
 
-    rc = osdep_evtchn_close(xce);
     xentoolcore__deregister_active_handle(&xce->tc_ah);
+    rc = osdep_evtchn_close(xce);
     xtl_logger_destroy(xce->logger_tofree);
     free(xce);
     return rc;

diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 79b24d273b738f9d66eeac4ec0172956f761d299..7c8562ae74776930218c6046c8c732cd7a4f4e4a 100644 (file)
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -57,8 +57,8 @@ xenforeignmemory_handle *xenforeignmemory_open(xentoollog_logger *logger,
     return fmem;
 
 err:
-    osdep_xenforeignmemory_close(fmem);
     xentoolcore__deregister_active_handle(&fmem->tc_ah);
+    osdep_xenforeignmemory_close(fmem);
     xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return NULL;
@@ -71,8 +71,8 @@ int xenforeignmemory_close(xenforeignmemory_handle *fmem)
     if ( !fmem )
         return 0;
 
-    rc = osdep_xenforeignmemory_close(fmem);
     xentoolcore__deregister_active_handle(&fmem->tc_ah);
+    rc = osdep_xenforeignmemory_close(fmem);
     xtl_logger_destroy(fmem->logger_tofree);
     free(fmem);
     return rc;

diff --git a/tools/libs/gnttab/gnttab_core.c b/tools/libs/gnttab/gnttab_core.c
index 5f761e54693c66d317ea480a5e8faf6e2243f2ab..98f1591e9da365da0f9b63f8c5d696f2cdb47357 100644 (file)
--- a/tools/libs/gnttab/gnttab_core.c
+++ b/tools/libs/gnttab/gnttab_core.c
@@ -54,8 +54,8 @@ xengnttab_handle *xengnttab_open(xentoollog_logger *logger, unsigned open_flags)
     return xgt;
 
 err:
-    osdep_gnttab_close(xgt);
     xentoolcore__deregister_active_handle(&xgt->tc_ah);
+    osdep_gnttab_close(xgt);
     xtl_logger_destroy(xgt->logger_tofree);
     free(xgt);
     return NULL;
@@ -68,8 +68,8 @@ int xengnttab_close(xengnttab_handle *xgt)
     if ( !xgt )
         return 0;
 
-    rc = osdep_gnttab_close(xgt);
     xentoolcore__deregister_active_handle(&xgt->tc_ah);
+    rc = osdep_gnttab_close(xgt);
     xtl_logger_destroy(xgt->logger_tofree);
     free(xgt);
     return rc;

diff --git a/tools/libs/toolcore/include/xentoolcore.h b/tools/libs/toolcore/include/xentoolcore.h
index 8d28c2d797223108b3af48e3223dcbdea5ce9fe8..b3a3c934e28314a9f0d2856376916a0d5eb8c00e 100644 (file)
--- a/tools/libs/toolcore/include/xentoolcore.h
+++ b/tools/libs/toolcore/include/xentoolcore.h
@@ -39,6 +39,15 @@
  * fail (even though such a call is potentially meaningful).
  * (If called again with a different domid, it will necessarily fail.)
  *
+ * Note for multi-threaded programs: If xentoolcore_restrict_all is
+ * called concurrently with a function which /or closes Xen library
+ * handles (e.g.  libxl_ctx_free, xs_close), the restriction is only
+ * guaranteed to be effective after all of the closing functions have
+ * returned, even if that is later than the return from
+ * xentoolcore_restrict_all.  (Of course if xentoolcore_restrict_all
+ * it is called concurrently with opening functions, the new handles
+ * might or might not be restricted.)
+ *
  *  ====================================================================
  *  IMPORTANT - IMPLEMENTATION STATUS
  *

diff --git a/tools/libs/toolcore/include/xentoolcore_internal.h b/tools/libs/toolcore/include/xentoolcore_internal.h
index dbdb1dd28721dc8644cfa0422d9039921770a61d..04f5848f097849f5662d7a0172a91db739d79f58 100644 (file)
--- a/tools/libs/toolcore/include/xentoolcore_internal.h
+++ b/tools/libs/toolcore/include/xentoolcore_internal.h
@@ -48,8 +48,10 @@
  *     4. ONLY THEN actually open the relevant fd or whatever
  *
  *   III. during the "close handle" function
- *     1. FIRST close the relevant fd or whatever
- *     2. call xentoolcore__deregister_active_handle
+ *     1. FIRST call xentoolcore__deregister_active_handle
+ *     2. close the relevant fd or whatever
+ *
+ * [ III(b). Do the same as III for error exit from the open function. ]
  *
  *   IV. in the restrict_callback function
  *     * Arrange that the fd (or other handle) can no longer by used

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index 23f3f09c8c9aebbd63d2ae9af5e90a190573cef6..abffd9cd808dddadf27be42b6cda1847822d6849 100644 (file)
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -279,9 +279,9 @@ err:
        saved_errno = errno;
 
        if (h) {
+               xentoolcore__deregister_active_handle(&h->tc_ah);
                if (h->fd >= 0)
                        close(h->fd);
-               xentoolcore__deregister_active_handle(&h->tc_ah);
        }
        free(h);
 
@@ -342,8 +342,8 @@ static void close_fds_free(struct xs_handle *h) {
                close(h->watch_pipe[1]);
        }
 
-        close(h->fd);
        xentoolcore__deregister_active_handle(&h->tc_ah);
+        close(h->fd);
         
        free(h);
 }