[lxc-devel] [PATCH] Use pthread_atfork() to unlock mutexes after fork()
Serge Hallyn
serge.hallyn at ubuntu.com
Thu Jan 2 05:37:32 UTC 2014
Quoting Stéphane Graber (stgraber at ubuntu.com):
> 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);
Hm, according to
http://stackoverflow.com/questions/12370970/undefined-reference-to-pthread-atfork-while-i-was-trying-to-port-libpcsclite-t
perhaps all we need is to declare the fn before its use. Do you
have a local bionic compiler handy to test that on?
> ^
> 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
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
More information about the lxc-devel
mailing list