Re: [Xen-devel] [PATCH] tools: xentoolcore_restrict_all: Do deregistration before close

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On 11/14/2017 12:15 PM, Ian Jackson wrote:
> 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
> slight, 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.
...
> diff --git a/tools/libs/toolcore/include/xentoolcore.h 
> b/tools/libs/toolcore/include/xentoolcore.h
> index 8d28c2d..b3a3c93 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
"which /or closes..." - Is this a typo?

> + * 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 dbdb1dd..04f5848 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 23f3f09..abffd9c 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);
>           
Since the rest of this file uses tabs, you may as well use tabs for this 
line as well.
Reviewed-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>

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