From 3b1e6d651d3e11c00fb90760558ea8193b4d460a Mon Sep 17 00:00:00 2001
From: Patrick Mooney <pmooney@pfmooney.com>
Date: Fri, 5 Aug 2022 23:25:45 +0000
Subject: [PATCH 1/9] 14892 pollhead lifetime too short in signalfd

---
 usr/src/uts/common/io/signalfd.c | 867 ++++++++++++++++---------------
 1 file changed, 462 insertions(+), 405 deletions(-)

diff --git a/usr/src/uts/common/io/signalfd.c b/usr/src/uts/common/io/signalfd.c
index 46d616dd79..cee211cb21 100644
--- a/usr/src/uts/common/io/signalfd.c
+++ b/usr/src/uts/common/io/signalfd.c
@@ -11,6 +11,7 @@
 
 /*
  * Copyright 2017 Joyent, Inc.
+ * Copyright 2022 Oxide Computer Company
  */
 
 /*
@@ -40,33 +41,36 @@
  * When a thread polling on a signalfd requires a pollhead, several steps must
  * be taken to safely ensure the proper result.  A sigfd_proc_state_t is
  * created for the calling process if it does not yet exist.  It is there where
- * a list of sigfd_poll_waiter_t structures reside which associate pollheads to
- * signalfd_state_t entries.  The sigfd_proc_state_t list is walked to find a
- * sigfd_poll_waiter_t matching the signalfd_state_t which corresponds to the
- * polled resource.  If one is found, it is reused.  Otherwise a new one is
- * created, incrementing the refcount on the signalfd_state_t, and it is added
- * to the sigfd_poll_waiter_t list.
+ * a list of signalfd_poller_t structures reside which associate pollheads to
+ * signalfd_state_t entries.  The sigfd_proc_state_t list is walked to find any
+ * signalfd_poller_t which is both associated with the polling process and
+ * corresponds to the signalfd resource being polled.  If none matching those
+ * conditions is found, then a new one with the appropriate associations is
+ * created.
  *
  * The complications imposed by fork(2) are why the pollhead is stored in the
- * associated sigfd_poll_waiter_t instead of directly in the signalfd_state_t.
+ * associated signalfd_poller_t instead of directly in the signalfd_state_t.
  * More than one process can hold a reference to the signalfd at a time but
  * arriving signals should wake only process-local pollers.  Additionally,
  * signalfd_close is called only when the last referencing fd is closed, hiding
  * occurrences of preceeding threads which released their references.  This
- * necessitates reference counting on the signalfd_state_t so it is able to
- * persist after close until all poll references have been cleansed.  Doing so
- * ensures that blocked pollers which hold references to the signalfd_state_t
- * will be able to do clean-up after the descriptor itself has been closed.
+ * necessitates a pollhead for each signalfd/process pair when being polled.
+ * Doing so ensures that those pollheads will live long enough for the greater
+ * poll machinery can act upon them without risk of use-after-free.  When a
+ * signalfd is closed, existing signalfd_poller_t instances are dissociated from
+ * their respective processes, causing pollwake() calls for any blocked pollers.
  *
  * When a signal arrives in a process polling on signalfd, signalfd_pollwake_cb
  * is called via the pointer in sigfd_proc_state_t.  It will walk over the
- * sigfd_poll_waiter_t entries present in the list, searching for any
- * associated with a signalfd_state_t with a matching signal mask.  The
- * approach of keeping the poller list in p_sigfd was chosen because a process
- * is likely to use few signalfds relative to its total file descriptors.  It
- * reduces the work required for each received signal.
+ * signalfd_poller_t entries present in the list, searching for any possessing a
+ * signal mask which matches the incoming signal.  (Changes to the signal mask
+ * held in signalfd_state_t is propagated to the signalfd_poller_t instance to
+ * avoid the need for additional locks during the callback.) The approach of
+ * keeping the poller list in p_sigfd was chosen because a process is likely to
+ * use few signalfds relative to its total file descriptors.  It reduces the
+ * work required for each received signal.
  *
- * When matching sigfd_poll_waiter_t entries are encountered in the poller list
+ * When matching signalfd_poller_t entries are encountered in the poller list
  * during signalfd_pollwake_cb, they are dispatched into signalfd_wakeq to
  * perform the pollwake.  This is due to a lock ordering conflict between
  * signalfd_poll and signalfd_pollwake_cb.  The former acquires
@@ -74,9 +78,13 @@
  * reverses the order.  Defering the pollwake into a taskq means it can be
  * performed without proc_t`p_lock held, avoiding the deadlock.
  *
- * The sigfd_list is self-cleaning; as signalfd_pollwake_cb is called, the list
- * will clear out on its own.  Any remaining per-process state which remains
- * will be cleaned up by the exit helper (signalfd_exit_helper).
+ * Poller entries in sigfd_proc_state_t`sigfd_list are cleaned up under two
+ * different circumstances.  When a signalfd instance is being closed, it will
+ * dissociate all of its remaining signalfd_poller_t instances from their
+ * polling processes.  When a process (which polled on signalfd instance(s)
+ * which have not yet been closed) exits, the exit helper (signalfd_exit_helper)
+ * is called, and it dissociates all signalfd_poller_t instances tied to the
+ * existing process.
  *
  * The structures associated with signalfd state are designed to operate
  * correctly across fork, but there is one caveat that applies.  Using
@@ -88,12 +96,28 @@
  *
  * Lock ordering:
  *
- * 1. signalfd_lock
+ * Calling signalfd_poll:
+ * 1. pollcache_t`pc_lock
  * 2. signalfd_state_t`sfd_lock
+ * 3. proc_t`p_lock
+ *
+ * Signal delivery, waking a pollhead:
+ * 1. proc_t`p_lock
+ * 2. signalfd_poller_t`sp_lock
+ *
+ * Process exit, cleaning up signalfd pollers:
+ * 1. proc_t`p_lock
+ * 2. signalfd_poller_t`sp_lock
+ *
+ * Waking a pollhead, from taskq:
+ * 1. signalfd_poller_t`sp_lock
+ * 2. pollcache_t`pc_lock
+ *
+ * Closing signalfd, dissociating pollers:
+ * 1. signalfd_state_t`sfd_lock
+ * 2. pidlock
+ * 3. proc_t`p_lock
  *
- * 1. proc_t`p_lock (to walk p_sigfd)
- * 2. signalfd_state_t`sfd_lock
- * 2a. signalfd_lock (after sfd_lock is dropped, when sfd_count falls to 0)
  */
 
 #include <sys/ddi.h>
@@ -109,266 +133,343 @@
 #include <sys/sdt.h>
 #include <sys/disp.h>
 #include <sys/taskq_impl.h>
+#include <sys/condvar.h>
+#include <sys/stdbool.h>
 
-typedef struct signalfd_state signalfd_state_t;
+typedef struct signalfd_state {
+	kmutex_t	sfd_lock;	/* protects fields below */
+	list_t		sfd_pollers;
+	k_sigset_t	sfd_mask;	/* signal mask for this instance */
+	minor_t		sfd_minor;	/* dev minor, fixed at creation */
+} signalfd_state_t;
 
-struct signalfd_state {
-	list_node_t	sfd_list;		/* node in global list */
-	kmutex_t	sfd_lock;		/* protects fields below */
-	uint_t		sfd_count;		/* ref count */
-	boolean_t	sfd_valid;		/* valid while open */
-	k_sigset_t	sfd_set;		/* signals for this fd */
-};
+typedef struct signalfd_poller {
+	/*
+	 * List node referenced by containing signalfd_state_t
+	 * Protected by signalfd_state`sfd_lock
+	 */
+	list_node_t	sp_state_node;
 
-typedef struct sigfd_poll_waiter {
-	list_node_t		spw_list;
-	signalfd_state_t	*spw_state;
-	pollhead_t		spw_pollhd;
-	taskq_ent_t		spw_taskent;
-	short			spw_pollev;
-} sigfd_poll_waiter_t;
+	/*
+	 * List node referenced by containing sigfd_proc_state_t
+	 * Protected by proc_t`plock
+	 */
+	list_node_t	sp_proc_node;
+
+	pollhead_t	sp_pollhead;
+
+	/*
+	 * The signalfd_state_t to which this poller is associated.
+	 * It remains fixed after its initialization at creation time.
+	 */
+	signalfd_state_t	*sp_state;
+
+	/*
+	 * The proc_t to which this poller is associated.
+	 * It is initialized under the protection of proc_t`p_lock when this
+	 * poller is created.  It is NULLed out, again under the protection of
+	 * proc_t`p_lock, when the poller is dissociated from the process.
+	 */
+	proc_t		*sp_proc;
+
+	kmutex_t	sp_lock;	/* protects fields below */
+	kcondvar_t	sp_cv;		/* CV for cleaning up */
+	taskq_ent_t	sp_taskent;	/* pollwakeup() dispatch taskq */
+	k_sigset_t	sp_mask;	/* signal match mask */
+	short		sp_pollev;	/* Event(s) pending delivery */
+} signalfd_poller_t;
 
-/*
- * Protects global state in signalfd_devi, signalfd_minor, signalfd_softstate,
- * and signalfd_state (including sfd_list field of members)
- */
-static kmutex_t		signalfd_lock;
 static dev_info_t	*signalfd_devi;		/* device info */
-static id_space_t	*signalfd_minor;	/* minor number arena */
+static id_space_t	*signalfd_minors;	/* minor number arena */
 static void		*signalfd_softstate;	/* softstate pointer */
-static list_t		signalfd_state;		/* global list of state */
 static taskq_t		*signalfd_wakeq;	/* pollwake event taskq */
 
-
 static void
-signalfd_state_enter_locked(signalfd_state_t *state)
+signalfd_proc_clean(proc_t *p)
 {
-	ASSERT(MUTEX_HELD(&state->sfd_lock));
-	ASSERT(state->sfd_count > 0);
-	VERIFY(state->sfd_valid == B_TRUE);
+	ASSERT(MUTEX_HELD(&p->p_lock));
+
+	sigfd_proc_state_t *pstate = p->p_sigfd;
+	if (pstate != NULL) {
+		ASSERT(list_is_empty(&pstate->sigfd_list));
 
-	state->sfd_count++;
+		p->p_sigfd = NULL;
+		list_destroy(&pstate->sigfd_list);
+		kmem_free(pstate, sizeof (*pstate));
+	}
 }
 
 static void
-signalfd_state_release(signalfd_state_t *state, boolean_t force_invalidate)
+signalfd_wake_task(void *arg)
 {
-	mutex_enter(&state->sfd_lock);
+	signalfd_poller_t *sp = arg;
 
-	if (force_invalidate) {
-		state->sfd_valid = B_FALSE;
+	mutex_enter(&sp->sp_lock);
+	VERIFY(sp->sp_pollev != 0);
+	pollwakeup(&sp->sp_pollhead, sp->sp_pollev);
+	if ((sp->sp_pollev & POLLERR) != 0) {
+		pollhead_clean(&sp->sp_pollhead);
 	}
+	sp->sp_pollev = 0;
 
-	ASSERT(state->sfd_count > 0);
-	if (state->sfd_count == 1) {
-		VERIFY(state->sfd_valid == B_FALSE);
-		mutex_exit(&state->sfd_lock);
-		if (force_invalidate) {
-			/*
-			 * The invalidation performed in signalfd_close is done
-			 * while signalfd_lock is held.
-			 */
-			ASSERT(MUTEX_HELD(&signalfd_lock));
-			list_remove(&signalfd_state, state);
-		} else {
-			ASSERT(MUTEX_NOT_HELD(&signalfd_lock));
-			mutex_enter(&signalfd_lock);
-			list_remove(&signalfd_state, state);
-			mutex_exit(&signalfd_lock);
-		}
-		kmem_free(state, sizeof (*state));
-		return;
-	}
-	state->sfd_count--;
-	mutex_exit(&state->sfd_lock);
+	/*
+	 * If this poller is being torn down, wake any thread waiting for the
+	 * event delivery to finish.
+	 */
+	cv_signal(&sp->sp_cv);
+	mutex_exit(&sp->sp_lock);
 }
 
-static sigfd_poll_waiter_t *
-signalfd_wake_list_add(sigfd_proc_state_t *pstate, signalfd_state_t *state)
+static void
+signalfd_poller_wake(signalfd_poller_t *sp, short ev)
 {
-	list_t *lst = &pstate->sigfd_list;
-	sigfd_poll_waiter_t *pw;
-
-	for (pw = list_head(lst); pw != NULL; pw = list_next(lst, pw)) {
-		if (pw->spw_state == state)
-			break;
-	}
+	ASSERT(MUTEX_HELD(&sp->sp_lock));
 
-	if (pw == NULL) {
-		pw = kmem_zalloc(sizeof (*pw), KM_SLEEP);
+	const bool pending = (sp->sp_pollev != 0);
+	sp->sp_pollev |= ev;
 
-		mutex_enter(&state->sfd_lock);
-		signalfd_state_enter_locked(state);
-		pw->spw_state = state;
-		mutex_exit(&state->sfd_lock);
-		list_insert_head(lst, pw);
+	if (!pending) {
+		taskq_dispatch_ent(signalfd_wakeq, signalfd_wake_task, sp, 0,
+		    &sp->sp_taskent);
 	}
-	return (pw);
 }
 
-static sigfd_poll_waiter_t *
-signalfd_wake_list_rm(sigfd_proc_state_t *pstate, signalfd_state_t *state)
+static void
+signalfd_pollwake_cb(void *arg0, int sig)
 {
-	list_t *lst = &pstate->sigfd_list;
-	sigfd_poll_waiter_t *pw;
+	proc_t *p = (proc_t *)arg0;
+	sigfd_proc_state_t *pstate = (sigfd_proc_state_t *)p->p_sigfd;
 
-	for (pw = list_head(lst); pw != NULL; pw = list_next(lst, pw)) {
-		if (pw->spw_state == state) {
-			break;
+	ASSERT(MUTEX_HELD(&p->p_lock));
+	ASSERT(pstate != NULL);
+
+	list_t *pollers = &pstate->sigfd_list;
+	for (signalfd_poller_t *sp = list_head(pollers); sp != NULL;
+	    sp = list_next(pollers, sp)) {
+		mutex_enter(&sp->sp_lock);
+		if (sigismember(&sp->sp_mask, sig)) {
+			signalfd_poller_wake(sp, POLLRDNORM | POLLIN);
 		}
+		mutex_exit(&sp->sp_lock);
 	}
+}
+
+/*
+ * Get the sigfd_proc_state_t for a given process, allocating one if necessary.
+ *
+ * Must be called with p_lock held, which may be dropped and reacquired during
+ * the allocation.
+ */
+static sigfd_proc_state_t *
+signalfd_proc_pstate(proc_t *p)
+{
+	ASSERT(MUTEX_HELD(&p->p_lock));
 
-	if (pw != NULL) {
-		list_remove(lst, pw);
-		pw->spw_state = NULL;
-		signalfd_state_release(state, B_FALSE);
+	sigfd_proc_state_t *pstate = p->p_sigfd;
+	if (pstate == NULL) {
+		mutex_exit(&p->p_lock);
+		pstate = kmem_zalloc(sizeof (*pstate), KM_SLEEP);
+		list_create(&pstate->sigfd_list,
+		    sizeof (signalfd_poller_t),
+		    offsetof(signalfd_poller_t, sp_proc_node));
+		pstate->sigfd_pollwake_cb = signalfd_pollwake_cb;
+
+		/* Check again, after blocking for the alloc. */
+		mutex_enter(&p->p_lock);
+		if (p->p_sigfd == NULL) {
+			p->p_sigfd = pstate;
+		} else {
+			/* Someone beat us to it */
+			list_destroy(&pstate->sigfd_list);
+			kmem_free(pstate, sizeof (*pstate));
+			pstate = p->p_sigfd;
+		}
 	}
 
-	return (pw);
+	return (pstate);
 }
 
-static void
-signalfd_wake_list_cleanup(proc_t *p)
+static signalfd_poller_t *
+signalfd_poller_associate(signalfd_state_t *state, proc_t *p)
 {
-	sigfd_proc_state_t *pstate = p->p_sigfd;
-	sigfd_poll_waiter_t *pw;
-	list_t *lst;
+	sigfd_proc_state_t *pstate;
+	list_t *pollers;
+	signalfd_poller_t *sp;
 
-	ASSERT(MUTEX_HELD(&p->p_lock));
-	ASSERT(pstate != NULL);
+	ASSERT(MUTEX_HELD(&state->sfd_lock));
 
-	lst = &pstate->sigfd_list;
-	while ((pw = list_remove_head(lst)) != NULL) {
-		signalfd_state_t *state = pw->spw_state;
+	mutex_enter(&p->p_lock);
 
-		pw->spw_state = NULL;
-		signalfd_state_release(state, B_FALSE);
+	pstate = signalfd_proc_pstate(p);
+	pollers = &pstate->sigfd_list;
 
-		pollwakeup(&pw->spw_pollhd, POLLERR);
-		pollhead_clean(&pw->spw_pollhd);
-		kmem_free(pw, sizeof (*pw));
+	/*
+	 * Check if there is already a signalfd_poller_t allocated for this
+	 * signalfd_state_t/proc_t pair.
+	 */
+	for (sp = list_head(pollers); sp != NULL; sp = list_next(pollers, sp)) {
+		if (sp->sp_state == state) {
+			mutex_exit(&p->p_lock);
+			return (sp);
+		}
 	}
-	list_destroy(lst);
 
-	p->p_sigfd = NULL;
-	kmem_free(pstate, sizeof (*pstate));
-}
+	/*
+	 * No existing poller found, so allocate one. Since sfd_lock remains
+	 * held, there is no risk of some other operation racing with us to
+	 * create such a poller.
+	 */
+	mutex_exit(&p->p_lock);
 
-static void
-signalfd_exit_helper(void)
-{
-	proc_t *p = curproc;
+	sp = kmem_zalloc(sizeof (*sp), KM_SLEEP);
+	mutex_init(&sp->sp_lock, NULL, MUTEX_DEFAULT, NULL);
+	cv_init(&sp->sp_cv, NULL, CV_DEFAULT, NULL);
+	sigorset(&sp->sp_mask, &state->sfd_mask);
+	sp->sp_state = state;
+	sp->sp_proc = p;
 
 	mutex_enter(&p->p_lock);
-	signalfd_wake_list_cleanup(p);
+	/*
+	 * Fetch the pstate again, since it could have been freed or reallocated
+	 * in the time p_lock was dropped.
+	 */
+	pstate = signalfd_proc_pstate(p);
+
+	list_insert_tail(&pstate->sigfd_list, sp);
+	list_insert_tail(&state->sfd_pollers, sp);
 	mutex_exit(&p->p_lock);
+
+	return (sp);
 }
 
-/*
- * Perform pollwake for a sigfd_poll_waiter_t entry.
- * Thanks to the strict and conflicting lock orders required for signalfd_poll
- * (pc_lock before p_lock) and signalfd_pollwake_cb (p_lock before pc_lock),
- * this is relegated to a taskq to avoid deadlock.
- */
 static void
-signalfd_wake_task(void *arg)
+signalfd_pollers_dissociate(signalfd_state_t *state)
 {
-	sigfd_poll_waiter_t *pw = arg;
-	signalfd_state_t *state = pw->spw_state;
-
-	pw->spw_state = NULL;
-	signalfd_state_release(state, B_FALSE);
-	pollwakeup(&pw->spw_pollhd, pw->spw_pollev);
-	pollhead_clean(&pw->spw_pollhd);
-	kmem_free(pw, sizeof (*pw));
+	ASSERT(MUTEX_HELD(&state->sfd_lock));
+
+	mutex_enter(&pidlock);
+
+	signalfd_poller_t *sp;
+	list_t *pollers = &state->sfd_pollers;
+	for (sp = list_head(pollers); sp != NULL; sp = list_next(pollers, sp)) {
+		proc_t *p = sp->sp_proc;
+
+		if (p == NULL) {
+			continue;
+		}
+
+		/*
+		 * Even if the process in question is racing us to clean-up in
+		 * proc_exit(), it will be unable to exit (and free itself)
+		 * since we hold pidlock.  This prevents us from otherwise
+		 * attempting to lock a p_lock which was freed.
+		 */
+		mutex_enter(&p->p_lock);
+		if (sp->sp_proc == NULL) {
+			mutex_exit(&p->p_lock);
+			continue;
+		}
+		VERIFY3P(sp->sp_proc, ==, p);
+		VERIFY3P(sp->sp_state, ==, state);
+		VERIFY3P(p->p_sigfd, !=, NULL);
+
+		sigfd_proc_state_t *pstate = p->p_sigfd;
+		list_remove(&pstate->sigfd_list, sp);
+		sp->sp_proc = NULL;
+
+		/* Wake any lingering pollers referencing the pollhead */
+		mutex_enter(&sp->sp_lock);
+		signalfd_poller_wake(sp, POLLERR);
+		mutex_exit(&sp->sp_lock);
+
+		signalfd_proc_clean(p);
+		mutex_exit(&p->p_lock);
+	}
+	mutex_exit(&pidlock);
 }
 
-/*
- * Called every time a signal is delivered to the process so that we can
- * see if any signal stream needs a pollwakeup. We maintain a list of
- * signal state elements so that we don't have to look at every file descriptor
- * on the process. If necessary, a further optimization would be to maintain a
- * signal set mask that is a union of all of the sets in the list so that
- * we don't even traverse the list if the signal is not in one of the elements.
- * However, since the list is likely to be very short, this is not currently
- * being done. A more complex data structure might also be used, but it is
- * unclear what that would be since each signal set needs to be checked for a
- * match.
- */
 static void
-signalfd_pollwake_cb(void *arg0, int sig)
+signalfd_pollers_free(signalfd_state_t *state)
 {
-	proc_t *p = (proc_t *)arg0;
-	sigfd_proc_state_t *pstate = (sigfd_proc_state_t *)p->p_sigfd;
-	list_t *lst;
-	sigfd_poll_waiter_t *pw;
+	ASSERT(MUTEX_HELD(&state->sfd_lock));
 
-	ASSERT(MUTEX_HELD(&p->p_lock));
-	ASSERT(pstate != NULL);
+	signalfd_poller_t *sp;
+	while ((sp = list_remove_head(&state->sfd_pollers)) != NULL) {
+		ASSERT3P(sp->sp_proc, ==, NULL);
 
-	lst = &pstate->sigfd_list;
-	pw = list_head(lst);
-	while (pw != NULL) {
-		signalfd_state_t *state = pw->spw_state;
-		sigfd_poll_waiter_t *next;
-
-		mutex_enter(&state->sfd_lock);
-		if (!state->sfd_valid) {
-			pw->spw_pollev = POLLERR;
-		} else if (sigismember(&state->sfd_set, sig)) {
-			pw->spw_pollev = POLLRDNORM | POLLIN;
-		} else {
-			mutex_exit(&state->sfd_lock);
-			pw = list_next(lst, pw);
-			continue;
+		mutex_enter(&sp->sp_lock);
+		while (sp->sp_pollev != 0) {
+			cv_wait(&sp->sp_cv, &sp->sp_lock);
 		}
-		mutex_exit(&state->sfd_lock);
+		/*
+		 * With the poller dissociated from its polling process, and any
+		 * lingering events delivered, the pollhead should be empty.
+		 */
+		ASSERT3P(sp->sp_pollhead.ph_list, ==, NULL);
+
+		cv_destroy(&sp->sp_cv);
+		mutex_destroy(&sp->sp_lock);
+		kmem_free(sp, sizeof (*sp));
+	}
+}
 
+static void
+signalfd_exit_helper(void)
+{
+	proc_t *p = curproc;
+
+	mutex_enter(&p->p_lock);
+
+	sigfd_proc_state_t *pstate = p->p_sigfd;
+	if (pstate == NULL) {
+		mutex_exit(&p->p_lock);
+		return;
+	}
+
+	signalfd_poller_t *sp;
+	while ((sp = list_remove_head(&pstate->sigfd_list)) != NULL) {
 		/*
-		 * Pull the sigfd_poll_waiter_t out of the list and dispatch it
-		 * to perform a pollwake.  This cannot be done synchronously
-		 * since signalfd_poll and signalfd_pollwake_cb have
-		 * conflicting lock orders which can deadlock.
+		 * Having been removed from the sigfd_list, make it clear that
+		 * this signalfd_poller_t is disssociated from the process.
 		 */
-		next = list_next(lst, pw);
-		list_remove(lst, pw);
-		taskq_dispatch_ent(signalfd_wakeq, signalfd_wake_task, pw, 0,
-		    &pw->spw_taskent);
-		pw = next;
+		sp->sp_proc = NULL;
+
+		/* Wake any lingering pollers referencing the pollhead */
+		mutex_enter(&sp->sp_lock);
+		signalfd_poller_wake(sp, POLLERR);
+		mutex_exit(&sp->sp_lock);
 	}
+	signalfd_proc_clean(p);
+	mutex_exit(&p->p_lock);
 }
 
 _NOTE(ARGSUSED(1))
 static int
-signalfd_open(dev_t *devp, int flag, int otyp, cred_t *cred_p)
+signalfd_open(dev_t *devp, int flag, int otyp, cred_t *cr)
 {
-	signalfd_state_t *state, **sstate;
-	major_t major = getemajor(*devp);
-	minor_t minor = getminor(*devp);
-
-	if (minor != SIGNALFDMNRN_SIGNALFD)
+	if (getminor(*devp) != SIGNALFDMNRN_SIGNALFD) {
 		return (ENXIO);
+	}
 
-	mutex_enter(&signalfd_lock);
+	const minor_t minor = (minor_t)id_allocff_nosleep(signalfd_minors);
+	if (minor == -1) {
+		return (ENOMEM);
+	}
 
-	minor = (minor_t)id_allocff(signalfd_minor);
 	if (ddi_soft_state_zalloc(signalfd_softstate, minor) != DDI_SUCCESS) {
-		id_free(signalfd_minor, minor);
-		mutex_exit(&signalfd_lock);
+		id_free(signalfd_minors, minor);
 		return (ENODEV);
 	}
 
-	state = kmem_zalloc(sizeof (*state), KM_SLEEP);
-	state->sfd_valid = B_TRUE;
-	state->sfd_count = 1;
-	list_insert_head(&signalfd_state, (void *)state);
+	signalfd_state_t *state = ddi_get_soft_state(signalfd_softstate, minor);
+	mutex_init(&state->sfd_lock, NULL, MUTEX_DEFAULT, NULL);
+	list_create(&state->sfd_pollers, sizeof (signalfd_poller_t),
+	    offsetof(signalfd_poller_t, sp_state_node));
+	state->sfd_minor = minor;
 
-	sstate = ddi_get_soft_state(signalfd_softstate, minor);
-	*sstate = state;
+	const major_t major = getemajor(*devp);
 	*devp = makedevice(major, minor);
 
-	mutex_exit(&signalfd_lock);
-
 	return (0);
 }
 
@@ -381,111 +482,114 @@ signalfd_open(dev_t *devp, int flag, int otyp, cred_t *cred_p)
  * Return 0 if we successfully consumed a signal or an errno if not.
  */
 static int
-consume_signal(k_sigset_t set, uio_t *uio, boolean_t block)
+signalfd_consume_signal(k_sigset_t set, uio_t *uio, bool should_block)
 {
-	k_sigset_t oldmask;
 	kthread_t *t = curthread;
 	klwp_t *lwp = ttolwp(t);
 	proc_t *p = ttoproc(t);
-	timespec_t now;
-	timespec_t *rqtp = NULL;	/* null means blocking */
-	int timecheck = 0;
 	int ret = 0;
-	k_siginfo_t info, *infop;
-	signalfd_siginfo_t ssi, *ssp = &ssi;
-
-	if (block == B_FALSE) {
-		timecheck = timechanged;
-		gethrestime(&now);
-		rqtp = &now;	/* non-blocking check for pending signals */
-	}
 
+	/*
+	 * Identify signals of interest so they can be processed, even if other
+	 * parts of the machinery would be poised to ignore them.
+	 */
 	t->t_sigwait = set;
 
 	mutex_enter(&p->p_lock);
-	/*
-	 * set the thread's signal mask to unmask those signals in the
-	 * specified set.
-	 */
+
+	/* Set thread signal mask to unmask those in the specified set. */
 	schedctl_finish_sigblock(t);
-	oldmask = t->t_hold;
+	const k_sigset_t oldmask = t->t_hold;
 	sigdiffset(&t->t_hold, &t->t_sigwait);
 
+	if (should_block) {
+		do {
+			ret = cv_waituntil_sig(&t->t_delay_cv, &p->p_lock,
+			    NULL, 0);
+		} while (ret > 0);
+	} else {
+		mutex_exit(&p->p_lock);
+		if (issig(FORREAL) == 0) {
+			ret = -1;
+		}
+		mutex_enter(&p->p_lock);
+	}
+
 	/*
-	 * Based on rqtp, wait indefinitely until we take a signal in our set
-	 * or return immediately if there are no signals pending from our set.
+	 * Restore thread's signal mask to its previous value.
+	 * Set t_sig_check so post_syscall sees new t_hold mask.
 	 */
-	while ((ret = cv_waituntil_sig(&t->t_delay_cv, &p->p_lock, rqtp,
-	    timecheck)) > 0)
-		continue;
-
-	/* Restore thread's signal mask to its previous value. */
 	t->t_hold = oldmask;
-	t->t_sig_check = 1;	/* so post_syscall sees new t_hold mask */
+	t->t_sig_check = 1;
 
 	if (ret == -1) {
 		/* no signals pending */
 		mutex_exit(&p->p_lock);
 		sigemptyset(&t->t_sigwait);
-		return (EAGAIN);	/* no signals pending */
+		return (EAGAIN);
 	}
 
-	/* Don't bother with signal if it is not in request set. */
+	/* Do not bother with signal if it is not in request set. */
 	if (lwp->lwp_cursig == 0 ||
 	    !sigismember(&t->t_sigwait, lwp->lwp_cursig)) {
-		mutex_exit(&p->p_lock);
 		/*
 		 * lwp_cursig is zero if pokelwps() awakened cv_wait_sig().
 		 * This happens if some other thread in this process called
 		 * forkall() or exit().
 		 */
+		mutex_exit(&p->p_lock);
 		sigemptyset(&t->t_sigwait);
 		return (EINTR);
 	}
 
-	if (lwp->lwp_curinfo) {
-		infop = &lwp->lwp_curinfo->sq_info;
+	/* Convert signal info into external, datamodel independent, struct. */
+	signalfd_siginfo_t ssi;
+	bzero(&ssi, sizeof (ssi));
+	if (lwp->lwp_curinfo != NULL) {
+		k_siginfo_t *infop = &lwp->lwp_curinfo->sq_info;
+
+		ssi.ssi_signo	= infop->si_signo;
+		ssi.ssi_errno	= infop->si_errno;
+		ssi.ssi_code	= infop->si_code;
+		ssi.ssi_pid	= infop->si_pid;
+		ssi.ssi_uid	= infop->si_uid;
+		ssi.ssi_fd	= infop->si_fd;
+		ssi.ssi_band	= infop->si_band;
+		ssi.ssi_trapno	= infop->si_trapno;
+		ssi.ssi_status	= infop->si_status;
+		ssi.ssi_utime	= infop->si_utime;
+		ssi.ssi_stime	= infop->si_stime;
+		ssi.ssi_addr	= (uint64_t)(intptr_t)infop->si_addr;
+
+		DTRACE_PROC2(signal__clear, int, 0, ksiginfo_t *, infop);
 	} else {
-		infop = &info;
-		bzero(infop, sizeof (info));
-		infop->si_signo = lwp->lwp_cursig;
-		infop->si_code = SI_NOINFO;
+		ssi.ssi_signo = lwp->lwp_cursig;
+		ssi.ssi_code = SI_NOINFO;
+
+		/* Convert to the format expected by the probe. */
+		k_siginfo_t info = {
+			.si_signo = lwp->lwp_cursig,
+			.si_code = SI_NOINFO,
+		};
+		DTRACE_PROC2(signal__clear, int, 0, ksiginfo_t *, &info);
 	}
 
 	lwp->lwp_ru.nsignals++;
-
-	DTRACE_PROC2(signal__clear, int, ret, ksiginfo_t *, infop);
 	lwp->lwp_cursig = 0;
 	lwp->lwp_extsig = 0;
-	mutex_exit(&p->p_lock);
-
-	/* Convert k_siginfo into external, datamodel independent, struct. */
-	bzero(ssp, sizeof (*ssp));
-	ssp->ssi_signo = infop->si_signo;
-	ssp->ssi_errno = infop->si_errno;
-	ssp->ssi_code = infop->si_code;
-	ssp->ssi_pid = infop->si_pid;
-	ssp->ssi_uid = infop->si_uid;
-	ssp->ssi_fd = infop->si_fd;
-	ssp->ssi_band = infop->si_band;
-	ssp->ssi_trapno = infop->si_trapno;
-	ssp->ssi_status = infop->si_status;
-	ssp->ssi_utime = infop->si_utime;
-	ssp->ssi_stime = infop->si_stime;
-	ssp->ssi_addr = (uint64_t)(intptr_t)infop->si_addr;
-
-	ret = uiomove(ssp, sizeof (*ssp), UIO_READ, uio);
-
-	if (lwp->lwp_curinfo) {
+	if (lwp->lwp_curinfo != NULL) {
 		siginfofree(lwp->lwp_curinfo);
 		lwp->lwp_curinfo = NULL;
 	}
+	mutex_exit(&p->p_lock);
+
+	ret = uiomove(&ssi, sizeof (ssi), UIO_READ, uio);
 	sigemptyset(&t->t_sigwait);
 	return (ret);
 }
 
 /*
- * This is similar to sigtimedwait. Based on the fd mode we may wait until a
+ * This is similar to sigtimedwait. Based on the fd mode, we may wait until a
  * signal within our specified set is posted. We consume as many available
  * signals within our set as we can.
  */
@@ -493,46 +597,48 @@ _NOTE(ARGSUSED(2))
 static int
 signalfd_read(dev_t dev, uio_t *uio, cred_t *cr)
 {
-	signalfd_state_t *state, **sstate;
-	minor_t minor = getminor(dev);
-	boolean_t block = B_TRUE;
+	signalfd_state_t *state;
 	k_sigset_t set;
-	boolean_t got_one = B_FALSE;
+	bool should_block = true, got_one = false;
 	int res;
 
-	if (uio->uio_resid < sizeof (signalfd_siginfo_t))
-		return (EINVAL);
+	state = ddi_get_soft_state(signalfd_softstate, getminor(dev));
+	if (state == NULL) {
+		return (ENXIO);
+	}
 
-	sstate = ddi_get_soft_state(signalfd_softstate, minor);
-	state = *sstate;
+	if (uio->uio_resid < sizeof (signalfd_siginfo_t)) {
+		return (EINVAL);
+	}
 
-	if (uio->uio_fmode & (FNDELAY|FNONBLOCK))
-		block = B_FALSE;
+	if (uio->uio_fmode & (FNDELAY|FNONBLOCK)) {
+		should_block = false;
+	}
 
 	mutex_enter(&state->sfd_lock);
-	set = state->sfd_set;
+	set = state->sfd_mask;
 	mutex_exit(&state->sfd_lock);
 
 	if (sigisempty(&set))
 		return (set_errno(EINVAL));
 
 	do  {
-		res = consume_signal(set, uio, block);
+		res = signalfd_consume_signal(set, uio, should_block);
 
 		if (res == 0) {
 			/*
 			 * After consuming one signal, do not block while
 			 * trying to consume more.
 			 */
-			got_one = B_TRUE;
-			block = B_FALSE;
+			got_one = true;
+			should_block = false;
 
 			/*
 			 * Refresh the matching signal set in case it was
 			 * updated during the wait.
 			 */
 			mutex_enter(&state->sfd_lock);
-			set = state->sfd_set;
+			set = state->sfd_mask;
 			mutex_exit(&state->sfd_lock);
 			if (sigisempty(&set))
 				break;
@@ -564,81 +670,70 @@ static int
 signalfd_poll(dev_t dev, short events, int anyyet, short *reventsp,
     struct pollhead **phpp)
 {
-	signalfd_state_t *state, **sstate;
-	minor_t minor = getminor(dev);
+	signalfd_state_t *state;
+	short revents = 0;
 	kthread_t *t = curthread;
 	proc_t *p = ttoproc(t);
-	short revents = 0;
 
-	sstate = ddi_get_soft_state(signalfd_softstate, minor);
-	state = *sstate;
+	state = ddi_get_soft_state(signalfd_softstate, getminor(dev));
+	if (state == NULL) {
+		return (ENXIO);
+	}
 
 	mutex_enter(&state->sfd_lock);
-
-	if (signalfd_sig_pending(p, t, state->sfd_set) != 0)
+	if (signalfd_sig_pending(p, t, state->sfd_mask) != 0) {
 		revents |= POLLRDNORM | POLLIN;
+	}
 
+	*reventsp = revents & events;
+	if ((*reventsp == 0 && !anyyet) || (events & POLLET) != 0) {
+		signalfd_poller_t *sp;
+
+		sp = signalfd_poller_associate(state, p);
+		*phpp = &sp->sp_pollhead;
+	}
 	mutex_exit(&state->sfd_lock);
 
-	*reventsp = revents & events;
-	if ((*reventsp == 0 && !anyyet) || (events & POLLET)) {
-		sigfd_proc_state_t *pstate;
-		sigfd_poll_waiter_t *pw;
+	return (0);
+}
 
-		/*
-		 * Enable pollwakeup handling.
-		 */
-		mutex_enter(&p->p_lock);
-		if ((pstate = (sigfd_proc_state_t *)p->p_sigfd) == NULL) {
+static void
+signalfd_set_mask(signalfd_state_t *state, const sigset_t *umask)
+{
+	k_sigset_t kmask;
 
-			mutex_exit(&p->p_lock);
-			pstate = kmem_zalloc(sizeof (*pstate), KM_SLEEP);
-			list_create(&pstate->sigfd_list,
-			    sizeof (sigfd_poll_waiter_t),
-			    offsetof(sigfd_poll_waiter_t, spw_list));
-			pstate->sigfd_pollwake_cb = signalfd_pollwake_cb;
-
-			/* Check again, after blocking for the alloc. */
-			mutex_enter(&p->p_lock);
-			if (p->p_sigfd == NULL) {
-				p->p_sigfd = pstate;
-			} else {
-				/* someone beat us to it */
-				list_destroy(&pstate->sigfd_list);
-				kmem_free(pstate, sizeof (*pstate));
-				pstate = p->p_sigfd;
-			}
-		}
+	sigutok(umask, &kmask);
 
-		pw = signalfd_wake_list_add(pstate, state);
-		*phpp = &pw->spw_pollhd;
-		mutex_exit(&p->p_lock);
+	mutex_enter(&state->sfd_lock);
+	state->sfd_mask = kmask;
+	list_t *pollers = &state->sfd_pollers;
+	for (signalfd_poller_t *sp = list_head(pollers); sp != NULL;
+	    sp = list_next(pollers, sp)) {
+		mutex_enter(&sp->sp_lock);
+		sp->sp_mask = kmask;
+		mutex_exit(&sp->sp_lock);
 	}
-
-	return (0);
+	mutex_exit(&state->sfd_lock);
 }
 
 _NOTE(ARGSUSED(4))
 static int
 signalfd_ioctl(dev_t dev, int cmd, intptr_t arg, int md, cred_t *cr, int *rv)
 {
-	signalfd_state_t *state, **sstate;
-	minor_t minor = getminor(dev);
+	signalfd_state_t *state;
 	sigset_t mask;
 
-	sstate = ddi_get_soft_state(signalfd_softstate, minor);
-	state = *sstate;
+	state = ddi_get_soft_state(signalfd_softstate, getminor(dev));
+	if (state == NULL) {
+		return (ENXIO);
+	}
 
 	switch (cmd) {
 	case SIGNALFDIOC_MASK:
-		if (ddi_copyin((caddr_t)arg, (caddr_t)&mask, sizeof (sigset_t),
-		    md) != 0)
-			return (set_errno(EFAULT));
-
-		mutex_enter(&state->sfd_lock);
-		sigutok(&mask, &state->sfd_set);
-		mutex_exit(&state->sfd_lock);
-
+		if (ddi_copyin((caddr_t)arg, &mask, sizeof (mask), md) != 0) {
+			return (EFAULT);
+		}
+		signalfd_set_mask(state, &mask);
 		return (0);
 
 	default:
@@ -652,41 +747,30 @@ _NOTE(ARGSUSED(1))
 static int
 signalfd_close(dev_t dev, int flag, int otyp, cred_t *cred_p)
 {
-	signalfd_state_t *state, **sstate;
-	sigfd_poll_waiter_t *pw = NULL;
-	minor_t minor = getminor(dev);
-	proc_t *p = curproc;
-
-	sstate = ddi_get_soft_state(signalfd_softstate, minor);
-	state = *sstate;
-
-	/* Make sure state is removed from this proc's pollwake list. */
-	mutex_enter(&p->p_lock);
-	if (p->p_sigfd != NULL) {
-		sigfd_proc_state_t *pstate = p->p_sigfd;
-
-		pw = signalfd_wake_list_rm(pstate, state);
-		if (list_is_empty(&pstate->sigfd_list)) {
-			signalfd_wake_list_cleanup(p);
-		}
-	}
-	mutex_exit(&p->p_lock);
+	signalfd_state_t *state;
+	const minor_t minor = getminor(dev);
 
-	if (pw != NULL) {
-		pollwakeup(&pw->spw_pollhd, POLLERR);
-		pollhead_clean(&pw->spw_pollhd);
-		kmem_free(pw, sizeof (*pw));
+	state = ddi_get_soft_state(signalfd_softstate, minor);
+	if (state == NULL) {
+		return (ENXIO);
 	}
 
-	mutex_enter(&signalfd_lock);
+	/*
+	 * With this signalfd instance being closed, sfd_lock is a formality, as
+	 * nothing else should be reaching for it to add pollers at this point.
+	 */
+	mutex_enter(&state->sfd_lock);
 
-	*sstate = NULL;
-	ddi_soft_state_free(signalfd_softstate, minor);
-	id_free(signalfd_minor, minor);
+	/* Dissociate any pollers from their respective processes */
+	signalfd_pollers_dissociate(state);
 
-	signalfd_state_release(state, B_TRUE);
+	/* ... and free all those (now-dissociated) pollers */
+	signalfd_pollers_free(state);
+	ASSERT(list_is_empty(&state->sfd_pollers));
 
-	mutex_exit(&signalfd_lock);
+	mutex_destroy(&state->sfd_lock);
+	ddi_soft_state_free(signalfd_softstate, minor);
+	id_free(signalfd_minors, minor);
 
 	return (0);
 }
@@ -694,47 +778,39 @@ signalfd_close(dev_t dev, int flag, int otyp, cred_t *cred_p)
 static int
 signalfd_attach(dev_info_t *devi, ddi_attach_cmd_t cmd)
 {
-	if (cmd != DDI_ATTACH || signalfd_devi != NULL)
+	if (cmd != DDI_ATTACH || signalfd_devi != NULL) {
 		return (DDI_FAILURE);
+	}
 
-	mutex_enter(&signalfd_lock);
-
-	signalfd_minor = id_space_create("signalfd_minor", 1, L_MAXMIN32 + 1);
-	if (signalfd_minor == NULL) {
+	signalfd_minors = id_space_create("signalfd_minors", 1, L_MAXMIN32 + 1);
+	if (signalfd_minors == NULL) {
 		cmn_err(CE_WARN, "signalfd couldn't create id space");
-		mutex_exit(&signalfd_lock);
 		return (DDI_FAILURE);
 	}
 
 	if (ddi_soft_state_init(&signalfd_softstate,
-	    sizeof (signalfd_state_t *), 0) != 0) {
+	    sizeof (signalfd_state_t), 0) != 0) {
 		cmn_err(CE_WARN, "signalfd failed to create soft state");
-		id_space_destroy(signalfd_minor);
-		mutex_exit(&signalfd_lock);
+		id_space_destroy(signalfd_minors);
 		return (DDI_FAILURE);
 	}
 
 	if (ddi_create_minor_node(devi, "signalfd", S_IFCHR,
 	    SIGNALFDMNRN_SIGNALFD, DDI_PSEUDO, 0) == DDI_FAILURE) {
-		cmn_err(CE_NOTE, "/dev/signalfd couldn't create minor node");
+		cmn_err(CE_NOTE, "signalfd couldn't create minor node");
 		ddi_soft_state_fini(&signalfd_softstate);
-		id_space_destroy(signalfd_minor);
-		mutex_exit(&signalfd_lock);
+		id_space_destroy(signalfd_minors);
 		return (DDI_FAILURE);
 	}
 
-	ddi_report_dev(devi);
-	signalfd_devi = devi;
 
 	sigfd_exit_helper = signalfd_exit_helper;
 
-	list_create(&signalfd_state, sizeof (signalfd_state_t),
-	    offsetof(signalfd_state_t, sfd_list));
-
 	signalfd_wakeq = taskq_create("signalfd_wake", 1, minclsyspri,
 	    0, INT_MAX, TASKQ_PREPOPULATE);
 
-	mutex_exit(&signalfd_lock);
+	ddi_report_dev(devi);
+	signalfd_devi = devi;
 
 	return (DDI_SUCCESS);
 }
@@ -743,41 +819,22 @@ _NOTE(ARGSUSED(0))
 static int
 signalfd_detach(dev_info_t *dip, ddi_detach_cmd_t cmd)
 {
-	switch (cmd) {
-	case DDI_DETACH:
-		break;
-
-	default:
-		return (DDI_FAILURE);
-	}
-
-	mutex_enter(&signalfd_lock);
-
-	if (!list_is_empty(&signalfd_state)) {
-		/*
-		 * There are dangling poll waiters holding signalfd_state_t
-		 * entries on the global list.  Detach is not possible until
-		 * they purge themselves.
-		 */
-		mutex_exit(&signalfd_lock);
+	if (cmd != DDI_DETACH) {
 		return (DDI_FAILURE);
 	}
-	list_destroy(&signalfd_state);
 
 	/*
-	 * With no remaining entries in the signalfd_state list, the wake taskq
-	 * should be empty with no possibility for new entries.
+	 * With all of the instances gone, it is safe to both destroy the waker
+	 * taskq (which must be empty) and tear down the exit helper (which must
+	 * be unreachable with no proc_t`p_sigfd associations).
 	 */
 	taskq_destroy(signalfd_wakeq);
-
-	id_space_destroy(signalfd_minor);
-
-	ddi_remove_minor_node(signalfd_devi, NULL);
-	signalfd_devi = NULL;
 	sigfd_exit_helper = NULL;
 
+	id_space_destroy(signalfd_minors);
 	ddi_soft_state_fini(&signalfd_softstate);
-	mutex_exit(&signalfd_lock);
+	ddi_remove_minor_node(signalfd_devi, NULL);
+	signalfd_devi = NULL;
 
 	return (DDI_SUCCESS);
 }
-- 
2.37.3

