[PATCH 0/4] mountd/exportd: exit signal handling fixups

[PATCH 0/4] mountd/exportd: exit signal handling fixups

[Benjamin Coddington]

mountd and exportd exit from inside their SIGTERM handler: killer()
unregisters with rpcbind, frees, logs and calls exit(), none of which is
async-signal-safe.  It mostly gets away with it because mountd spends
nearly all its time in select().

Setting rootdir in the [exports] section of nfs.conf makes it much
easier to hit.  nfsd_path_init() then starts a workqueue thread to do
path lookups in the chroot, and it does that after killer() is
installed.  pthread_create() allocates the new thread's TLS with
calloc(), so a SIGTERM there - say, from a reboot while mountd is still
coming up - runs killer() on top of a half-finished malloc.  We hit this
in a reboot test:

  malloc(): unsorted double linked list corrupted
  ...
  calloc
  clnt_vc_create
  local_rpcb
  rpcb_unset
  nfs_svc_unregister
  unregister_services
  killer
  <signal handler called>
  _int_malloc
  calloc
  allocate_dtv
  _dl_allocate_tls
  pthread_create
  main

Once that thread exists malloc takes the arena lock, so the same
re-entry later can deadlock instead of aborting.  Out of 400 SIGTERMs
sent during startup with rootdir set, one mountd hung on a futex, which
I think is that, though I didn't catch a stack.

These patches take HUP, INT and TERM through a signalfd in the cache
loop, so the shutdown work runs in normal context.  The parent of forked
workers keeps a handler, but all it does is kill(0, SIGTERM).  The last
patch holds signals while mountd registers with rpcbind rather than
ignoring them; a SIGTERM there used to just get dropped.

Tested in a private net namespace with its own rpcbind: foreground and
-t 4, HUP, TERM, an ha-callout, and 400 SIGTERMs at random points in
startup with rootdir set.  No aborts or hangs, and nothing dropped.

Benjamin Coddington (4):
  support/export: take SIGINT/SIGTERM/SIGHUP via signalfd
  mountd: stop exiting from a signal handler
  exportd: stop exiting from a signal handler
  mountd: hold SIGTERM during rpcbind registration instead of ignoring
    it

 support/export/cache.c       | 122 ++++++++++++++++++++++++++++++++++-
 support/export/export.h      |   2 +
 support/include/ha-callout.h |   5 +-
 utils/exportd/exportd.c      |  30 +++------
 utils/mountd/mountd.c        |  32 +++------
 utils/mountd/svc_run.c       |   2 +
 6 files changed, 147 insertions(+), 46 deletions(-)

-- 
2.53.0

[PATCH 1/4] support/export: take SIGINT/SIGTERM/SIGHUP via signalfd

[Benjamin Coddington]

mountd and exportd exit from a signal handler that unregisters with
rpcbind, frees memory, logs and calls exit().  None of that is
async-signal-safe.  We have a core from a mountd that took SIGTERM while
pthread_create() was allocating TLS for the nfsd_path workqueue thread;
killer() went on to calloc() in clnt_vc_create() and glibc aborted with
"malloc(): unsorted double linked list corrupted".

Add cache_block_signals() for the daemons to call in place of installing
handlers.  It blocks the three signals and opens a signalfd that
cache_process() reads in its normal loop; cache_stop_signal() tells the
caller when to shut down.  SIGHUP is logged and ignored there as before.

Workers close the signalfd and unblock, so they still die of SIGTERM.
The parent of the workers keeps a handler, but all it does is send
SIGTERM to the process group once.  cache_wait_for_workers() now retries
on EINTR, and the caller's existing "no more workers" path cleans up.

ha_callout() clears the signal mask in its child before exec, since a
blocked mask survives exec.

Assisted-by: LLM
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 support/export/cache.c       | 122 ++++++++++++++++++++++++++++++++++-
 support/export/export.h      |   2 +
 support/include/ha-callout.h |   5 +-
 3 files changed, 127 insertions(+), 2 deletions(-)

diff --git a/support/export/cache.c b/support/export/cache.c
index a5e95afd37ba..7f7f9ad17ef6 100644
--- a/support/export/cache.c
+++ b/support/export/cache.c
@@ -13,10 +13,12 @@
 #include <sys/sysmacros.h>
 #include <sys/types.h>
 #include <sys/select.h>
+#include <sys/signalfd.h>
 #include <sys/stat.h>
 #include <sys/vfs.h>
 #include <sys/wait.h>
 #include <time.h>
+#include <signal.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <unistd.h>
@@ -3880,6 +3882,76 @@ int cache_process_req(fd_set *readfds)
 	return cnt;
 }
 
+/*
+ * SIGHUP, SIGINT and SIGTERM are held blocked from cache_block_signals()
+ * on, so no handler can run while the daemon is in the middle of malloc()
+ * or anything else that isn't async-signal-safe.  A single-process daemon
+ * reads them from cache_sigfd in cache_process().  Worker children get the
+ * default actions back, and the parent of the workers takes them with a
+ * handler that does nothing but pass SIGTERM on to the process group.
+ */
+static int cache_sigfd = -1;
+static volatile sig_atomic_t stop_signal;
+
+static void cache_signal_mask(sigset_t *mask)
+{
+	sigemptyset(mask);
+	sigaddset(mask, SIGHUP);
+	sigaddset(mask, SIGINT);
+	sigaddset(mask, SIGTERM);
+}
+
+/**
+ * cache_block_signals - start taking termination signals via signalfd
+ *
+ * Call this where a daemon would otherwise install its SIGTERM handler.
+ * Once cache_process() has picked up SIGINT or SIGTERM,
+ * cache_stop_signal() returns it and the caller should shut down.
+ */
+void cache_block_signals(void)
+{
+	struct sigaction sa;
+	sigset_t mask;
+
+	cache_signal_mask(&mask);
+	if (sigprocmask(SIG_BLOCK, &mask, NULL) < 0)
+		xlog(L_FATAL, "cannot block signals: %m");
+
+	/* An ignored signal is discarded, not left pending for signalfd */
+	sa.sa_handler = SIG_DFL;
+	sa.sa_flags = 0;
+	sigemptyset(&sa.sa_mask);
+	sigaction(SIGHUP, &sa, NULL);
+	sigaction(SIGINT, &sa, NULL);
+	sigaction(SIGTERM, &sa, NULL);
+
+	cache_sigfd = signalfd(-1, &mask, SFD_NONBLOCK | SFD_CLOEXEC);
+	if (cache_sigfd < 0)
+		xlog(L_FATAL, "cannot create signalfd: %m");
+}
+
+/**
+ * cache_stop_signal - the SIGINT or SIGTERM that asked us to exit, or 0
+ */
+int cache_stop_signal(void)
+{
+	return stop_signal;
+}
+
+static void cache_read_signals(void)
+{
+	struct signalfd_siginfo si;
+
+	while (read(cache_sigfd, &si, sizeof(si)) == sizeof(si)) {
+		if (si.ssi_signo == SIGHUP) {
+			/* don't exit on SIGHUP */
+			xlog(L_NOTICE, "Received SIGHUP... Ignoring.");
+			continue;
+		}
+		stop_signal = si.ssi_signo;
+	}
+}
+
 /**
  * cache_process - process incoming upcalls
  * Returns -ve on error, or number of fds in svc_fds
@@ -3897,6 +3969,8 @@ int cache_process(fd_set *readfds)
 	}
 	cache_set_fds(readfds);
 	v4clients_set_fds(readfds);
+	if (cache_sigfd >= 0)
+		FD_SET(cache_sigfd, readfds);
 
 	if (delayed || delayed_expkey || delayed_export) {
 		time_t now = time(NULL);
@@ -3938,6 +4012,11 @@ int cache_process(fd_set *readfds)
 		return -1;
 
 	default:
+		if (cache_sigfd >= 0 && FD_ISSET(cache_sigfd, readfds)) {
+			cache_read_signals();
+			FD_CLR(cache_sigfd, readfds);
+			selret--;
+		}
 		selret -= cache_process_req(readfds);
 		selret -= v4clients_process(readfds);
 		if (selret < 0)
@@ -4100,6 +4179,8 @@ cache_wait_for_workers(char *prog)
 		if (pid < 0) {
 			if (errno == ECHILD)
 				return; /* no more children */
+			if (errno == EINTR)
+				continue; /* cache_parent_signal() */
 			xlog(L_FATAL, "%s: can't wait: %s\n", prog,
 					strerror(errno));
 		}
@@ -4137,10 +4218,26 @@ static struct nl_sock *nl_cmd_sock_reopen(struct nl_sock *old)
 	return sock;
 }
 
+/*
+ * SIGINT/SIGTERM handler for the parent of the workers.  Only
+ * async-signal-safe calls here: the workers die of the SIGTERM,
+ * cache_wait_for_workers() returns, and the caller cleans up.
+ */
+static void cache_parent_signal(int sig)
+{
+	if (!stop_signal) {
+		stop_signal = sig;
+		/* play Kronos and eat our children */
+		kill(0, SIGTERM);
+	}
+}
+
 /* Fork num_threads worker children and wait for them */
 int
 cache_fork_workers(char *prog, int num_threads)
 {
+	struct sigaction sa;
+	sigset_t mask;
 	int i;
 	pid_t pid;
 
@@ -4184,7 +4281,6 @@ cache_fork_workers(char *prog, int num_threads)
 			 * so that workers die naturally when sent them.
 			 * Only the parent unregisters with pmap and
 			 * hence needs to do special SIGTERM handling. */
-			struct sigaction sa;
 			sa.sa_handler = SIG_DFL;
 			sa.sa_flags = 0;
 			sigemptyset(&sa.sa_mask);
@@ -4192,12 +4288,36 @@ cache_fork_workers(char *prog, int num_threads)
 			sigaction(SIGINT, &sa, NULL);
 			sigaction(SIGTERM, &sa, NULL);
 
+			if (cache_sigfd >= 0) {
+				close(cache_sigfd);
+				cache_sigfd = -1;
+			}
+			cache_signal_mask(&mask);
+			sigprocmask(SIG_UNBLOCK, &mask, NULL);
+
 			/* fall into my_svc_run in caller */
 			return 1;
 		}
 	}
 
 	/* in parent */
+	if (cache_sigfd >= 0) {
+		close(cache_sigfd);
+		cache_sigfd = -1;
+	}
+	sa.sa_handler = cache_parent_signal;
+	sa.sa_flags = 0;
+	sigemptyset(&sa.sa_mask);
+	sigaction(SIGINT, &sa, NULL);
+	sigaction(SIGTERM, &sa, NULL);
+	sa.sa_handler = SIG_IGN;
+	sigaction(SIGHUP, &sa, NULL);
+	cache_signal_mask(&mask);
+	sigprocmask(SIG_UNBLOCK, &mask, NULL);
+
 	cache_wait_for_workers(prog);
+	if (stop_signal)
+		xlog(L_NOTICE, "%s: caught signal %d, workers stopped",
+		     prog, stop_signal);
 	return 0;
 }
diff --git a/support/export/export.h b/support/export/export.h
index e2009ccdc443..801fb01cbd89 100644
--- a/support/export/export.h
+++ b/support/export/export.h
@@ -32,6 +32,8 @@ int		cache_export(nfs_export *exp, char *path);
 int		cache_fork_workers(char *prog, int num_threads);
 void		cache_wait_for_workers(char *prog);
 int		cache_process(fd_set *readfds);
+void		cache_block_signals(void);
+int		cache_stop_signal(void);
 
 bool ipaddr_client_matches(nfs_export *exp, struct addrinfo *ai);
 bool namelist_client_matches(nfs_export *exp, char *dom);
diff --git a/support/include/ha-callout.h b/support/include/ha-callout.h
index a454bdbdb708..bb2ccfd93470 100644
--- a/support/include/ha-callout.h
+++ b/support/include/ha-callout.h
@@ -42,7 +42,10 @@ ha_callout(char *event, char *arg1, char *arg2, int arg3)
 	sigaction(SIGCHLD, &newact, &oldact);
 	pid = fork();
 	switch (pid) {
-		case 0: execl(ha_callout_prog, ha_callout_prog,
+		case 0: /* mountd keeps SIGTERM et al blocked for signalfd */
+			sigemptyset(&newact.sa_mask);
+			sigprocmask(SIG_SETMASK, &newact.sa_mask, NULL);
+			execl(ha_callout_prog, ha_callout_prog,
 				event, arg1, arg2, 
 			      arg3 < 0 ? NULL : buf,
 			      NULL);
-- 
2.53.0

[PATCH 2/4] mountd: stop exiting from a signal handler

[Benjamin Coddington]

Use cache_block_signals() instead of installing killer() and sig_hup().
my_svc_run() returns once a stop signal is seen and main() calls
killer() from normal context.  The worker handling in killer() is gone;
cache_fork_workers() does that now.

Assisted-by: LLM
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 utils/mountd/mountd.c  | 25 ++++++-------------------
 utils/mountd/svc_run.c |  2 ++
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 92d8c4690efc..bdbff1dda0b7 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -124,17 +124,13 @@ cleanup_lockfiles (void)
 }
 
 /*
- * Signal handler.
+ * Called from main() once my_svc_run() has seen SIGINT or SIGTERM,
+ * never from a signal handler: see cache_block_signals().
  */
 static void
 killer (int sig)
 {
 	unregister_services();
-	if (num_threads > 1) {
-		/* play Kronos and eat our children */
-		kill(0, SIGTERM);
-		cache_wait_for_workers("mountd");
-	}
 	cleanup_lockfiles();
 	free_state_path_names(&etab);
 	free_state_path_names(&rmtab);
@@ -142,14 +138,6 @@ killer (int sig)
 	exit(0);
 }
 
-static void
-sig_hup (int UNUSED(sig))
-{
-	/* don't exit on SIGHUP */
-	xlog (L_NOTICE, "Received SIGHUP... Ignoring.\n");
-	return;
-}
-
 bool_t
 mount_null_1_svc(struct svc_req *rqstp, void *UNUSED(argp),
 	void *UNUSED(resp))
@@ -883,11 +871,7 @@ main(int argc, char **argv)
 	if (version23() && listeners == 0)
 		xlog(L_WARNING, "mountd: No V2 or V3 listeners created!");
 
-	sa.sa_handler = killer;
-	sigaction(SIGINT, &sa, NULL);
-	sigaction(SIGTERM, &sa, NULL);
-	sa.sa_handler = sig_hup;
-	sigaction(SIGHUP, &sa, NULL);
+	cache_block_signals();
 
 	if (!foreground) {
 		/* We first fork off a child. */
@@ -939,6 +923,9 @@ main(int argc, char **argv)
 	xlog(L_NOTICE, "Version " VERSION " starting");
 	my_svc_run();
 
+	if (cache_stop_signal())
+		killer(cache_stop_signal());
+
 	xlog(L_ERROR, "RPC service loop terminated unexpectedly. Exiting...\n");
 	unregister_services();
 	free_state_path_names(&etab);
diff --git a/utils/mountd/svc_run.c b/utils/mountd/svc_run.c
index 2aaf3756bbb1..259d03b2f031 100644
--- a/utils/mountd/svc_run.c
+++ b/utils/mountd/svc_run.c
@@ -105,5 +105,7 @@ my_svc_run(void)
 		}
 		if (selret)
 			svc_getreqset(&readfds);
+		if (cache_stop_signal())
+			return;
 	}
 }
-- 
2.53.0

[PATCH 3/4] exportd: stop exiting from a signal handler

[Benjamin Coddington]

Same change as for mountd: use cache_block_signals() and call killer()
from the process loop once cache_stop_signal() is set.

Assisted-by: LLM
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 utils/exportd/exportd.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/utils/exportd/exportd.c b/utils/exportd/exportd.c
index a08aaaccbc2f..d63195c4ea28 100644
--- a/utils/exportd/exportd.c
+++ b/utils/exportd/exportd.c
@@ -61,14 +61,13 @@ cleanup_lockfiles (void)
 	unlink(etab.lockfn);
 }
 
+/*
+ * Called from main() once cache_process() has seen SIGINT or SIGTERM,
+ * never from a signal handler: see cache_block_signals().
+ */
 static void
 killer (int sig)
 {
-	if (num_threads > 1) {
-		/* play Kronos and eat our children */
-		kill(0, SIGTERM);
-		cache_wait_for_workers("exportd");
-	}
 	cleanup_lockfiles();
 	free_state_path_names(&etab);
 	xlog (L_NOTICE, "Caught signal %d, exiting.", sig);
@@ -76,14 +75,6 @@ killer (int sig)
 	exit(0);
 }
 
-static void
-sig_hup (int UNUSED(sig))
-{
-	/* don't exit on SIGHUP */
-	xlog (L_NOTICE, "Received SIGHUP... Ignoring.\n");
-	return;
-}
-
 inline static void
 set_signals(void)
 {
@@ -96,12 +87,7 @@ set_signals(void)
 	/* WARNING: the following works on Linux and SysV, but not BSD! */
 	sigaction(SIGCHLD, &sa, NULL);
 
-	sa.sa_handler = killer;
-	sigaction(SIGINT, &sa, NULL);
-	sigaction(SIGTERM, &sa, NULL);
-
-	sa.sa_handler = sig_hup;
-	sigaction(SIGHUP, &sa, NULL);
+	cache_block_signals();
 }
 
 static void
@@ -242,8 +228,10 @@ main(int argc, char **argv)
 	v4clients_init();
 
 	/* Process incoming upcalls */
-	while (cache_process(NULL) >= 0)
-		;
+	while (cache_process(NULL) >= 0) {
+		if (cache_stop_signal())
+			killer(cache_stop_signal());
+	}
 
 	xlog(L_ERROR, "%s: process loop terminated unexpectedly(%m). Exiting...\n",
 		progname);
-- 
2.53.0

[PATCH 4/4] mountd: hold SIGTERM during rpcbind registration instead of ignoring it

[Benjamin Coddington]

mountd ignores HUP, INT and TERM from just before it unregisters and
re-registers with rpcbind until it installs its real handling.  A SIGTERM
in that window is simply lost, and mountd carries on as if nothing
happened.  With a local rpcbind, about half of the SIGTERMs sent in the
first 30ms of startup were dropped this way.

Call cache_block_signals() there instead of ignoring them.  A signal that
arrives during registration now stays pending until my_svc_run(), which
unregisters and exits.

Assisted-by: LLM
Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
---
 utils/mountd/mountd.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index bdbff1dda0b7..5c2d5ddf2577 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -851,13 +851,14 @@ main(int argc, char **argv)
 	sa.sa_handler = SIG_IGN;
 	sa.sa_flags = 0;
 	sigemptyset(&sa.sa_mask);
-	sigaction(SIGHUP, &sa, NULL);
-	sigaction(SIGINT, &sa, NULL);
-	sigaction(SIGTERM, &sa, NULL);
 	sigaction(SIGPIPE, &sa, NULL);
 	/* WARNING: the following works on Linux and SysV, but not BSD! */
 	sigaction(SIGCHLD, &sa, NULL);
 
+	/* Hold HUP, INT and TERM until my_svc_run() rather than dropping
+	 * them while we register with rpcbind */
+	cache_block_signals();
+
 	unregister_services();
 	if (version2()) {
 		listeners += nfs_svc_create("mountd", MOUNTPROG,
@@ -871,8 +872,6 @@ main(int argc, char **argv)
 	if (version23() && listeners == 0)
 		xlog(L_WARNING, "mountd: No V2 or V3 listeners created!");
 
-	cache_block_signals();
-
 	if (!foreground) {
 		/* We first fork off a child. */
 		if ((c = fork()) > 0)
-- 
2.53.0