[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Fri, 2012-03-16 at 16:26 +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 anywhere in libxl.  Until all locations in
> 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.

One of the xl callers of postfork does quickly exec. I presume it is
harmless to call the libxl noexec function anyway?

> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> ---
>  tools/libxl/Makefile         |    2 +-
>  tools/libxl/libxl.c          |    3 +
>  tools/libxl/libxl_event.h    |   12 ++++
>  tools/libxl/libxl_fork.c     |  146 
> ++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |   63 ++++++++++++++++++
>  tools/libxl/xl.c             |    3 +
>  6 files changed, 228 insertions(+), 1 deletions(-)
>  create mode 100644 tools/libxl/libxl_fork.c
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 80c36ad..b30d11f 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -53,7 +53,7 @@ LIBXL_LIBS += -lyajl
>  LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
>                         libxl_dom.o libxl_exec.o libxl_xshelp.o 
> libxl_device.o \
>                         libxl_internal.o libxl_utils.o libxl_uuid.o 
> libxl_json.o \
> -                       libxl_qmp.o libxl_event.o $(LIBXL_OBJS-y)
> +                       libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
>  LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
> 
>  $(LIBXL_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest) 
> $(CFLAGS_libxenstore) $(CFLAGS_libblktapctl) -include 
> $(XEN_ROOT)/tools/config.h
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 5827bd4..469f66a 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -68,6 +68,9 @@ int libxl_ctx_alloc(libxl_ctx **pctx, int version,
> 
>      /* Now ctx is safe for ctx_free; failures simply set rc and "goto out" */
> 
> +    rc = libxl__atfork_init(ctx);
> +    if (rc) goto out;
> +
>      rc = libxl__poller_init(ctx, &ctx->poller_app);
>      if (rc) goto out;
> 
> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
> index ea553f6..41aebc0 100644
> --- a/tools/libxl/libxl_event.h
> +++ b/tools/libxl/libxl_event.h
> @@ -371,6 +371,18 @@ 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

One too many underscores after libxl here.

> + * on any existing (or specially made) ctx is sufficient; after
> + * this all previously existing libxl_ctx's are invalidated and
> + * must not be used - or even freed.
> + */
> +void libxl_postfork_child_noexec(libxl_ctx *ctx);
> +
> +
>  #endif
> 
>  /*
> diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
> new file mode 100644
> index 0000000..4aaa0b5
> --- /dev/null
> +++ b/tools/libxl/libxl_fork.c
> @@ -0,0 +1,146 @@
> +/*
> + * 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;
> +static int atfork_registered;
> +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) { rc = 0; goto out; }
> +
> +    r = pthread_atfork(atfork_lock, atfork_unlock, atfork_unlock);
> +    if (r) {
> +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "pthread_atfork failed");
> +        rc = ERROR_NOMEM;

I thought we were calling the magic log+exit function on these now ;-)

Surprising that ENOMEM is the only error that can be returned here
(you'd think all manner of things could go wrong when threads are
involved). No matter.

> +        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(); }
> +
> +libxl__carefd *libxl__carefd_record(libxl_ctx *ctx, int fd)
> +{
> +    libxl__carefd *cf = 0;
> +
> +    libxl_fd_set_cloexec(ctx, fd, 1);
> +    cf = libxl__zalloc(NULL, sizeof(*cf));
> +    cf->fd = fd;
> +    LIBXL_LIST_INSERT_HEAD(&carefds, cf, entry);
> +    return cf;
> +}
> +
> +libxl__carefd *libxl__carefd_opened(libxl_ctx *ctx, int fd)
> +{
> +    libxl__carefd *cf = 0;
> +
> +    cf = libxl__carefd_record(ctx, fd);
> +    libxl__carefd_unlock();
> +    return cf;
> +}
> +
> +void libxl_postfork_child_noexec(libxl_ctx *ctx)
> +{
> +    libxl__carefd *cf, *cf_tmp;
> +    int r;
> +
> +    atfork_lock();
> +    LIBXL_LIST_FOREACH_SAFE(cf, &carefds, entry, cf_tmp) {
> +        if (cf->fd >= 0) {

Where can an fd < 0 come from? Shouldn't we reject those at
libxl__carefd_record? Or does it make error handling easier in the
callers somehow?

> +            r = close(cf->fd);
> +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_WARNING, "failed to close fd=%d"
> +                             " (perhaps of another libxl ctx)", cf->fd);

Shouldn't there be an if (r < 0) before this logging?

> +        }
> +        free(cf);
> +    }
> +    LIBXL_LIST_INIT(&carefds);
> +    atfork_unlock();
> +}
> +
> +int libxl__carefd_close(libxl__carefd *cf)
> +{
> +    if (!cf) return 0;
> +    int r = cf->fd < 0 ? 0 : close(cf->fd);
> +    int esave = errno;
> +    LIBXL_LIST_REMOVE(cf, entry);

Are all the LIBXL_LIST_foo thread safe?

This function isn't called with the fork lock held.

> +    free(cf);
> +    errno = esave;
> +    return r;
> +}
> +
> +int libxl__carefd_fd(const libxl__carefd *cf)
> +{
> +    if (!cf) return -1;
> +    return cf->fd;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 465d564..8091ca2 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -606,6 +606,9 @@ void libxl__poller_put(libxl_ctx *ctx, libxl__poller *p);
>  void libxl__poller_wakeup(libxl__egc *egc, libxl__poller *p);
> 
> 
> +int libxl__atfork_init(libxl_ctx *ctx);
> +
> +
>  /* from xl_dom */
>  _hidden libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid);
>  _hidden int libxl__domain_shutdown_reason(libxl__gc *gc, uint32_t domid);
> @@ -1302,6 +1305,66 @@ _hidden void libxl__ao_complete(libxl__egc *egc, 
> libxl__ao *ao, int rc);
>  /* For use by ao machinery ONLY */
>  _hidden void libxl__ao__destroy(libxl_ctx*, libxl__ao *ao);
> 
> +
> +/*
> + * File descriptors and CLOEXEC
> + */
> +
> +/*
> + * For libxl functions which create file descriptors, at least one
> + * of the following must be true:
> + *  (a) libxl does not care if copies of this open-file are inherited
> + *      by random children and might remain open indefinitely
> + *  (b) libxl must take extra care for the fd (the actual descriptor,
> + *      not the open-file) as below.  We call this a "carefd".
> + *
> + * The rules for opening a carefd are:
> + *  (i)   Before bringing any carefds into existence,
> + *        libxl code must call libxl__carefd_begin.
> + *  (ii)  Then for each carefd brought into existence,
> + *        libxl code must call libxl__carefd_record
> + *        and remember the libxl__carefd_record*.
> + *  (iii) Then it must call libxl__carefd_unlock.
> + *  (iv)  When in a child process the fd is to be passed across
> + *        exec by libxl, the libxl code must unset FD_CLOEXEC
> + *        on the fd eg by using libxl_fd_set_cloexec.
> + *  (v)   Later, when the fd is to be closed in the same process,
> + *        libxl code must not call close.  Instead, it must call
> + *        libxl__carefd_close.
> + * Steps (ii) and (iii) can be combined by calling the convenience
> + * function libxl__carefd_opened.
> + */
> +/* libxl__carefd_begin and _unlock (or _opened) must be called always
> + * in pairs.  They may be called with the CTX lock held.  In between
> + * _begin and _unlock, the following are prohibited:
> + *   - anything which might block
> + *   - any callbacks to the application
> + *   - nested calls to libxl__carefd_begin
> + *   - fork (libxl__fork)
> + * In general nothing should be done before _unlock that could be done
> + * afterwards.
> + */
> +typedef struct libxl__carefd libxl__carefd;
> +
> +void libxl__carefd_begin(void);
> +void libxl__carefd_unlock(void);
> +
> +/* fd may be -1, in which case this returns a dummy libxl__fd_record
> + * on which it _carefd_close is a no-op.  Cannot fail. */

answers my earlier question about fd==-1.

> +libxl__carefd *libxl__carefd_record(libxl_ctx *ctx, int fd);
> +
> +/* Combines _record and _unlock in a single call.  If fd==-1,
> + * still does the unlock, but returns 0.  Cannot fail. */
> +libxl__carefd *libxl__carefd_opened(libxl_ctx *ctx, int fd);
> +
> +/* Works just like close(2).  You may pass NULL, in which case it's
> + * a successful no-op. */
> +int libxl__carefd_close(libxl__carefd*);
> +
> +/* You may pass NULL in which case the answer is -1. */
> +int libxl__carefd_fd(const libxl__carefd*);
> +
> +
>  /*
>   * Convenience macros.
>   */
> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index 9fd67b4..d806077 100644
> --- a/tools/libxl/xl.c
> +++ b/tools/libxl/xl.c
> @@ -97,6 +97,9 @@ static void parse_global_config(const char *configfile,
> 
>  void postfork(void)
>  {
> +    libxl_postfork_child_noexec(ctx); /* in case we don't exit/exec */
> +    ctx = 0;
> +
>      if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, (xentoollog_logger*)logger)) 
> {
>          fprintf(stderr, "cannot reinit xl context after fork\n");
>          exit(-1);
> --
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.