Re: [Xen-devel] [PATCH 4/6] libxl: Protect fds with CLOEXEC even with forking threads

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-02-17 at 19:15 +0000, Ian Jackson wrote:
> We introduce a new "carefd" concept, which relates to fds that we care
> about not being inherited by long-lived children.
> 
> As yet we do not use this anywherein libxl.  Until all locations in

"anywherein"

> libxl which make such fds are converted, libxl__postfork may not work
> entirely properly.  If these locations do not use O_CLOEXEC (or use
> calls for which there is no O_CLOEXEC) then multithreaded programs may
> not work properly.
> 
> This introduces a new API call libxl_postfork_child_noexec which must
> be called by applications which make long-running non-execing
> children.  Add the appropriate call to xl's postfork function.
> 
> On Linux pthread_atfork demands compilation with -pthread, so add
> PTHREAD_CFLAGS, _LDFLAGS, _LIBS to the corresponding variables in the
> libxl Makefile.  It is not clear why we got away without these before.

Since I assume the Makefile bit could safely be split out and applied,
that bit is:
Acked-by: Ian Campbell <Ian.Campbell@xxxxxxxxxx>

> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
> index f889115..74a0402 100644
> --- a/tools/libxl/libxl_event.h
> +++ b/tools/libxl/libxl_event.h
> @@ -369,6 +369,16 @@ void libxl_osevent_occurred_fd(libxl_ctx *ctx, void 
> *for_libxl,
>   */
>  void libxl_osevent_occurred_timeout(libxl_ctx *ctx, void *for_libxl);
> 
> +
> +/*
> + * An application which initialises a libxl_ctx in a parent process
> + * and then forks a child which does not quickly exec, must
> + * instead libxl__postfork_child_noexec in the child.  One call
> + * for all libxl contexts is sufficient.
> + */
> +void libxl_postfork_child_noexec(libxl_ctx *ctx);

Is the ctx passed here required to be one of the existing ctx's or a
newly allocated one in this context?

> +
> +
>  #endif
> 
>  /*
> diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
> new file mode 100644
> index 0000000..66d0ee0
> --- /dev/null
> +++ b/tools/libxl/libxl_fork.c
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (C) 2012      Citrix Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +/*
> + * Internal child process machinery for use by other parts of libxl
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +
> +#include "libxl_internal.h"
> +
> +/*
> + * carefd arrangements
> + *
> + * carefd_begin and _unlock take out the no_forking lock, which we
> + * also take and release in our pthread_atfork handlers.  So when this
> + * lock is held the whole process cannot fork.  We therefore protect
> + * our fds from leaking into children made by other threads.
> + *
> + * We maintain a list of all the carefds, so that if the application
> + * wants to fork a long-running but non-execing child, we can close
> + * them all.
> + *
> + * So the record function sets CLOEXEC for the benefit of execing
> + * children, and makes a note of the fd for the benefit of non-execing
> + * ones.
> + */
> +
> +struct libxl__carefd {
> +    LIBXL_LIST_ENTRY(libxl__carefd) entry;
> +    int fd;
> +};
> +
> +static pthread_mutex_t no_forking = PTHREAD_MUTEX_INITIALIZER;

We previously had trouble with PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
not being defined on NetBSD.

http://www.daemon-systems.org/man/pthread_mutex_init.3.html suggests
PTHREAD_MUTEX_INITIALIZER is but perhaps we should confirm?

> +static int atfork_registered;

Does pthread_atfork persist into the children?

If the parent has set this then it will be set for the child too, which
if the answer to my question is "No" is a problem.

I suspect the answer is yes though.

> +static LIBXL_LIST_HEAD(, libxl__carefd) carefds =
> +    LIBXL_LIST_HEAD_INITIALIZER(carefds);
> +
> +static void atfork_lock(void)
> +{
> +    int r = pthread_mutex_lock(&no_forking);
> +    assert(!r);
> +}
> +
> +static void atfork_unlock(void)
> +{
> +    int r = pthread_mutex_unlock(&no_forking);
> +    assert(!r);
> +}
> +
> +int libxl__atfork_init(libxl_ctx *ctx)
> +{
> +    int r, rc;
> +
> +    atfork_lock();
> +    if (atfork_registered)

Missing an unlock?

> +        return 0;
> +
> +    r = pthread_atfork(atfork_lock, atfork_unlock, atfork_unlock);
> +    if (r) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "pthread_atfork failed");
> +        rc = ERROR_NOMEM;
> +        goto out;
> +    }
> +
> +    atfork_registered = 1;
> +    rc = 0;
> + out:
> +    atfork_unlock();
> +    return rc;
> +}
> +
> +void libxl__carefd_begin(void) { atfork_lock(); }
> +void libxl__carefd_unlock(void) { atfork_unlock(); }

This naming seems asymmetric.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel