Quoting Andrey Mazo ([email protected]): > Signed-off-by: Andrey Mazo <[email protected]> > --- > 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 <[email protected]> (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 > [email protected] > http://lists.linuxcontainers.org/listinfo/lxc-devel _______________________________________________ lxc-devel mailing list [email protected] http://lists.linuxcontainers.org/listinfo/lxc-devel
