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

[Roger Pau Monnà <roger.pau@xxxxxxxxxxxxx>]

2012/2/20 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?


Just tested on a NetBSD system, and PTHREAD_MUTEX_INITIALIZER macro is allowed.

>> +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

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