From: Ian Jackson <ian.jackson@eu.citrix.com>
To: xen-devel@lists.xensource.com
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Subject: [PATCH 11/15] libxl: Protect fds with CLOEXEC even with forking threads
Date: Fri, 24 Feb 2012 18:54:59 +0000 [thread overview]
Message-ID: <1330109703-6536-12-git-send-email-ian.jackson@eu.citrix.com> (raw)
In-Reply-To: <1330109703-6536-1-git-send-email-ian.jackson@eu.citrix.com>
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.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
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 347f192..a041294 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -57,7 +57,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)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index fd890cf..716163a 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 f889115..f8bc22f 100644
--- a/tools/libxl/libxl_event.h
+++ b/tools/libxl/libxl_event.h
@@ -369,6 +369,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
+ * 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;
+ 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) {
+ r = close(cf->fd);
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_WARNING, "failed to close fd=%d"
+ " (perhaps of another libxl ctx)", cf->fd);
+ }
+ 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);
+ 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 b2f9092..3d0379e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -596,6 +596,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);
@@ -1279,6 +1282,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. */
+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
next prev parent reply other threads:[~2012-02-24 18:54 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-02-24 18:54 (no subject) Ian Jackson
2012-02-24 18:54 ` [PATCH 01/15] libxl: ao: allow immediate completion Ian Jackson
2012-02-24 18:54 ` [PATCH 02/15] libxl: fix hang due to libxl__initiate_device_remove Ian Jackson
2012-02-24 18:54 ` [PATCH 03/15] libxl: Fix eventloop_iteration over-locking Ian Jackson
2012-02-24 18:54 ` [PATCH 04/15] libxl: Fix leak of ctx->lock Ian Jackson
2012-02-24 18:54 ` [PATCH 05/15] libxl: abolish libxl_ctx_postfork Ian Jackson
2012-02-24 18:54 ` [PATCH 06/15] tools: Correct PTHREAD options in config/StdGNU.mk Ian Jackson
2012-02-24 18:54 ` [PATCH 07/15] libxl: Use PTHREAD_CFLAGS, LDFLAGS, LIBS Ian Jackson
2012-02-24 18:54 ` [PATCH 08/15] libxl: Crash (more sensibly) on malloc failure Ian Jackson
2012-02-24 18:54 ` [PATCH 09/15] libxl: Make libxl__zalloc et al tolerate a NULL gc Ian Jackson
2012-02-24 18:54 ` [PATCH 10/15] libxl: Introduce some convenience macros Ian Jackson
2012-02-24 18:54 ` Ian Jackson [this message]
2012-02-24 18:55 ` [PATCH 12/15] libxl: libxl_event.c:beforepoll_internal, REQUIRE_FDS Ian Jackson
2012-02-24 18:55 ` [PATCH 13/15] libxl: event API: new facilities for waiting for subprocesses Ian Jackson
2012-02-24 18:55 ` [PATCH 14/15] libxl: Provide libxl_string_list_length Ian Jackson
2012-02-24 18:55 ` [PATCH 15/15] libxl: Introduce libxl__sendmsg_fds and libxl__recvmsg_fds Ian Jackson
2012-02-24 18:57 ` [PATCH v2 00/15] libxl: child process handling Ian Jackson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1330109703-6536-12-git-send-email-ian.jackson@eu.citrix.com \
--to=ian.jackson@eu.citrix.com \
--cc=xen-devel@lists.xensource.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).