From: Ian Jackson Date: Tue, 14 Nov 2017 12:15:42 +0000 (+0000) Subject: tools: xentoolcore_restrict_all: Do deregistration before close X-Git-Tag: archive/raspbian/4.11.1-1+rpi1~1^2~66^2~1000 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=9976f3874d4cca829f2d2916feab18615337bb5c;p=xen.git 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 Acked-by: Wei Liu Reviewed-by: Ross Lagerwall Release-acked-by: Julien Grall --- diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c index b256fce98c..f3a34009da 100644 --- 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 b66d4f9294..355b7dec18 100644 --- 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 2dba58bf00..aff6ecfaa0 100644 --- 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 79b24d273b..7c8562ae74 100644 --- 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 5f761e5469..98f1591e9d 100644 --- 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 8d28c2d797..b3a3c934e2 100644 --- 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 dbdb1dd287..04f5848f09 100644 --- 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 23f3f09c8c..abffd9cd80 100644 --- 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); }