Rework the handling of stop signals in the NFS client. The changes in

195702, 195703, and 195821 prevented a thread from suspending while holding
locks inside of NFS by forcing the thread to fail sleeps with EINTR or
ERESTART but defer the thread suspension to the user boundary.  However,
this had the effect that stopping a process during an NFS request could
abort the request and trigger EINTR errors that were visible to userland
processes (previously the thread would have suspended and completed the
request once it was resumed).

This change instead effectively masks stop signals while in the NFS client.
It uses the existing TDF_SBDRY flag to effect this since SIGSTOP cannot
be masked directly.  Also, instead of setting PBDRY on individual sleeps,
the NFS client now sets the TDF_SBDRY flag around each NFS request and
stop signals are masked for all sleeps during that region (the previous
change missed sleeps in lockmgr locks).  The end result is that stop
signals sent to threads performing an NFS request are completely
ignored until after the NFS request has finished processing and the
thread prepares to return to userland.  This restores the behavior of
stop signals being transparent to userland processes while still
preventing threads from suspending while holding NFS locks.

Reviewed by:	kib
MFC after:	1 month

5 files changed, 57 insertions, 21 deletions

diff --git a/sys/fs/nfs/nfs_commonkrpc.c b/sys/fs/nfs/nfs_commonkrpc.c
index 23d8954..f08ba85 100644
--- a/sys/fs/nfs/nfs_commonkrpc.c
+++ b/sys/fs/nfs/nfs_commonkrpc.c
@@ -1031,7 +1031,6 @@ int newnfs_sig_set[] = {
 	SIGTERM,
 	SIGHUP,
 	SIGKILL,
-	SIGSTOP,
 	SIGQUIT
 };
 
@@ -1052,7 +1051,7 @@ nfs_sig_pending(sigset_t set)
  
 /*
  * The set/restore sigmask functions are used to (temporarily) overwrite
- * the process p_sigmask during an RPC call (for example). These are also
+ * the thread td_sigmask during an RPC call (for example). These are also
  * used in other places in the NFS client that might tsleep().
  */
 void
@@ -1081,8 +1080,10 @@ newnfs_set_sigmask(struct thread *td, sigset_t *oldset)
 			SIGDELSET(newset, newnfs_sig_set[i]);
 	}
 	mtx_unlock(&p->p_sigacts->ps_mtx);
+	sigdeferstop(td);
+	kern_sigprocmask(td, SIG_SETMASK, &newset, oldset,
+	    SIGPROCMASK_PROC_LOCKED);
 	PROC_UNLOCK(p);
-	kern_sigprocmask(td, SIG_SETMASK, &newset, oldset, 0);
 }
 
 void
@@ -1091,6 +1092,7 @@ newnfs_restore_sigmask(struct thread *td, sigset_t *set)
 	if (td == NULL)
 		td = curthread; /* XXX */
 	kern_sigprocmask(td, SIG_SETMASK, set, NULL, 0);
+	sigallowstop(td);
 }
 
 /*
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 9c52707..e28bdc4 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -2364,6 +2364,13 @@ tdsigwakeup(struct thread *td, int sig, sig_t action, int intrval)
 		}
 
 		/*
+		 * Don't awaken a sleeping thread for SIGSTOP if the
+		 * STOP signal is deferred.
+		 */
+		if ((prop & SA_STOP) && (td->td_flags & TDF_SBDRY))
+			goto out;
+
+		/*
 		 * Give low priority threads a better chance to run.
 		 */
 		if (td->td_priority > PUSER)
@@ -2404,12 +2411,13 @@ sig_suspend_threads(struct thread *td, struct proc *p, int sending)
 		if ((TD_IS_SLEEPING(td2) || TD_IS_SWAPPED(td2)) &&
 		    (td2->td_flags & TDF_SINTR)) {
 			if (td2->td_flags & TDF_SBDRY) {
-				if (TD_IS_SUSPENDED(td2))
-					wakeup_swapper |=
-					    thread_unsuspend_one(td2);
-				if (TD_ON_SLEEPQ(td2))
-					wakeup_swapper |=
-					    sleepq_abort(td2, ERESTART);
+				/*
+				 * Once a thread is asleep with
+				 * TDF_SBDRY set, it should never
+				 * become suspended due to this check.
+				 */
+				KASSERT(!TD_IS_SUSPENDED(td2),
+				    ("thread with deferred stops suspended"));
 			} else if (!TD_IS_SUSPENDED(td2)) {
 				thread_suspend_one(td2);
 			}
@@ -2529,6 +2537,34 @@ tdsigcleanup(struct thread *td)
 
 }
 
+/* Defer the delivery of SIGSTOP for the current thread. */
+void
+sigdeferstop(struct thread *td)
+{
+
+	KASSERT(!(td->td_flags & TDF_SBDRY),
+	    ("attempt to set TDF_SBDRY recursively"));
+	thread_lock(td);
+	td->td_flags |= TDF_SBDRY;
+	thread_unlock(td);
+}
+
+/*
+ * Permit the delivery of SIGSTOP for the current thread.  This does
+ * not immediately suspend if a stop was posted.  Instead, the thread
+ * will suspend either via ast() or a subsequent interruptible sleep.
+ */
+void
+sigallowstop(struct thread *td)
+{
+
+	KASSERT(td->td_flags & TDF_SBDRY,
+	    ("attempt to clear already-cleared TDF_SBDRY"));
+	thread_lock(td);
+	td->td_flags &= ~TDF_SBDRY;
+	thread_unlock(td);
+}
+
 /*
  * If the current process has received a signal (should be caught or cause
  * termination, should interrupt current syscall), return the signal number.
@@ -2561,7 +2597,7 @@ issignal(struct thread *td, int stop_allowed)
 		SIGSETOR(sigpending, p->p_sigqueue.sq_signals);
 		SIGSETNAND(sigpending, td->td_sigmask);
 
-		if (p->p_flag & P_PPWAIT)
+		if (p->p_flag & P_PPWAIT || td->td_flags & TDF_SBDRY)
 			SIG_STOPSIGMASK(sigpending);
 		if (SIGISEMPTY(sigpending))	/* no signal to send */
 			return (0);
@@ -2677,10 +2713,6 @@ issignal(struct thread *td, int stop_allowed)
 				    (p->p_pgrp->pg_jobc == 0 &&
 				     prop & SA_TTYSTOP))
 					break;	/* == ignore */
-
-				/* Ignore, but do not drop the stop signal. */
-				if (stop_allowed != SIG_STOP_ALLOWED)
-					return (sig);
 				mtx_unlock(&ps->ps_mtx);
 				WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK,
 				    &p->p_mtx.lock_object, "Catching SIGSTOP");
diff --git a/sys/kern/subr_sleepqueue.c b/sys/kern/subr_sleepqueue.c
index 2d5ea51..b6bd8fc 100644
--- a/sys/kern/subr_sleepqueue.c
+++ b/sys/kern/subr_sleepqueue.c
@@ -352,8 +352,6 @@ sleepq_add(void *wchan, struct lock_object *lock, const char *wmesg, int flags,
 	if (flags & SLEEPQ_INTERRUPTIBLE) {
 		td->td_flags |= TDF_SINTR;
 		td->td_flags &= ~TDF_SLEEPABORT;
-		if (flags & SLEEPQ_STOP_ON_BDRY)
-			td->td_flags |= TDF_SBDRY;
 	}
 	thread_unlock(td);
 }
@@ -600,7 +598,7 @@ sleepq_check_signals(void)
 
 	/* We are no longer in an interruptible sleep. */
 	if (td->td_flags & TDF_SINTR)
-		td->td_flags &= ~(TDF_SINTR | TDF_SBDRY);
+		td->td_flags &= ~TDF_SINTR;
 
 	if (td->td_flags & TDF_SLEEPABORT) {
 		td->td_flags &= ~TDF_SLEEPABORT;
@@ -747,7 +745,7 @@ sleepq_resume_thread(struct sleepqueue *sq, struct thread *td, int pri)
 
 	td->td_wmesg = NULL;
 	td->td_wchan = NULL;
-	td->td_flags &= ~(TDF_SINTR | TDF_SBDRY);
+	td->td_flags &= ~TDF_SINTR;
 
 	CTR3(KTR_PROC, "sleepq_wakeup: thread %p (pid %ld, %s)",
 	    (void *)td, (long)td->td_proc->p_pid, td->td_name);
diff --git a/sys/nfsclient/nfs_krpc.c b/sys/nfsclient/nfs_krpc.c
index dd441ae..851b0b6 100644
--- a/sys/nfsclient/nfs_krpc.c
+++ b/sys/nfsclient/nfs_krpc.c
@@ -699,7 +699,6 @@ int nfs_sig_set[] = {
 	SIGTERM,
 	SIGHUP,
 	SIGKILL,
-	SIGSTOP,
 	SIGQUIT
 };
 
@@ -720,7 +719,7 @@ nfs_sig_pending(sigset_t set)
 
 /*
  * The set/restore sigmask functions are used to (temporarily) overwrite
- * the process p_sigmask during an RPC call (for example).  These are also
+ * the thread td_sigmask during an RPC call (for example).  These are also
  * used in other places in the NFS client that might tsleep().
  */
 void
@@ -749,8 +748,10 @@ nfs_set_sigmask(struct thread *td, sigset_t *oldset)
 			SIGDELSET(newset, nfs_sig_set[i]);
 	}
 	mtx_unlock(&p->p_sigacts->ps_mtx);
+	sigdeferstop(td);
+	kern_sigprocmask(td, SIG_SETMASK, &newset, oldset,
+	    SIGPROCMASK_PROC_LOCKED);
 	PROC_UNLOCK(p);
-	kern_sigprocmask(td, SIG_SETMASK, &newset, oldset, 0);
 }
 
 void
@@ -759,6 +760,7 @@ nfs_restore_sigmask(struct thread *td, sigset_t *set)
 	if (td == NULL)
 		td = curthread; /* XXX */
 	kern_sigprocmask(td, SIG_SETMASK, set, NULL, 0);
+	sigallowstop(td);
 }
 
 /*
diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h
index 71685e7..91655b2 100644
--- a/sys/sys/signalvar.h
+++ b/sys/sys/signalvar.h
@@ -328,6 +328,8 @@ extern struct mtx	sigio_lock;
 #define	SIGPROCMASK_PS_LOCKED	0x0004
 
 int	cursig(struct thread *td, int stop_allowed);
+void	sigdeferstop(struct thread *td);
+void	sigallowstop(struct thread *td);
 void	execsigs(struct proc *p);
 void	gsignal(int pgid, int sig, ksiginfo_t *ksi);
 void	killproc(struct proc *p, char *why);