[lxc-devel] [PATCH] Fix race/corruption with multiple lxc-start, lxc-execute

[Dwight Engen]

If you start more than one lxc-start/lxc-execute with the same name at the
same time, or just do an lxc-start/lxc-execute with the name of a container
that is already running, lxc doesn't figure out that the container with this
name is already running until fairly late in the initialization process: ie
when __lxc_start() -> lxc_poll() -> lxc_command_mainloop_add() attempts to
create the same abstract socket name.

By this point a fair amount of initialization has been done that actually
messes up the running container. For example __lxc_start() -> lxc_spawn() ->
lxc_cgroup_create() -> lxc_one_cgroup_create() -> try_to_move_cgname() moves
the running container's cgroup to a name of deadXXXXXX.

The solution in this patch is to use the atomic existence of the abstract
socket name as the indicator that the container is already running.  To do
so, I just refactored lxc_command_mainloop_add() into an lxc_command_init()
routine that attempts to bind the socket, and ensure this is called earlier
before much initialization has been done.

In testing, I verified that maincmd_fd was still open at the time of lxc_fini,
so the entire lifetime of the container's run should be covered. The only
explicit close of this fd was in the reboot case of lxcapi_start(), which is
now moved to lxc_fini(), which I think is more appropriate.

Even though it is not checked any more, set maincmd_fd to -1 instead of 0 to
indicate its not open since 0 could be a valid fd.

Signed-off-by: Dwight Engen <dwight.engen at oracle.com>
---
 src/lxc/commands.c     |   17 ++++++++++++-----
 src/lxc/commands.h     |    1 +
 src/lxc/conf.c         |    1 +
 src/lxc/lxccontainer.c |    3 ---
 src/lxc/start.c        |   10 +++++++++-
 5 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/src/lxc/commands.c b/src/lxc/commands.c
index 737540d..40c685e 100644
--- a/src/lxc/commands.c
+++ b/src/lxc/commands.c
@@ -287,11 +287,9 @@ out_close:
 	goto out;
 }
 
-extern int lxc_command_mainloop_add(const char *name,
-				    struct lxc_epoll_descr *descr,
-				    struct lxc_handler *handler)
+extern int lxc_command_init(const char *name, struct lxc_handler *handler)
 {
-	int ret, fd;
+	int fd;
 	char path[sizeof(((struct sockaddr_un *)0)->sun_path)] = { 0 };
 	char *offset = &path[1];
 	int rc, len;
@@ -320,6 +318,16 @@ extern int lxc_command_mainloop_add(const char *name,
 		return -1;
 	}
 
+	handler->conf->maincmd_fd = fd;
+	return 0;
+}
+
+extern int lxc_command_mainloop_add(const char *name,
+				    struct lxc_epoll_descr *descr,
+				    struct lxc_handler *handler)
+{
+	int ret, fd = handler->conf->maincmd_fd;
+
 	ret = lxc_mainloop_add_handler(descr, fd, incoming_command_handler,
 				       handler);
 	if (ret) {
@@ -327,6 +335,5 @@ extern int lxc_command_mainloop_add(const char *name,
 		close(fd);
 	}
 
-	handler->conf->maincmd_fd = fd;
 	return ret;
 }
diff --git a/src/lxc/commands.h b/src/lxc/commands.h
index 3b0ac9a..0e1c8f9 100644
--- a/src/lxc/commands.h
+++ b/src/lxc/commands.h
@@ -60,6 +60,7 @@ extern int lxc_command_connected(const char *name, struct lxc_command *command,
 struct lxc_epoll_descr;
 struct lxc_handler;
 
+extern int lxc_command_init(const char *name, struct lxc_handler *handler);
 extern int lxc_command_mainloop_add(const char *name, struct lxc_epoll_descr *descr,
 				    struct lxc_handler *handler);
 
diff --git a/src/lxc/conf.c b/src/lxc/conf.c
index 84e51db..65f18c8 100644
--- a/src/lxc/conf.c
+++ b/src/lxc/conf.c
@@ -1895,6 +1895,7 @@ struct lxc_conf *lxc_conf_init(void)
 	new->console.master = -1;
 	new->console.slave = -1;
 	new->console.name[0] = '\0';
+	new->maincmd_fd = -1;
 	new->rootfs.mount = default_rootfs_mount;
 	new->loglevel = LXC_LOG_PRIORITY_NOTSET;
 	lxc_list_init(&new->cgroup);
diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 1345ab5..fd77aac 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -381,9 +381,6 @@ reboot:
 	if (conf->reboot) {
 		INFO("container requested reboot");
 		conf->reboot = 0;
-		if (conf->maincmd_fd)
-			close(conf->maincmd_fd);
-		conf->maincmd_fd = 0;
 		goto reboot;
 	}
 
diff --git a/src/lxc/start.c b/src/lxc/start.c
index 7320d74..05271fa 100644
--- a/src/lxc/start.c
+++ b/src/lxc/start.c
@@ -377,9 +377,12 @@ struct lxc_handler *lxc_init(const char *name, struct lxc_conf *conf)
 		goto out_free;
 	}
 
+	if (lxc_command_init(name, handler))
+		goto out_free_name;
+
 	if (lxc_read_seccomp_config(conf) != 0) {
 		ERROR("failed loading seccomp policy");
-		goto out_free_name;
+		goto out_close_maincmd_fd;
 	}
 
 	/* Begin the set the state to STARTING*/
@@ -421,6 +424,9 @@ out_delete_tty:
 	lxc_delete_tty(&conf->tty_info);
 out_aborting:
 	lxc_set_state(name, handler, ABORTING);
+out_close_maincmd_fd:
+	close(conf->maincmd_fd);
+	conf->maincmd_fd = -1;
 out_free_name:
 	free(handler->name);
 	handler->name = NULL;
@@ -446,6 +452,8 @@ void lxc_fini(const char *name, struct lxc_handler *handler)
 
 	lxc_delete_console(&handler->conf->console);
 	lxc_delete_tty(&handler->conf->tty_info);
+	close(handler->conf->maincmd_fd);
+	handler->conf->maincmd_fd = -1;
 	free(handler->name);
 	free(handler);
 }
-- 
1.7.1