[lxc-devel] [PATCH] Use pthread_atfork() to unlock mutexes after fork()
Stéphane Graber
stgraber at ubuntu.com
Wed Jan 1 21:11:20 UTC 2014
This patch caused a build failure on Android:
arm-linux-androideabi-gcc --sysroot=/tmp/android-build-scripts/android-ndk-r9b/platforms/android-9/arch-arm/ -DHAVE_CONFIG_H -I. -I../../src -fPIC -DPIC -I../../src -DLXCROOTFSMOUNT=\"/data/lxc/lxc/lib/lxc/rootfs\" -DLXCPATH=\"/data/lxc/lxc/var/lib/lxc\" -DLXC_GLOBAL_CONF=\"/data/lxc/lxc/etc/lxc/lxc.conf\" -DLXCINITDIR=\"/data/lxc/lxc/libexec\" -DLXCTEMPLATEDIR=\"/data/lxc/lxc/share/lxc/templates\" -DLOGPATH=\"/data/lxc/lxc/var/log/lxc\" -DLXC_DEFAULT_CONFIG=\"/data/lxc/lxc/etc/lxc/default.conf\" -DLXClxclock.c: In function 'process_lock_setup_atfork':
lxclock.c:332:2: error: implicit declaration of function 'pthread_atfork' [-Werror=implicit-function-declaration]
pthread_atfork(process_lock, process_unlock, process_unlock);
^
cc1: all warnings being treated as errors
make[3]: *** [liblxc_so-lxclock.o] Error 1
make[3]: Leaving directory `/tmp/android-build-scripts/lxc/src/lxc'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/tmp/android-build-scripts/lxc/src'
make[1]: *** [all] Error 2
make[1]: Leaving directory `/tmp/android-build-scripts/lxc/src'
make: *** [all-recursive] Error 1
It looks like pthread_atfork doesn't exist in bionic.
On Wed, Jan 01, 2014 at 01:52:33PM -0600, Serge Hallyn wrote:
> Quoting Andrey Mazo (mazo at telum.ru):
> > Signed-off-by: Andrey Mazo <mazo at telum.ru>
> > ---
> > src/lxc/Makefile.am | 3 ++-
> > src/lxc/attach.c | 2 --
> > src/lxc/bdev.c | 13 -------------
> > src/lxc/lxccontainer.c | 4 ----
> > src/lxc/lxclock.c | 23 ++++++++++++++++++++++-
> > src/lxc/monitor.c | 1 -
> > 6 files changed, 24 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/lxc/Makefile.am b/src/lxc/Makefile.am
> > index 74b38e2..f18d6e1 100644
> > --- a/src/lxc/Makefile.am
> > +++ b/src/lxc/Makefile.am
> > @@ -135,9 +135,10 @@ AM_CFLAGS += -DHAVE_SECCOMP
> > liblxc_so_SOURCES += seccomp.c
> > endif
> >
> > -liblxc_so_CFLAGS = -fPIC -DPIC $(AM_CFLAGS)
> > +liblxc_so_CFLAGS = -fPIC -DPIC $(AM_CFLAGS) -pthread
> >
> > liblxc_so_LDFLAGS = \
> > + -pthread \
> > -shared \
> > -Wl,-soname,liblxc.so.$(firstword $(subst ., ,$(VERSION)))
> >
> > diff --git a/src/lxc/attach.c b/src/lxc/attach.c
> > index 6749617..252e172 100644
> > --- a/src/lxc/attach.c
> > +++ b/src/lxc/attach.c
> > @@ -497,7 +497,6 @@ char *lxc_attach_getpwshell(uid_t uid)
> > NULL
> > };
> >
> > - process_unlock(); // we're no longer sharing
> > close(pipes[0]);
> >
> > /* we want to capture stdout */
> > @@ -790,7 +789,6 @@ int lxc_attach(const char* name, const char* lxcpath, lxc_attach_exec_t exec_fun
> > return -1;
> > }
> >
> > - process_unlock(); // we're no longer sharing
> > /* first subprocess begins here, we close the socket that is for the
> > * initial thread
> > */
> > diff --git a/src/lxc/bdev.c b/src/lxc/bdev.c
> > index b737eff..ec0d0ee 100644
> > --- a/src/lxc/bdev.c
> > +++ b/src/lxc/bdev.c
> > @@ -72,7 +72,6 @@ static int do_rsync(const char *src, const char *dest)
> > if (pid > 0)
> > return wait_for_pid(pid);
> >
> > - process_unlock(); // we're no longer sharing
> > l = strlen(src) + 2;
> > s = malloc(l);
> > if (!s)
> > @@ -197,7 +196,6 @@ static int do_mkfs(const char *path, const char *fstype)
> > if (pid > 0)
> > return wait_for_pid(pid);
> >
> > - process_unlock(); // we're no longer sharing
> > // If the file is not a block device, we don't want mkfs to ask
> > // us about whether to proceed.
> > close(0);
> > @@ -282,7 +280,6 @@ static int detect_fs(struct bdev *bdev, char *type, int len)
> > return ret;
> > }
> >
> > - process_unlock(); // we're no longer sharing
> > if (unshare(CLONE_NEWNS) < 0)
> > exit(1);
> >
> > @@ -574,7 +571,6 @@ static int zfs_clone(const char *opath, const char *npath, const char *oname,
> > if (!pid) {
> > char dev[MAXPATHLEN];
> >
> > - process_unlock(); // we're no longer sharing
> > ret = snprintf(dev, MAXPATHLEN, "%s/%s", zfsroot, nname);
> > if (ret < 0 || ret >= MAXPATHLEN)
> > exit(1);
> > @@ -598,7 +594,6 @@ static int zfs_clone(const char *opath, const char *npath, const char *oname,
> > if ((pid = fork()) < 0)
> > return -1;
> > if (!pid) {
> > - process_unlock(); // we're no longer sharing
> > execlp("zfs", "zfs", "destroy", path1, NULL);
> > exit(1);
> > }
> > @@ -609,7 +604,6 @@ static int zfs_clone(const char *opath, const char *npath, const char *oname,
> > if ((pid = fork()) < 0)
> > return -1;
> > if (!pid) {
> > - process_unlock(); // we're no longer sharing
> > execlp("zfs", "zfs", "snapshot", path1, NULL);
> > exit(1);
> > }
> > @@ -620,7 +614,6 @@ static int zfs_clone(const char *opath, const char *npath, const char *oname,
> > if ((pid = fork()) < 0)
> > return -1;
> > if (!pid) {
> > - process_unlock(); // we're no longer sharing
> > execlp("zfs", "zfs", "clone", option, path1, path2, NULL);
> > exit(1);
> > }
> > @@ -671,7 +664,6 @@ static int zfs_destroy(struct bdev *orig)
> > if (pid)
> > return wait_for_pid(pid);
> >
> > - process_unlock(); // we're no longer sharing
> > if (!zfs_list_entry(orig->src, output, MAXPATHLEN)) {
> > ERROR("Error: zfs entry for %s not found", orig->src);
> > return -1;
> > @@ -716,7 +708,6 @@ static int zfs_create(struct bdev *bdev, const char *dest, const char *n,
> > if (pid)
> > return wait_for_pid(pid);
> >
> > - process_unlock(); // we're no longer sharing
> > char dev[MAXPATHLEN];
> > ret = snprintf(dev, MAXPATHLEN, "%s/%s", zfsroot, n);
> > if (ret < 0 || ret >= MAXPATHLEN)
> > @@ -861,7 +852,6 @@ static int do_lvm_create(const char *path, unsigned long size, const char *thinp
> > if (pid > 0)
> > return wait_for_pid(pid);
> >
> > - process_unlock(); // we're no longer sharing
> > // lvcreate default size is in M, not bytes.
> > ret = snprintf(sz, 24, "%lu", size/1000000);
> > if (ret < 0 || ret >= 24)
> > @@ -921,7 +911,6 @@ static int lvm_snapshot(const char *orig, const char *path, unsigned long size)
> > if (pid > 0)
> > return wait_for_pid(pid);
> >
> > - process_unlock(); // we're no longer sharing
> > // lvcreate default size is in M, not bytes.
> > ret = snprintf(sz, 24, "%lu", size/1000000);
> > if (ret < 0 || ret >= 24)
> > @@ -1055,7 +1044,6 @@ static int lvm_destroy(struct bdev *orig)
> > if ((pid = fork()) < 0)
> > return -1;
> > if (!pid) {
> > - process_unlock(); // we're no longer sharing
> > execlp("lvremove", "lvremove", "-f", orig->src, NULL);
> > exit(1);
> > }
> > @@ -2091,7 +2079,6 @@ struct bdev *bdev_copy(const char *src, const char *oldname, const char *cname,
> > return new;
> > }
> >
> > - process_unlock(); // we're no longer sharing
> > if (unshare(CLONE_NEWNS) < 0) {
> > SYSERROR("unshare CLONE_NEWNS");
> > exit(1);
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index e1d004f..cf71f28 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -599,7 +599,6 @@ static bool lxcapi_start(struct lxc_container *c, int useinit, char * const argv
> > if (pid != 0)
> > return wait_on_daemonized_start(c, pid);
> >
> > - process_unlock(); // we're no longer sharing
> > /* second fork to be reparented by init */
> > pid = fork();
> > if (pid < 0) {
> > @@ -837,7 +836,6 @@ static bool create_run_template(struct lxc_container *c, char *tpath, bool quiet
> > char **newargv;
> > struct lxc_conf *conf = c->lxc_conf;
> >
> > - process_unlock(); // we're no longer sharing
> > if (quiet) {
> > close(0);
> > close(1);
> > @@ -1234,7 +1232,6 @@ static bool lxcapi_create(struct lxc_container *c, const char *t,
> > if (pid == 0) { // child
> > struct bdev *bdev = NULL;
> >
> > - process_unlock(); // we're no longer sharing
> > if (!(bdev = do_bdev_create(c, bdevtype, specs))) {
> > ERROR("Error creating backing store type %s for %s",
> > bdevtype ? bdevtype : "(none)", c->name);
> > @@ -2326,7 +2323,6 @@ static int clone_update_rootfs(struct lxc_container *c0,
> > if (pid > 0)
> > return wait_for_pid(pid);
> >
> > - process_unlock(); // we're no longer sharing
> > bdev = bdev_init(c->lxc_conf->rootfs.path, c->lxc_conf->rootfs.mount, NULL);
> > if (!bdev)
> > exit(1);
> > diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
> > index 28691b0..36c4bd3 100644
> > --- a/src/lxc/lxclock.c
> > +++ b/src/lxc/lxclock.c
> > @@ -87,7 +87,7 @@ void unlock_mutex(pthread_mutex_t *l)
> > int ret;
> >
> > if ((ret = pthread_mutex_unlock(l)) != 0) {
> > - fprintf(stderr, "pthread_mutex_lock returned:%d %s", ret, strerror(ret));
> > + fprintf(stderr, "pthread_mutex_unlock returned:%d %s", ret, strerror(ret));
> > dump_stacktrace();
> > exit(1);
> > }
> > @@ -315,6 +315,21 @@ void process_unlock(void)
> > unlock_mutex(&thread_mutex);
> > }
> >
> > +/* One thread can do fork() while another one is holding a mutex.
> > + * There is only one thread in child just after the fork(), so noone will ever release that mutex.
> > + * We setup a "child" fork handler to unlock the mutex just after the fork().
> > + * For several mutex types, unlocking an unlocked mutex can lead to undefined behavior.
>
> Thanks for pointing this out. I have to say I don't like that we have
> to lock this... though it should be safe with our current usage (and
> I see Caglar has verified it :)
>
> An alternative would be to have a unlock_if_locked() for each lock,
> and do pthread_atfork(NULL, NULL, process_unlock_if_locked()).
> There would be no risk of a race since the child is single-threaded.
>
> But this is ok for now.
>
> Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
>
> (Note I'm acking, but not applying until tomorrow)
>
> > + * One way to deal with it is to setup "prepare" fork handler
> > + * to lock the mutex before fork() and both "parent" and "child" fork handlers
> > + * to unlock the mutex.
> > + * This forbids doing fork() while explicitly holding the lock.
> > + */
> > +__attribute__((constructor))
> > +static void process_lock_setup_atfork(void)
> > +{
> > + pthread_atfork(process_lock, process_unlock, process_unlock);
> > +}
> > +
> > /* Protects static const values inside the lxc_global_config_value funtion */
> > void static_lock(void)
> > {
> > @@ -326,6 +341,12 @@ void static_unlock(void)
> > unlock_mutex(&static_mutex);
> > }
> >
> > +__attribute__((constructor))
> > +static void static_lock_setup_atfork(void)
> > +{
> > + pthread_atfork(static_lock, static_unlock, static_unlock);
> > +}
> > +
> > int container_mem_lock(struct lxc_container *c)
> > {
> > return lxclock(c->privlock, 0);
> > diff --git a/src/lxc/monitor.c b/src/lxc/monitor.c
> > index 7e0a713..0b6a065 100644
> > --- a/src/lxc/monitor.c
> > +++ b/src/lxc/monitor.c
> > @@ -300,7 +300,6 @@ int lxc_monitord_spawn(const char *lxcpath)
> > return 0;
> > }
> >
> > - process_unlock(); // we're no longer sharing
> > if (pipe(pipefd) < 0) {
> > SYSERROR("failed to create pipe");
> > exit(EXIT_FAILURE);
> > --
> > 1.8.4.5
> >
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20140101/dfc07c5c/attachment.pgp>
More information about the lxc-devel
mailing list