From a529bc25cd7d4f80510f01b200d182be4e4815e3 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Mon, 25 Dec 2017 14:52:39 +0100 Subject: [PATCH 1/5] mainloop: add mainloop macros This makes it clearer why handlers return what value. Signed-off-by: Christian Brauner --- src/lxc/console.c | 65 +++++++++++++++++++++++++++------------------- src/lxc/console.h | 2 ++ src/lxc/mainloop.c | 15 +++++------ src/lxc/mainloop.h | 3 +++ src/lxc/start.h | 2 +- 5 files changed, 52 insertions(+), 35 deletions(-) diff --git a/src/lxc/console.c b/src/lxc/console.c index ad4a31405..39515e7f8 100644 --- a/src/lxc/console.c +++ b/src/lxc/console.c @@ -122,7 +122,7 @@ int lxc_console_cb_signal_fd(int fd, uint32_t events, void *cbdata, if (siginfo.ssi_signo == SIGTERM) { DEBUG("Received SIGTERM. Detaching from the console"); - return 1; + return LXC_MAINLOOP_CLOSE; } if (siginfo.ssi_signo == SIGWINCH) @@ -204,8 +204,8 @@ void lxc_console_signal_fini(struct lxc_tty_state *ts) free(ts); } -static int lxc_console_cb_con(int fd, uint32_t events, void *data, - struct lxc_epoll_descr *descr) +int lxc_console_cb_con(int fd, uint32_t events, void *data, + struct lxc_epoll_descr *descr) { struct lxc_console *console = (struct lxc_console *)data; char buf[LXC_CONSOLE_BUFFER_SIZE]; @@ -225,7 +225,7 @@ static int lxc_console_cb_con(int fd, uint32_t events, void *data, return 0; } close(fd); - return 1; + return LXC_MAINLOOP_CLOSE; } if (fd == console->peer) @@ -259,27 +259,36 @@ static int lxc_console_cb_con(int fd, uint32_t events, void *data, return 0; } -static void lxc_console_mainloop_add_peer(struct lxc_console *console) +static int lxc_console_mainloop_add_peer(struct lxc_console *console) { - if (console->peer >= 0) { - if (lxc_mainloop_add_handler(console->descr, console->peer, - lxc_console_cb_con, console)) - WARN("Failed to add console peer handler to mainloop"); - } + int ret; - if (console->tty_state && console->tty_state->sigfd != -1) { - if (lxc_mainloop_add_handler(console->descr, - console->tty_state->sigfd, - lxc_console_cb_signal_fd, - console->tty_state)) { - WARN("Failed to add signal handler to mainloop"); + if (console->peer >= 0) { + ret = lxc_mainloop_add_handler(console->descr, console->peer, + lxc_console_cb_con, console); + if (ret < 0) { + WARN("Failed to add console peer handler to mainloop"); + return -1; } } + + if (!console->tty_state || console->tty_state->sigfd < 0) + return 0; + + ret = lxc_mainloop_add_handler(console->descr, console->tty_state->sigfd, + lxc_console_cb_signal_fd, console->tty_state); + if (ret < 0) { + WARN("Failed to add signal handler to mainloop"); + return -1; + } + + return 0; } extern int lxc_console_mainloop_add(struct lxc_epoll_descr *descr, struct lxc_conf *conf) { + int ret; struct lxc_console *console = &conf->console; if (!conf->rootfs.path) { @@ -303,7 +312,9 @@ extern int lxc_console_mainloop_add(struct lxc_epoll_descr *descr, * does attach to it in lxc_console_allocate() */ console->descr = descr; - lxc_console_mainloop_add_peer(console); + ret = lxc_console_mainloop_add_peer(console); + if (ret < 0) + return -1; return 0; } @@ -412,7 +423,9 @@ static int lxc_console_peer_proxy_alloc(struct lxc_console *console, int sockfd) console->tty_state = ts; console->peer = console->peerpty.slave; console->peerpty.busy = sockfd; - lxc_console_mainloop_add_peer(console); + ret = lxc_console_mainloop_add_peer(console); + if (ret < 0) + goto err1; DEBUG("%d %s peermaster:%d sockfd:%d", lxc_raw_getpid(), __FUNCTION__, console->peerpty.master, sockfd); return 0; @@ -794,10 +807,10 @@ int lxc_console_cb_tty_stdin(int fd, uint32_t events, void *cbdata, char c; if (fd != ts->stdinfd) - return 1; + return LXC_MAINLOOP_CLOSE; if (lxc_read_nointr(ts->stdinfd, &c, 1) <= 0) - return 1; + return LXC_MAINLOOP_CLOSE; if (ts->escape >= 1) { /* we want to exit the console with Ctrl+a q */ @@ -807,13 +820,13 @@ int lxc_console_cb_tty_stdin(int fd, uint32_t events, void *cbdata, } if (c == 'q' && ts->saw_escape) - return 1; + return LXC_MAINLOOP_CLOSE; ts->saw_escape = 0; } if (lxc_write_nointr(ts->masterfd, &c, 1) <= 0) - return 1; + return LXC_MAINLOOP_CLOSE; return 0; } @@ -826,17 +839,17 @@ int lxc_console_cb_tty_master(int fd, uint32_t events, void *cbdata, int r, w; if (fd != ts->masterfd) - return 1; + return LXC_MAINLOOP_CLOSE; r = lxc_read_nointr(fd, buf, sizeof(buf)); if (r <= 0) - return 1; + return LXC_MAINLOOP_CLOSE; w = lxc_write_nointr(ts->stdoutfd, buf, r); if (w <= 0) { - return 1; + return LXC_MAINLOOP_CLOSE; } else if (w != r) { - SYSERROR("failed to write"); + SYSERROR("Failed to write"); return 1; } diff --git a/src/lxc/console.h b/src/lxc/console.h index 60b299631..375349e02 100644 --- a/src/lxc/console.h +++ b/src/lxc/console.h @@ -223,5 +223,7 @@ extern void lxc_console_signal_fini(struct lxc_tty_state *ts); extern int lxc_console_write_ringbuffer(struct lxc_console *console); extern int lxc_console_create_log_file(struct lxc_console *console); +extern int lxc_console_cb_con(int fd, uint32_t events, void *data, + struct lxc_epoll_descr *descr); #endif diff --git a/src/lxc/mainloop.c b/src/lxc/mainloop.c index ab9c27dc0..a7383d632 100644 --- a/src/lxc/mainloop.c +++ b/src/lxc/mainloop.c @@ -40,12 +40,11 @@ struct mainloop_handler { int lxc_mainloop(struct lxc_epoll_descr *descr, int timeout_ms) { - int i, nfds; + int i, nfds, ret; struct mainloop_handler *handler; struct epoll_event events[MAX_EVENTS]; for (;;) { - nfds = epoll_wait(descr->epfd, events, MAX_EVENTS, timeout_ms); if (nfds < 0) { if (errno == EINTR) @@ -54,13 +53,13 @@ int lxc_mainloop(struct lxc_epoll_descr *descr, int timeout_ms) } for (i = 0; i < nfds; i++) { - handler = - (struct mainloop_handler *) events[i].data.ptr; + handler = events[i].data.ptr; - /* If the handler returns a positive value, exit - the mainloop */ - if (handler->callback(handler->fd, events[i].events, - handler->data, descr) > 0) + /* If the handler returns a positive value, exit the + * mainloop. */ + ret = handler->callback(handler->fd, events[i].events, + handler->data, descr); + if (ret == LXC_MAINLOOP_CLOSE) return 0; } diff --git a/src/lxc/mainloop.h b/src/lxc/mainloop.h index 46a820c66..ea53a012c 100644 --- a/src/lxc/mainloop.h +++ b/src/lxc/mainloop.h @@ -27,6 +27,9 @@ #include #include "list.h" +#define LXC_MAINLOOP_CONTINUE 0 +#define LXC_MAINLOOP_CLOSE 1 + struct lxc_epoll_descr { int epfd; struct lxc_list handlers; diff --git a/src/lxc/start.h b/src/lxc/start.h index 498097b89..0e01c3cf0 100644 --- a/src/lxc/start.h +++ b/src/lxc/start.h @@ -138,5 +138,5 @@ extern int __lxc_start(const char *, struct lxc_handler *, struct lxc_operations *, void *, const char *, bool); extern int resolve_clone_flags(struct lxc_handler *handler); -#endif +#endif From 3c319edbb0b139f16ce213041f438c76fa7061fa Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Mon, 25 Dec 2017 14:53:40 +0100 Subject: [PATCH 2/5] mainloop: capture output of short-lived init procs The handler for the signal fd will detect when the init process of a container has exited and cause the mainloop to close. However, this can happen before the console handlers - or any other events for that matter - are handled. So in the case of init exiting we still need to allow for all buffered input to the console to be handled before exiting. This allows us to capture output from short-lived init processes. This is conceptually equivalent to my implementation of ExecReaderToChannel() https://github.com/lxc/lxd/blob/master/shared/util_linux.go#L527 Closes #1694. Signed-off-by: Christian Brauner --- src/lxc/mainloop.c | 13 ++++--- src/lxc/start.c | 89 +++++++++++++++++++++++++++++----------------- src/lxc/start.h | 3 ++ 3 files changed, 67 insertions(+), 38 deletions(-) diff --git a/src/lxc/mainloop.c b/src/lxc/mainloop.c index a7383d632..c102295ef 100644 --- a/src/lxc/mainloop.c +++ b/src/lxc/mainloop.c @@ -20,11 +20,12 @@ * License along with this library; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA */ -#include -#include -#include + #include #include +#include +#include +#include #include #include @@ -49,6 +50,7 @@ int lxc_mainloop(struct lxc_epoll_descr *descr, int timeout_ms) if (nfds < 0) { if (errno == EINTR) continue; + return -1; } @@ -56,14 +58,15 @@ int lxc_mainloop(struct lxc_epoll_descr *descr, int timeout_ms) handler = events[i].data.ptr; /* If the handler returns a positive value, exit the - * mainloop. */ + * mainloop. + */ ret = handler->callback(handler->fd, events[i].events, handler->data, descr); if (ret == LXC_MAINLOOP_CLOSE) return 0; } - if (nfds == 0 && timeout_ms != 0) + if (nfds == 0) return 0; if (lxc_list_empty(&descr->handlers)) diff --git a/src/lxc/start.c b/src/lxc/start.c index d70764fa1..e4f8fb976 100644 --- a/src/lxc/start.c +++ b/src/lxc/start.c @@ -299,11 +299,10 @@ static int setup_signal_fd(sigset_t *oldmask) static int signal_handler(int fd, uint32_t events, void *data, struct lxc_epoll_descr *descr) { - struct signalfd_siginfo siginfo; - siginfo_t info; int ret; - pid_t *pid = data; - bool init_died = false; + siginfo_t info; + struct signalfd_siginfo siginfo; + struct lxc_handler *hdlr = data; ret = read(fd, &siginfo, sizeof(siginfo)); if (ret < 0) { @@ -318,34 +317,34 @@ static int signal_handler(int fd, uint32_t events, void *data, /* Check whether init is running. */ info.si_pid = 0; - ret = waitid(P_PID, *pid, &info, WEXITED | WNOWAIT | WNOHANG); - if (ret == 0 && info.si_pid == *pid) - init_died = true; + ret = waitid(P_PID, hdlr->pid, &info, WEXITED | WNOWAIT | WNOHANG); + if (ret == 0 && info.si_pid == hdlr->pid) + hdlr->init_died = true; if (siginfo.ssi_signo != SIGCHLD) { - kill(*pid, siginfo.ssi_signo); - INFO("Forwarded signal %d to pid %d.", siginfo.ssi_signo, *pid); - return init_died ? 1 : 0; + kill(hdlr->pid, siginfo.ssi_signo); + INFO("Forwarded signal %d to pid %d.", siginfo.ssi_signo, hdlr->pid); + return hdlr->init_died ? LXC_MAINLOOP_CLOSE : 0; } if (siginfo.ssi_code == CLD_STOPPED) { INFO("Container init process was stopped."); - return init_died ? 1 : 0; + return hdlr->init_died ? LXC_MAINLOOP_CLOSE : 0; } else if (siginfo.ssi_code == CLD_CONTINUED) { INFO("Container init process was continued."); - return init_died ? 1 : 0; + return hdlr->init_died ? LXC_MAINLOOP_CLOSE : 0; } /* More robustness, protect ourself from a SIGCHLD sent * by a process different from the container init. */ - if (siginfo.ssi_pid != *pid) { - NOTICE("Received SIGCHLD from pid %d instead of container init %d.", siginfo.ssi_pid, *pid); - return init_died ? 1 : 0; + if (siginfo.ssi_pid != hdlr->pid) { + NOTICE("Received SIGCHLD from pid %d instead of container init %d.", siginfo.ssi_pid, hdlr->pid); + return hdlr->init_died ? LXC_MAINLOOP_CLOSE : 0; } - DEBUG("Container init process %d exited.", *pid); - return 1; + DEBUG("Container init process %d exited.", hdlr->pid); + return LXC_MAINLOOP_CLOSE; } static int lxc_serve_state_clients(const char *name, @@ -467,37 +466,61 @@ int lxc_set_state(const char *name, struct lxc_handler *handler, int lxc_poll(const char *name, struct lxc_handler *handler) { + int ret; + struct lxc_epoll_descr descr, descr_console; int sigfd = handler->sigfd; - int pid = handler->pid; - struct lxc_epoll_descr descr; - if (lxc_mainloop_open(&descr)) { - ERROR("Failed to create LXC mainloop."); + ret = lxc_mainloop_open(&descr); + if (ret < 0) { + ERROR("Failed to create mainloop"); goto out_sigfd; } - if (lxc_mainloop_add_handler(&descr, sigfd, signal_handler, &pid)) { - ERROR("Failed to add signal handler with file descriptor %d to LXC mainloop.", sigfd); - goto out_mainloop_open; + ret = lxc_mainloop_open(&descr_console); + if (ret < 0) { + ERROR("Failed to create console mainloop"); + goto out_mainloop; } - if (lxc_console_mainloop_add(&descr, handler->conf)) { - ERROR("Failed to add console handler to LXC mainloop."); - goto out_mainloop_open; + ret = lxc_mainloop_add_handler(&descr, sigfd, signal_handler, handler); + if (ret < 0) { + ERROR("Failed to add signal handler for %d to mainloop", sigfd); + goto out_mainloop_console; } - if (lxc_cmd_mainloop_add(name, &descr, handler)) { - ERROR("Failed to add command handler to LXC mainloop."); - goto out_mainloop_open; + ret = lxc_console_mainloop_add(&descr, handler->conf); + if (ret < 0) { + ERROR("Failed to add console handlers to mainloop"); + goto out_mainloop_console; } - TRACE("lxc mainloop is ready"); + ret = lxc_console_mainloop_add(&descr_console, handler->conf); + if (ret < 0) { + ERROR("Failed to add console handlers to console mainloop"); + goto out_mainloop_console; + } - return lxc_mainloop(&descr, -1); + ret = lxc_cmd_mainloop_add(name, &descr, handler); + if (ret < 0) { + ERROR("Failed to add command handler to mainloop"); + goto out_mainloop_console; + } -out_mainloop_open: + TRACE("Mainloop is ready"); + + ret = lxc_mainloop(&descr, -1); + if (ret < 0) + return -1; + if (handler->init_died) + return lxc_mainloop(&descr_console, 0); + return ret; + +out_mainloop: lxc_mainloop_close(&descr); +out_mainloop_console: + lxc_mainloop_close(&descr_console); + out_sigfd: close(sigfd); diff --git a/src/lxc/start.h b/src/lxc/start.h index 0e01c3cf0..bfa4c9062 100644 --- a/src/lxc/start.h +++ b/src/lxc/start.h @@ -85,6 +85,9 @@ struct lxc_handler { /* The child's pid. */ pid_t pid; + /* Whether the child has already exited. */ + bool init_died; + /* The signal mask prior to setting up the signal file descriptor. */ sigset_t oldmask; From 1cc8bd4b61988dcd73dac35243a6ed1591cc1299 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Tue, 26 Dec 2017 13:45:12 +0100 Subject: [PATCH 3/5] start: properly cleanup mainloop Signed-off-by: Christian Brauner --- src/lxc/mainloop.c | 5 +++- src/lxc/start.c | 69 +++++++++++++++++++++++++++++----------------- 2 files changed, 47 insertions(+), 27 deletions(-) diff --git a/src/lxc/mainloop.c b/src/lxc/mainloop.c index c102295ef..9afded97c 100644 --- a/src/lxc/mainloop.c +++ b/src/lxc/mainloop.c @@ -161,5 +161,8 @@ int lxc_mainloop_close(struct lxc_epoll_descr *descr) iterator = next; } - return close(descr->epfd); + if (descr->epfd >= 0) + return close(descr->epfd); + + return 0; } diff --git a/src/lxc/start.c b/src/lxc/start.c index e4f8fb976..b57661f8c 100644 --- a/src/lxc/start.c +++ b/src/lxc/start.c @@ -73,6 +73,7 @@ #include "confile_utils.h" #include "console.h" #include "error.h" +#include "list.h" #include "log.h" #include "lxccontainer.h" #include "lxclock.h" @@ -468,7 +469,6 @@ int lxc_poll(const char *name, struct lxc_handler *handler) { int ret; struct lxc_epoll_descr descr, descr_console; - int sigfd = handler->sigfd; ret = lxc_mainloop_open(&descr); if (ret < 0) { @@ -482,9 +482,9 @@ int lxc_poll(const char *name, struct lxc_handler *handler) goto out_mainloop; } - ret = lxc_mainloop_add_handler(&descr, sigfd, signal_handler, handler); + ret = lxc_mainloop_add_handler(&descr, handler->sigfd, signal_handler, handler); if (ret < 0) { - ERROR("Failed to add signal handler for %d to mainloop", sigfd); + ERROR("Failed to add signal handler for %d to mainloop", handler->sigfd); goto out_mainloop_console; } @@ -509,22 +509,27 @@ int lxc_poll(const char *name, struct lxc_handler *handler) TRACE("Mainloop is ready"); ret = lxc_mainloop(&descr, -1); - if (ret < 0) - return -1; - if (handler->init_died) - return lxc_mainloop(&descr_console, 0); - return ret; + close(descr.epfd); + descr.epfd = -EBADF; + if (ret < 0 || !handler->init_died) + goto out_mainloop; + + ret = lxc_mainloop(&descr_console, 0); out_mainloop: lxc_mainloop_close(&descr); + TRACE("Closed mainloop"); out_mainloop_console: lxc_mainloop_close(&descr_console); + TRACE("Closed console mainloop"); out_sigfd: - close(sigfd); + close(handler->sigfd); + TRACE("Closed signal file descriptor %d", handler->sigfd); + handler->sigfd = -EBADF; - return -1; + return ret; } void lxc_zero_handler(struct lxc_handler *handler) @@ -552,10 +557,32 @@ void lxc_zero_handler(struct lxc_handler *handler) handler->sync_sock[1] = -1; } +static void lxc_put_nsfds(struct lxc_handler *handler) +{ + int i; + + for (i = 0; i < LXC_NS_MAX; i++) { + if (handler->nsfd[i] < 0) + continue; + + close(handler->nsfd[i]); + handler->nsfd[i] = -EBADF; + } +} + void lxc_free_handler(struct lxc_handler *handler) { - if (handler->conf && handler->conf->maincmd_fd) - close(handler->conf->maincmd_fd); + if (handler->pinfd >= 0) + close(handler->pinfd); + + if (handler->sigfd >= 0) + close(handler->sigfd); + + lxc_put_nsfds(handler); + + if (handler->conf && handler->conf->reboot == 0) + if (handler->conf->maincmd_fd) + close(handler->conf->maincmd_fd); if (handler->state_socket_pair[0] >= 0) close(handler->state_socket_pair[0]); @@ -565,6 +592,7 @@ void lxc_free_handler(struct lxc_handler *handler) handler->conf = NULL; free(handler); + handler = NULL; } struct lxc_handler *lxc_init_handler(const char *name, struct lxc_conf *conf, @@ -590,6 +618,8 @@ struct lxc_handler *lxc_init_handler(const char *name, struct lxc_conf *conf, handler->conf = conf; handler->lxcpath = lxcpath; handler->pinfd = -1; + handler->sigfd = -EBADF; + handler->init_died = false; handler->state_socket_pair[0] = handler->state_socket_pair[1] = -1; if (handler->conf->reboot == 0) lxc_list_init(&handler->conf->state_clients); @@ -797,14 +827,6 @@ void lxc_fini(const char *name, struct lxc_handler *handler) while (namespace_count--) free(namespaces[namespace_count]); - for (i = 0; i < LXC_NS_MAX; i++) { - if (handler->nsfd[i] < 0) - continue; - - close(handler->nsfd[i]); - handler->nsfd[i] = -1; - } - cgroup_destroy(handler); if (handler->conf->reboot == 0) { @@ -866,15 +888,10 @@ void lxc_fini(const char *name, struct lxc_handler *handler) free(cur); } - if (handler->data_sock[0] != -1) { - close(handler->data_sock[0]); - close(handler->data_sock[1]); - } - if (handler->conf->ephemeral == 1 && handler->conf->reboot != 1) lxc_destroy_container_on_signal(handler, name); - free(handler); + lxc_free_handler(handler); } void lxc_abort(const char *name, struct lxc_handler *handler) From a63fade55b1e091bc1933bff181a64c38a28e302 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Tue, 26 Dec 2017 18:00:08 +0100 Subject: [PATCH 4/5] console: do not allow non-pty devices on open() We don't allow non-pty devices anyway so don't let open() create unneeded files. Signed-off-by: Christian Brauner --- src/lxc/console.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lxc/console.c b/src/lxc/console.c index 39515e7f8..ac7999e4a 100644 --- a/src/lxc/console.c +++ b/src/lxc/console.c @@ -518,9 +518,9 @@ static int lxc_console_peer_default(struct lxc_console *console) goto out; } - console->peer = lxc_unpriv(open(path, O_CLOEXEC | O_RDWR | O_CREAT | O_APPEND, 0600)); + console->peer = lxc_unpriv(open(path, O_RDWR | O_CLOEXEC)); if (console->peer < 0) { - ERROR("failed to open \"%s\": %s", path, strerror(errno)); + ERROR("Failed to open \"%s\": %s", path, strerror(errno)); return -ENOTTY; } DEBUG("using \"%s\" as peer tty device", path); From 12c2798ed1143e45f7a42e56a57502ee5a6c0b68 Mon Sep 17 00:00:00 2001 From: Christian Brauner Date: Tue, 26 Dec 2017 20:57:12 +0100 Subject: [PATCH 5/5] mainloop: use epoll_create1(EPOLL_CLOEXEC) Signed-off-by: Christian Brauner --- src/lxc/mainloop.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/lxc/mainloop.c b/src/lxc/mainloop.c index 9afded97c..84dfb39e9 100644 --- a/src/lxc/mainloop.c +++ b/src/lxc/mainloop.c @@ -134,15 +134,10 @@ int lxc_mainloop_del_handler(struct lxc_epoll_descr *descr, int fd) int lxc_mainloop_open(struct lxc_epoll_descr *descr) { /* hint value passed to epoll create */ - descr->epfd = epoll_create(2); + descr->epfd = epoll_create1(EPOLL_CLOEXEC); if (descr->epfd < 0) return -1; - if (fcntl(descr->epfd, F_SETFD, FD_CLOEXEC)) { - close(descr->epfd); - return -1; - } - lxc_list_init(&descr->handlers); return 0; }