git: 81f2e9063d64 - main - signal: Add SIG_FOREACH and refactor issignal()

From: Mark Johnston <markj_at_FreeBSD.org>
Date: Mon, 18 Oct 2021 14:08:14 UTC
The branch main has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=81f2e9063d64cc976b47e7ee1e9c35692cda7cb4

commit 81f2e9063d64cc976b47e7ee1e9c35692cda7cb4
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2021-10-16 13:44:40 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2021-10-18 13:56:58 +0000

    signal: Add SIG_FOREACH and refactor issignal()
    
    Add a SIG_FOREACH macro that can be used to iterate over a signal set.
    This is a bit cleaner and more efficient than calling sig_ffs() in a
    loop.  The implementation is based on BIT_FOREACH_ISSET(), except
    that the bitset limbs are always 32 bits wide, and signal sets are
    1-indexed rather than 0-indexed like bitset(9) sets.
    
    issignal() cannot really be modified to use SIG_FOREACH() directly.
    Take this opportunity to split the function into two explicit loops.
    I've always found this function hard to read and think that this change
    is an improvement.
    
    Remove sig_ffs(), nothing uses it now.
    
    Reviewed by:    kib
    MFC after:      2 weeks
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D32473
---
 sys/kern/kern_sig.c | 386 +++++++++++++++++++++++++++++-----------------------
 sys/sys/signalvar.h |   1 -
 2 files changed, 217 insertions(+), 170 deletions(-)

diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index b7155074aa5a..d6826e8dc507 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -249,6 +249,29 @@ static int sigproptbl[NSIG] = {
 	[SIGUSR2] =	SIGPROP_KILL,
 };
 
+#define	_SIG_FOREACH_ADVANCE(i, set) ({					\
+	int __found;							\
+	for (;;) {							\
+		if (__bits != 0) {					\
+			int __sig = ffs(__bits);			\
+			__bits &= ~(1u << (__sig - 1));			\
+			sig = __i * sizeof((set)->__bits[0]) * NBBY + __sig; \
+			__found = 1;					\
+			break;						\
+		}							\
+		if (++__i == _SIG_WORDS) {				\
+			__found = 0;					\
+			break;						\
+		}							\
+		__bits = (set)->__bits[__i];				\
+	}								\
+	__found != 0;							\
+})
+
+#define	SIG_FOREACH(i, set)						\
+	for (int32_t __i = -1, __bits = 0;				\
+	    _SIG_FOREACH_ADVANCE(i, set); )				\
+
 sigset_t fastblock_mask;
 
 static void
@@ -660,17 +683,6 @@ sigprop(int sig)
 	return (0);
 }
 
-int
-sig_ffs(sigset_t *set)
-{
-	int i;
-
-	for (i = 0; i < _SIG_WORDS; i++)
-		if (set->__bits[i])
-			return (ffs(set->__bits[i]) + (i * 32));
-	return (0);
-}
-
 static bool
 sigact_flag_test(const struct sigaction *act, int flag)
 {
@@ -2761,8 +2773,7 @@ reschedule_signals(struct proc *p, sigset_t block, int flags)
 		return;
 	SIGSETAND(block, p->p_siglist);
 	fastblk = (flags & SIGPROCMASK_FASTBLK) != 0;
-	while ((sig = sig_ffs(&block)) != 0) {
-		SIGDELSET(block, sig);
+	SIG_FOREACH(sig, &block) {
 		td = sigtd(p, sig, fastblk);
 
 		/*
@@ -2894,13 +2905,188 @@ sigallowstop_impl(int prev)
 	}
 }
 
+enum sigstatus {
+	SIGSTATUS_HANDLE,
+	SIGSTATUS_HANDLED,
+	SIGSTATUS_IGNORE,
+	SIGSTATUS_SBDRY_STOP,
+};
+
+/*
+ * The thread has signal "sig" pending.  Figure out what to do with it:
+ *
+ * _HANDLE     -> the caller should handle the signal
+ * _HANDLED    -> handled internally, reload pending signal set
+ * _IGNORE     -> ignored, remove from the set of pending signals and try the
+ *                next pending signal
+ * _SBDRY_STOP -> the signal should stop the thread but this is not
+ *                permitted in the current context
+ */
+static enum sigstatus
+sigprocess(struct thread *td, int sig)
+{
+	struct proc *p;
+	struct sigacts *ps;
+	struct sigqueue *queue;
+	ksiginfo_t ksi;
+	int prop;
+
+	KASSERT(_SIG_VALID(sig), ("%s: invalid signal %d", __func__, sig));
+
+	p = td->td_proc;
+	ps = p->p_sigacts;
+	mtx_assert(&ps->ps_mtx, MA_OWNED);
+	PROC_LOCK_ASSERT(p, MA_OWNED);
+
+	/*
+	 * We should allow pending but ignored signals below
+	 * only if there is sigwait() active, or P_TRACED was
+	 * on when they were posted.
+	 */
+	if (SIGISMEMBER(ps->ps_sigignore, sig) &&
+	    (p->p_flag & P_TRACED) == 0 &&
+	    (td->td_flags & TDF_SIGWAIT) == 0) {
+		return (SIGSTATUS_IGNORE);
+	}
+
+	if ((p->p_flag & (P_TRACED | P_PPTRACE)) == P_TRACED) {
+		/*
+		 * If traced, always stop.
+		 * Remove old signal from queue before the stop.
+		 * XXX shrug off debugger, it causes siginfo to
+		 * be thrown away.
+		 */
+		queue = &td->td_sigqueue;
+		ksiginfo_init(&ksi);
+		if (sigqueue_get(queue, sig, &ksi) == 0) {
+			queue = &p->p_sigqueue;
+			sigqueue_get(queue, sig, &ksi);
+		}
+		td->td_si = ksi.ksi_info;
+
+		mtx_unlock(&ps->ps_mtx);
+		sig = ptracestop(td, sig, &ksi);
+		mtx_lock(&ps->ps_mtx);
+
+		td->td_si.si_signo = 0;
+
+		/*
+		 * Keep looking if the debugger discarded or
+		 * replaced the signal.
+		 */
+		if (sig == 0)
+			return (SIGSTATUS_HANDLED);
+
+		/*
+		 * If the signal became masked, re-queue it.
+		 */
+		if (SIGISMEMBER(td->td_sigmask, sig)) {
+			ksi.ksi_flags |= KSI_HEAD;
+			sigqueue_add(&p->p_sigqueue, sig, &ksi);
+			return (SIGSTATUS_HANDLED);
+		}
+
+		/*
+		 * If the traced bit got turned off, requeue the signal and
+		 * reload the set of pending signals.  This ensures that p_sig*
+		 * and p_sigact are consistent.
+		 */
+		if ((p->p_flag & P_TRACED) == 0) {
+			ksi.ksi_flags |= KSI_HEAD;
+			sigqueue_add(queue, sig, &ksi);
+			return (SIGSTATUS_HANDLED);
+		}
+	}
+
+	/*
+	 * Decide whether the signal should be returned.
+	 * Return the signal's number, or fall through
+	 * to clear it from the pending mask.
+	 */
+	switch ((intptr_t)p->p_sigacts->ps_sigact[_SIG_IDX(sig)]) {
+	case (intptr_t)SIG_DFL:
+		/*
+		 * Don't take default actions on system processes.
+		 */
+		if (p->p_pid <= 1) {
+#ifdef DIAGNOSTIC
+			/*
+			 * Are you sure you want to ignore SIGSEGV
+			 * in init? XXX
+			 */
+			printf("Process (pid %lu) got signal %d\n",
+				(u_long)p->p_pid, sig);
+#endif
+			return (SIGSTATUS_IGNORE);
+		}
+
+		/*
+		 * If there is a pending stop signal to process with
+		 * default action, stop here, then clear the signal.
+		 * Traced or exiting processes should ignore stops.
+		 * Additionally, a member of an orphaned process group
+		 * should ignore tty stops.
+		 */
+		prop = sigprop(sig);
+		if (prop & SIGPROP_STOP) {
+			mtx_unlock(&ps->ps_mtx);
+			if ((p->p_flag & (P_TRACED | P_WEXIT |
+			    P_SINGLE_EXIT)) != 0 || ((p->p_pgrp->
+			    pg_flags & PGRP_ORPHANED) != 0 &&
+			    (prop & SIGPROP_TTYSTOP) != 0)) {
+				mtx_lock(&ps->ps_mtx);
+				return (SIGSTATUS_IGNORE);
+			}
+			if (TD_SBDRY_INTR(td)) {
+				KASSERT((td->td_flags & TDF_SBDRY) != 0,
+				    ("lost TDF_SBDRY"));
+				mtx_lock(&ps->ps_mtx);
+				return (SIGSTATUS_SBDRY_STOP);
+			}
+			WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK,
+			    &p->p_mtx.lock_object, "Catching SIGSTOP");
+			sigqueue_delete(&td->td_sigqueue, sig);
+			sigqueue_delete(&p->p_sigqueue, sig);
+			p->p_flag |= P_STOPPED_SIG;
+			p->p_xsig = sig;
+			PROC_SLOCK(p);
+			sig_suspend_threads(td, p, 0);
+			thread_suspend_switch(td, p);
+			PROC_SUNLOCK(p);
+			mtx_lock(&ps->ps_mtx);
+			return (SIGSTATUS_HANDLED);
+		} else if ((prop & SIGPROP_IGNORE) != 0 &&
+		    (td->td_flags & TDF_SIGWAIT) == 0) {
+			/*
+			 * Default action is to ignore; drop it if
+			 * not in kern_sigtimedwait().
+			 */
+			return (SIGSTATUS_IGNORE);
+		} else {
+			return (SIGSTATUS_HANDLE);
+		}
+
+	case (intptr_t)SIG_IGN:
+		if ((td->td_flags & TDF_SIGWAIT) == 0)
+			return (SIGSTATUS_IGNORE);
+		else
+			return (SIGSTATUS_HANDLE);
+
+	default:
+		/*
+		 * This signal has an action, let postsig() process it.
+		 */
+		return (SIGSTATUS_HANDLE);
+	}
+}
+
 /*
  * If the current process has received a signal (should be caught or cause
  * termination, should interrupt current syscall), return the signal number.
  * Stop signals with default action are processed immediately, then cleared;
  * they aren't returned.  This is checked after each entry to the system for
- * a syscall or trap (though this can usually be done without calling issignal
- * by checking the pending signal masks in cursig.) The normal call
+ * a syscall or trap (though this can usually be done without calling
+ * issignal by checking the pending signal masks in cursig.) The normal call
  * sequence is
  *
  *	while (sig = cursig(curthread))
@@ -2910,16 +3096,12 @@ static int
 issignal(struct thread *td)
 {
 	struct proc *p;
-	struct sigacts *ps;
-	struct sigqueue *queue;
 	sigset_t sigpending;
-	ksiginfo_t ksi;
-	int prop, sig;
+	int sig;
 
 	p = td->td_proc;
-	ps = p->p_sigacts;
-	mtx_assert(&ps->ps_mtx, MA_OWNED);
 	PROC_LOCK_ASSERT(p, MA_OWNED);
+
 	for (;;) {
 		sigpending = td->td_sigqueue.sq_signals;
 		SIGSETOR(sigpending, p->p_sigqueue.sq_signals);
@@ -2957,160 +3139,27 @@ issignal(struct thread *td)
 			 * execute the debugger attach ritual in
 			 * order.
 			 */
-			sig = SIGSTOP;
 			td->td_dbgflags |= TDB_FSTP;
-		} else {
-			sig = sig_ffs(&sigpending);
+			SIGEMPTYSET(sigpending);
+			SIGADDSET(sigpending, SIGSTOP);
 		}
 
-		/*
-		 * We should allow pending but ignored signals below
-		 * only if there is sigwait() active, or P_TRACED was
-		 * on when they were posted.
-		 */
-		if (SIGISMEMBER(ps->ps_sigignore, sig) &&
-		    (p->p_flag & P_TRACED) == 0 &&
-		    (td->td_flags & TDF_SIGWAIT) == 0) {
-			sigqueue_delete(&td->td_sigqueue, sig);
-			sigqueue_delete(&p->p_sigqueue, sig);
-			continue;
-		}
-		if ((p->p_flag & (P_TRACED | P_PPTRACE)) == P_TRACED) {
-			/*
-			 * If traced, always stop.
-			 * Remove old signal from queue before the stop.
-			 * XXX shrug off debugger, it causes siginfo to
-			 * be thrown away.
-			 */
-			queue = &td->td_sigqueue;
-			ksiginfo_init(&ksi);
-			if (sigqueue_get(queue, sig, &ksi) == 0) {
-				queue = &p->p_sigqueue;
-				sigqueue_get(queue, sig, &ksi);
-			}
-			td->td_si = ksi.ksi_info;
-
-			mtx_unlock(&ps->ps_mtx);
-			sig = ptracestop(td, sig, &ksi);
-			mtx_lock(&ps->ps_mtx);
-
-			td->td_si.si_signo = 0;
-
-			/* 
-			 * Keep looking if the debugger discarded or
-			 * replaced the signal.
-			 */
-			if (sig == 0)
-				continue;
-
-			/*
-			 * If the signal became masked, re-queue it.
-			 */
-			if (SIGISMEMBER(td->td_sigmask, sig)) {
-				ksi.ksi_flags |= KSI_HEAD;
-				sigqueue_add(&p->p_sigqueue, sig, &ksi);
-				continue;
-			}
-
-			/*
-			 * If the traced bit got turned off, requeue
-			 * the signal and go back up to the top to
-			 * rescan signals.  This ensures that p_sig*
-			 * and p_sigact are consistent.
-			 */
-			if ((p->p_flag & P_TRACED) == 0) {
-				ksi.ksi_flags |= KSI_HEAD;
-				sigqueue_add(queue, sig, &ksi);
-				continue;
-			}
-		}
-
-		prop = sigprop(sig);
-
-		/*
-		 * Decide whether the signal should be returned.
-		 * Return the signal's number, or fall through
-		 * to clear it from the pending mask.
-		 */
-		switch ((intptr_t)p->p_sigacts->ps_sigact[_SIG_IDX(sig)]) {
-		case (intptr_t)SIG_DFL:
-			/*
-			 * Don't take default actions on system processes.
-			 */
-			if (p->p_pid <= 1) {
-#ifdef DIAGNOSTIC
-				/*
-				 * Are you sure you want to ignore SIGSEGV
-				 * in init? XXX
-				 */
-				printf("Process (pid %lu) got signal %d\n",
-					(u_long)p->p_pid, sig);
-#endif
-				break;		/* == ignore */
-			}
-			/*
-			 * If there is a pending stop signal to process with
-			 * default action, stop here, then clear the signal.
-			 * Traced or exiting processes should ignore stops.
-			 * Additionally, a member of an orphaned process group
-			 * should ignore tty stops.
-			 */
-			if (prop & SIGPROP_STOP) {
-				mtx_unlock(&ps->ps_mtx);
-				if ((p->p_flag & (P_TRACED | P_WEXIT |
-				    P_SINGLE_EXIT)) != 0 || ((p->p_pgrp->
-				    pg_flags & PGRP_ORPHANED) != 0 &&
-				    (prop & SIGPROP_TTYSTOP) != 0)) {
-					mtx_lock(&ps->ps_mtx);
-					break;	/* == ignore */
-				}
-				if (TD_SBDRY_INTR(td)) {
-					KASSERT((td->td_flags & TDF_SBDRY) != 0,
-					    ("lost TDF_SBDRY"));
-					mtx_lock(&ps->ps_mtx);
-					return (-1);
-				}
-				WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK,
-				    &p->p_mtx.lock_object, "Catching SIGSTOP");
+		SIG_FOREACH(sig, &sigpending) {
+			switch (sigprocess(td, sig)) {
+			case SIGSTATUS_HANDLE:
+				return (sig);
+			case SIGSTATUS_HANDLED:
+				goto next;
+			case SIGSTATUS_IGNORE:
 				sigqueue_delete(&td->td_sigqueue, sig);
 				sigqueue_delete(&p->p_sigqueue, sig);
-				p->p_flag |= P_STOPPED_SIG;
-				p->p_xsig = sig;
-				PROC_SLOCK(p);
-				sig_suspend_threads(td, p, 0);
-				thread_suspend_switch(td, p);
-				PROC_SUNLOCK(p);
-				mtx_lock(&ps->ps_mtx);
-				goto next;
-			} else if ((prop & SIGPROP_IGNORE) != 0 &&
-			    (td->td_flags & TDF_SIGWAIT) == 0) {
-				/*
-				 * Default action is to ignore; drop it if
-				 * not in kern_sigtimedwait().
-				 */
-				break;		/* == ignore */
-			} else
-				return (sig);
-			/*NOTREACHED*/
-
-		case (intptr_t)SIG_IGN:
-			if ((td->td_flags & TDF_SIGWAIT) == 0)
-				break;		/* == ignore */
-			else
-				return (sig);
-
-		default:
-			/*
-			 * This signal has an action, let
-			 * postsig() process it.
-			 */
-			return (sig);
+				break;
+			case SIGSTATUS_SBDRY_STOP:
+				return (-1);
+			}
 		}
-		sigqueue_delete(&td->td_sigqueue, sig);	/* take the signal! */
-		sigqueue_delete(&p->p_sigqueue, sig);
 next:;
 	}
-	/* NOTREACHED */
 }
 
 void
@@ -4119,8 +4168,7 @@ sig_drop_caught(struct proc *p)
 	ps = p->p_sigacts;
 	PROC_LOCK_ASSERT(p, MA_OWNED);
 	mtx_assert(&ps->ps_mtx, MA_OWNED);
-	while (SIGNOTEMPTY(ps->ps_sigcatch)) {
-		sig = sig_ffs(&ps->ps_sigcatch);
+	SIG_FOREACH(sig, &ps->ps_sigcatch) {
 		sigdflt(ps, sig);
 		if ((sigprop(sig) & SIGPROP_IGNORE) != 0)
 			sigqueue_delete_proc(p, sig);
diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h
index a7fe174b40bf..4e86f54856f6 100644
--- a/sys/sys/signalvar.h
+++ b/sys/sys/signalvar.h
@@ -404,7 +404,6 @@ int	sig_ast_needsigchk(struct thread *td);
 void	sig_drop_caught(struct proc *p);
 void	sigexit(struct thread *td, int sig) __dead2;
 int	sigev_findtd(struct proc *p, struct sigevent *sigev, struct thread **);
-int	sig_ffs(sigset_t *set);
 void	sigfastblock_clear(struct thread *td);
 void	sigfastblock_fetch(struct thread *td);
 void	sigfastblock_setpend(struct thread *td, bool resched);