Fix race condition with zed pidfile creation

When the zed is started as a forking daemon (by default),
a race-condition exists where the parent process can terminate before
the pidfile has been created by the grandchild process.  When invoked
as a Type=forking systemd service, this can result in the following:

  systemd[1]: Starting ZFS Event Daemon (zed)...
  systemd[1]: PID file /var/run/zed.pid not readable (yet?) after start.

This commit adds a daemonize pipe to allow the grandchild process to
signal the parent process that initialization is complete (and the
pidfile has been created).  The parent process will wait for this
notification before exiting.

Signed-off-by: Chris Dunlap <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Issue #2252

4 files changed, 183 insertions, 21 deletions

diff --git a/cmd/zed/zed.c b/cmd/zed/zed.c
index c54a59b0a..bf29c0486 100644
--- a/cmd/zed/zed.c
+++ b/cmd/zed/zed.c
@@ -93,6 +93,9 @@ _setup_sig_handlers(void)
  * Lock all current and future pages in the virtual memory address space.
  *   Access to locked pages will never be delayed by a page fault.
  * EAGAIN is tested up to max_tries in case this is a transient error.
+ * Note that memory locks are not inherited by a child created via fork()
+ *   and are automatically removed during an execve().  As such, this must
+ *   be called after the daemon fork()s (when running in the background).
  */
 static void
 _lock_memory(void)
@@ -117,25 +120,57 @@ _lock_memory(void)
 }
 
 /*
- * Transform the process into a daemon.
+ * Start daemonization of the process including the double fork().
+ * The parent process will block here until _finish_daemonize() is called
+ *   (in the grandchild process), at which point the parent process will exit.
+ *   This prevents the parent process from exiting until initialization is
+ *   complete.
  */
 static void
-_become_daemon(void)
+_start_daemonize(void)
 {
 	pid_t pid;
-	int fd;
+	struct sigaction sa;
+
+	/* Create pipe for communicating with child during daemonization. */
+	zed_log_pipe_open();
 
+	/* Background process and ensure child is not process group leader. */
 	pid = fork();
 	if (pid < 0) {
 		zed_log_die("Failed to create child process: %s",
 		    strerror(errno));
 	} else if (pid > 0) {
+
+		/* Close writes since parent will only read from pipe. */
+		zed_log_pipe_close_writes();
+
+		/* Wait for notification that daemonization is complete. */
+		zed_log_pipe_wait();
+
+		zed_log_pipe_close_reads();
 		_exit(EXIT_SUCCESS);
 	}
+
+	/* Close reads since child will only write to pipe. */
+	zed_log_pipe_close_reads();
+
+	/* Create independent session and detach from terminal. */
 	if (setsid() < 0)
 		zed_log_die("Failed to create new session: %s",
 		    strerror(errno));
 
+	/* Prevent child from terminating on HUP when session leader exits. */
+	if (sigemptyset(&sa.sa_mask) < 0)
+		zed_log_die("Failed to initialize sigset");
+
+	sa.sa_flags = 0;
+	sa.sa_handler = SIG_IGN;
+
+	if (sigaction(SIGHUP, &sa, NULL) < 0)
+		zed_log_die("Failed to ignore SIGHUP");
+
+	/* Ensure process cannot re-acquire terminal. */
 	pid = fork();
 	if (pid < 0) {
 		zed_log_die("Failed to create grandchild process: %s",
@@ -143,25 +178,40 @@ _become_daemon(void)
 	} else if (pid > 0) {
 		_exit(EXIT_SUCCESS);
 	}
-	fd = open("/dev/null", O_RDWR);
+}
+
+/*
+ * Finish daemonization of the process by closing stdin/stdout/stderr.
+ * This must be called at the end of initialization after all external
+ *   communication channels are established and accessible.
+ */
+static void
+_finish_daemonize(void)
+{
+	int devnull;
 
-	if (fd < 0)
+	/* Preserve fd 0/1/2, but discard data to/from stdin/stdout/stderr. */
+	devnull = open("/dev/null", O_RDWR);
+	if (devnull < 0)
 		zed_log_die("Failed to open /dev/null: %s", strerror(errno));
 
-	if (dup2(fd, STDIN_FILENO) < 0)
+	if (dup2(devnull, STDIN_FILENO) < 0)
 		zed_log_die("Failed to dup /dev/null onto stdin: %s",
 		    strerror(errno));
 
-	if (dup2(fd, STDOUT_FILENO) < 0)
+	if (dup2(devnull, STDOUT_FILENO) < 0)
 		zed_log_die("Failed to dup /dev/null onto stdout: %s",
 		    strerror(errno));
 
-	if (dup2(fd, STDERR_FILENO) < 0)
+	if (dup2(devnull, STDERR_FILENO) < 0)
 		zed_log_die("Failed to dup /dev/null onto stderr: %s",
 		    strerror(errno));
 
-	if (close(fd) < 0)
+	if (close(devnull) < 0)
 		zed_log_die("Failed to close /dev/null: %s", strerror(errno));
+
+	/* Notify parent that daemonization is complete. */
+	zed_log_pipe_close_writes();
 }
 
 /*
@@ -184,33 +234,36 @@ main(int argc, char *argv[])
 	if (geteuid() != 0)
 		zed_log_die("Must be run as root");
 
-	(void) umask(0);
-
-	_setup_sig_handlers();
-
 	zed_conf_parse_file(zcp);
 
 	zed_file_close_from(STDERR_FILENO + 1);
 
+	(void) umask(0);
+
 	if (chdir("/") < 0)
 		zed_log_die("Failed to change to root directory");
 
 	if (zed_conf_scan_dir(zcp) < 0)
 		exit(EXIT_FAILURE);
 
-	if (zcp->do_memlock)
-		_lock_memory();
-
 	if (!zcp->do_foreground) {
-		_become_daemon();
+		_start_daemonize();
 		zed_log_syslog_open(LOG_DAEMON);
-		zed_log_stderr_close();
 	}
-	zed_log_msg(LOG_NOTICE,
-	    "ZFS Event Daemon %s-%s", ZFS_META_VERSION, ZFS_META_RELEASE);
+	_setup_sig_handlers();
+
+	if (zcp->do_memlock)
+		_lock_memory();
 
 	(void) zed_conf_write_pid(zcp);
 
+	if (!zcp->do_foreground)
+		_finish_daemonize();
+
+	zed_log_msg(LOG_NOTICE,
+	    "ZFS Event Daemon %s-%s (PID %d)",
+	    ZFS_META_VERSION, ZFS_META_RELEASE, (int) getpid());
+
 	if (zed_conf_open_state(zcp) < 0)
 		exit(EXIT_FAILURE);
 
diff --git a/cmd/zed/zed_conf.c b/cmd/zed/zed_conf.c
index 78b45e910..e6f601f3a 100644
--- a/cmd/zed/zed_conf.c
+++ b/cmd/zed/zed_conf.c
@@ -430,7 +430,13 @@ zed_conf_scan_dir(struct zed_conf *zcp)
 /*
  * Write the PID file specified in [zcp].
  * Return 0 on success, -1 on error.
- * XXX: This must be called after fork()ing to become a daemon.
+ * This must be called after fork()ing to become a daemon (so the correct PID
+ *   is recorded), but before daemonization is complete and the parent process
+ *   exits (for synchronization with systemd).
+ * FIXME: Only update the PID file after verifying the PID previously stored
+ *   in the PID file no longer exists or belongs to a foreign process
+ *   in order to ensure the daemon cannot be started more than once.
+ *   (This check is currently done by zed_conf_open_state().)
  */
 int
 zed_conf_write_pid(struct zed_conf *zcp)
diff --git a/cmd/zed/zed_log.c b/cmd/zed/zed_log.c
index bc432bc21..2a787e357 100644
--- a/cmd/zed/zed_log.c
+++ b/cmd/zed/zed_log.c
@@ -25,6 +25,7 @@
  */
 
 #include <assert.h>
+#include <errno.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -40,6 +41,7 @@ static struct {
 	unsigned do_syslog:1;
 	int level;
 	char id[ZED_LOG_MAX_ID_LEN];
+	int pipe_fd[2];
 } _ctx;
 
 void
@@ -53,6 +55,8 @@ zed_log_init(const char *identity)
 	} else {
 		_ctx.id[0] = '\0';
 	}
+	_ctx.pipe_fd[0] = -1;
+	_ctx.pipe_fd[1] = -1;
 }
 
 void
@@ -63,6 +67,97 @@ zed_log_fini()
 	}
 }
 
+/*
+ * Create pipe for communicating daemonization status between the parent and
+ *   child processes across the double-fork().
+ */
+void
+zed_log_pipe_open(void)
+{
+	if ((_ctx.pipe_fd[0] != -1) || (_ctx.pipe_fd[1] != -1))
+		zed_log_die("Invalid use of zed_log_pipe_open in PID %d",
+		    (int) getpid());
+
+	if (pipe(_ctx.pipe_fd) < 0)
+		zed_log_die("Failed to create daemonize pipe in PID %d: %s",
+		    (int) getpid(), strerror(errno));
+}
+
+/*
+ * Close the read-half of the daemonize pipe.
+ * This should be called by the child after fork()ing from the parent since
+ *   the child will never read from this pipe.
+ */
+void
+zed_log_pipe_close_reads(void)
+{
+	if (_ctx.pipe_fd[0] < 0)
+		zed_log_die(
+		    "Invalid use of zed_log_pipe_close_reads in PID %d",
+		    (int) getpid());
+
+	if (close(_ctx.pipe_fd[0]) < 0)
+		zed_log_die(
+		    "Failed to close reads on daemonize pipe in PID %d: %s",
+		    (int) getpid(), strerror(errno));
+
+	_ctx.pipe_fd[0] = -1;
+}
+
+/*
+ * Close the write-half of the daemonize pipe.
+ * This should be called by the parent after fork()ing its child since the
+ *   parent will never write to this pipe.
+ * This should also be called by the child once initialization is complete
+ *   in order to signal the parent that it can safely exit.
+ */
+void
+zed_log_pipe_close_writes(void)
+{
+	if (_ctx.pipe_fd[1] < 0)
+		zed_log_die(
+		    "Invalid use of zed_log_pipe_close_writes in PID %d",
+		    (int) getpid());
+
+	if (close(_ctx.pipe_fd[1]) < 0)
+		zed_log_die(
+		    "Failed to close writes on daemonize pipe in PID %d: %s",
+		    (int) getpid(), strerror(errno));
+
+	_ctx.pipe_fd[1] = -1;
+}
+
+/*
+ * Block on reading from the daemonize pipe until signaled by the child
+ *   (via zed_log_pipe_close_writes()) that initialization is complete.
+ * This should only be called by the parent while waiting to exit after
+ *   fork()ing the child.
+ */
+void
+zed_log_pipe_wait(void)
+{
+	ssize_t n;
+	char c;
+
+	if (_ctx.pipe_fd[0] < 0)
+		zed_log_die("Invalid use of zed_log_pipe_wait in PID %d",
+		    (int) getpid());
+
+	for (;;) {
+		n = read(_ctx.pipe_fd[0], &c, sizeof (c));
+		if (n < 0) {
+			if (errno == EINTR)
+				continue;
+			zed_log_die(
+			    "Failed to read from daemonize pipe in PID %d: %s",
+			    (int) getpid(), strerror(errno));
+		}
+		if (n == 0) {
+			break;
+		}
+	}
+}
+
 void
 zed_log_stderr_open(int level)
 {
diff --git a/cmd/zed/zed_log.h b/cmd/zed/zed_log.h
index 7ae4549fe..767c4c7db 100644
--- a/cmd/zed/zed_log.h
+++ b/cmd/zed/zed_log.h
@@ -33,6 +33,14 @@ void zed_log_init(const char *identity);
 
 void zed_log_fini(void);
 
+void zed_log_pipe_open(void);
+
+void zed_log_pipe_close_reads(void);
+
+void zed_log_pipe_close_writes(void);
+
+void zed_log_pipe_wait(void);
+
 void zed_log_stderr_open(int level);
 
 void zed_log_stderr_close(void);