Fix a long standing race between sleep queue and thread

suspension code. When a thread A is going to sleep, it calls
sleepq_catch_signals() to detect any pending signals or thread
suspension request, if nothing happens, it returns without
holding process lock or scheduler lock, this opens a race
window which allows thread B to come in and do process
suspension work, however since A is still at running state,
thread B can do nothing to A, thread A continues, and puts
itself into actually sleeping state, but B has never seen it,
and it sits there forever until B is woken up by other threads
sometimes later(this can be very long delay or never
happen). Fix this bug by forcing sleepq_catch_signals to
return with scheduler lock held.
Fix sleepq_abort() by passing it an interrupted code, previously,
it worked as wakeup_one(), and the interruption can not be
identified correctly by sleep queue code when the sleeping
thread is resumed.
Let thread_suspend_check() returns EINTR or ERESTART, so sleep
queue no longer has to use SIGSTOP as a hack to build a return
value.

Reviewed by:	jhb
MFC after:	1 week

8 files changed, 99 insertions, 114 deletions

diff --git a/sys/kern/kern_condvar.c b/sys/kern/kern_condvar.c
index ede6b61..f430ea8 100644
--- a/sys/kern/kern_condvar.c
+++ b/sys/kern/kern_condvar.c
@@ -166,7 +166,7 @@ cv_wait_sig(struct cv *cvp, struct mtx *mp)
 {
 	struct thread *td;
 	struct proc *p;
-	int rval, sig;
+	int rval;
 	WITNESS_SAVE_DECL(mp);
 
 	td = curthread;
@@ -210,10 +210,7 @@ cv_wait_sig(struct cv *cvp, struct mtx *mp)
 
 	sleepq_add(cvp, mp, cvp->cv_description, SLEEPQ_CONDVAR |
 	    SLEEPQ_INTERRUPTIBLE);
-	sig = sleepq_catch_signals(cvp);
 	rval = sleepq_wait_sig(cvp);
-	if (rval == 0)
-		rval = sleepq_calc_signal_retval(sig);
 
 #ifdef KTRACE
 	if (KTRPOINT(td, KTR_CSW))
@@ -292,7 +289,6 @@ cv_timedwait_sig(struct cv *cvp, struct mtx *mp, int timo)
 	struct thread *td;
 	struct proc *p;
 	int rval;
-	int sig;
 	WITNESS_SAVE_DECL(mp);
 
 	td = curthread;
@@ -338,10 +334,7 @@ cv_timedwait_sig(struct cv *cvp, struct mtx *mp, int timo)
 	sleepq_add(cvp, mp, cvp->cv_description, SLEEPQ_CONDVAR |
 	    SLEEPQ_INTERRUPTIBLE);
 	sleepq_set_timeout(cvp, timo);
-	sig = sleepq_catch_signals(cvp);
-	rval = sleepq_timedwait_sig(cvp, sig != 0);
-	if (rval == 0)
-		rval = sleepq_calc_signal_retval(sig);
+	rval = sleepq_timedwait_sig(cvp);
 
 #ifdef KTRACE
 	if (KTRPOINT(td, KTR_CSW))
diff --git a/sys/kern/kern_kse.c b/sys/kern/kern_kse.c
index 7618043..e5ae121 100644
--- a/sys/kern/kern_kse.c
+++ b/sys/kern/kern_kse.c
@@ -223,7 +223,7 @@ kse_thr_interrupt(struct thread *td, struct kse_thr_interrupt_args *uap)
 			else
 				td2->td_intrval = ERESTART;
 			if (TD_ON_SLEEPQ(td2) && (td2->td_flags & TDF_SINTR))
-				sleepq_abort(td2);
+				sleepq_abort(td2, td2->td_intrval);
 			mtx_unlock_spin(&sched_lock);
 		}
 		PROC_UNLOCK(p);
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 41bb6ff..1a49a0c 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -92,7 +92,7 @@ static char	*expand_name(const char *, uid_t, pid_t);
 static int	killpg1(struct thread *td, int sig, int pgid, int all);
 static int	issignal(struct thread *p);
 static int	sigprop(int sig);
-static void	tdsigwakeup(struct thread *, int, sig_t);
+static void	tdsigwakeup(struct thread *, int, sig_t, int);
 static void	sig_suspend_threads(struct thread *, struct proc *, int);
 static int	filt_sigattach(struct knote *kn);
 static void	filt_sigdetach(struct knote *kn);
@@ -2056,6 +2056,7 @@ do_tdsignal(struct proc *p, struct thread *td, int sig, ksiginfo_t *ksi)
 	sigqueue_t *sigqueue;
 	int prop;
 	struct sigacts *ps;
+	int intrval;
 	int ret = 0;
 
 	PROC_LOCK_ASSERT(p, MA_OWNED);
@@ -2116,6 +2117,10 @@ do_tdsignal(struct proc *p, struct thread *td, int sig, ksiginfo_t *ksi)
 		action = SIG_CATCH;
 	else
 		action = SIG_DFL;
+	if (SIGISMEMBER(ps->ps_sigintr, sig))
+		intrval = EINTR;
+	else
+		intrval = ERESTART;
 	mtx_unlock(&ps->ps_mtx);
 
 	if (prop & SA_CONT)
@@ -2260,7 +2265,7 @@ do_tdsignal(struct proc *p, struct thread *td, int sig, ksiginfo_t *ksi)
 		 */
 		mtx_lock_spin(&sched_lock);
 		if (TD_ON_SLEEPQ(td) && (td->td_flags & TDF_SINTR))
-			sleepq_abort(td);
+			sleepq_abort(td, intrval);
 		mtx_unlock_spin(&sched_lock);
 		goto out;
 		/*
@@ -2270,7 +2275,7 @@ do_tdsignal(struct proc *p, struct thread *td, int sig, ksiginfo_t *ksi)
 	} else if (p->p_state == PRS_NORMAL) {
 		if (p->p_flag & P_TRACED || action == SIG_CATCH) {
 			mtx_lock_spin(&sched_lock);
-			tdsigwakeup(td, sig, action);
+			tdsigwakeup(td, sig, action, intrval);
 			mtx_unlock_spin(&sched_lock);
 			goto out;
 		}
@@ -2315,7 +2320,7 @@ do_tdsignal(struct proc *p, struct thread *td, int sig, ksiginfo_t *ksi)
 
 runfast:
 	mtx_lock_spin(&sched_lock);
-	tdsigwakeup(td, sig, action);
+	tdsigwakeup(td, sig, action, intrval);
 	thread_unsuspend(p);
 	mtx_unlock_spin(&sched_lock);
 out:
@@ -2330,7 +2335,7 @@ out:
  * out of any sleep it may be in etc.
  */
 static void
-tdsigwakeup(struct thread *td, int sig, sig_t action)
+tdsigwakeup(struct thread *td, int sig, sig_t action, int intrval)
 {
 	struct proc *p = td->td_proc;
 	register int prop;
@@ -2382,7 +2387,7 @@ tdsigwakeup(struct thread *td, int sig, sig_t action)
 		if (td->td_priority > PUSER)
 			sched_prio(td, PUSER);
 
-		sleepq_abort(td);
+		sleepq_abort(td, intrval);
 	} else {
 		/*
 		 * Other states do nothing with the signal immediately,
diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
index 66fbef9..1abbc92 100644
--- a/sys/kern/kern_synch.c
+++ b/sys/kern/kern_synch.c
@@ -124,7 +124,7 @@ msleep(ident, mtx, priority, wmesg, timo)
 {
 	struct thread *td;
 	struct proc *p;
-	int catch, rval, sig, flags;
+	int catch, rval, flags;
 	WITNESS_SAVE_DECL(mtx);
 
 	td = curthread;
@@ -205,10 +205,6 @@ msleep(ident, mtx, priority, wmesg, timo)
 	sleepq_add(ident, mtx, wmesg, flags);
 	if (timo)
 		sleepq_set_timeout(ident, timo);
-	if (catch) {
-		sig = sleepq_catch_signals(ident);
-	} else
-		sig = 0;
 
 	/*
 	 * Adjust this thread's priority.
@@ -218,7 +214,7 @@ msleep(ident, mtx, priority, wmesg, timo)
 	mtx_unlock_spin(&sched_lock);
 
 	if (timo && catch)
-		rval = sleepq_timedwait_sig(ident, sig != 0);
+		rval = sleepq_timedwait_sig(ident);
 	else if (timo)
 		rval = sleepq_timedwait(ident);
 	else if (catch)
@@ -227,8 +223,6 @@ msleep(ident, mtx, priority, wmesg, timo)
 		sleepq_wait(ident);
 		rval = 0;
 	}
-	if (rval == 0 && catch)
-		rval = sleepq_calc_signal_retval(sig);
 #ifdef KTRACE
 	if (KTRPOINT(td, KTR_CSW))
 		ktrcsw(0, 0);
diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c
index 5237dcf..6b96f74 100644
--- a/sys/kern/kern_thread.c
+++ b/sys/kern/kern_thread.c
@@ -762,7 +762,7 @@ thread_single(int mode)
 						thread_unsuspend_one(td2);
 					if (TD_ON_SLEEPQ(td2) &&
 					    (td2->td_flags & TDF_SINTR))
-						sleepq_abort(td2);
+						sleepq_abort(td2, EINTR);
 					break;
 				case SINGLE_BOUNDARY:
 					if (TD_IS_SUSPENDED(td2) &&
@@ -770,7 +770,7 @@ thread_single(int mode)
 						thread_unsuspend_one(td2);
 					if (TD_ON_SLEEPQ(td2) &&
 					    (td2->td_flags & TDF_SINTR))
-						sleepq_abort(td2);
+						sleepq_abort(td2, ERESTART);
 					break;
 				default:	
 					if (TD_IS_SUSPENDED(td2))
@@ -894,12 +894,12 @@ thread_suspend_check(int return_instead)
 				return (0);	/* Exempt from stopping. */
 		}
 		if ((p->p_flag & P_SINGLE_EXIT) && return_instead)
-			return (1);
+			return (EINTR);
 
 		/* Should we goto user boundary if we didn't come from there? */
 		if (P_SHOULDSTOP(p) == P_STOPPED_SINGLE &&
 		    (p->p_flag & P_SINGLE_BOUNDARY) && return_instead)
-			return (1);
+			return (ERESTART);
 
 		/* If thread will exit, flush its pending signals */
 		if ((p->p_flag & P_SINGLE_EXIT) && (p->p_singlethread != td))
diff --git a/sys/kern/subr_sleepqueue.c b/sys/kern/subr_sleepqueue.c
index 238d4bf..464cf28 100644
--- a/sys/kern/subr_sleepqueue.c
+++ b/sys/kern/subr_sleepqueue.c
@@ -318,8 +318,10 @@ sleepq_add(void *wchan, struct mtx *lock, const char *wmesg, int flags)
 	mtx_lock_spin(&sched_lock);
 	td->td_wchan = wchan;
 	td->td_wmesg = wmesg;
-	if (flags & SLEEPQ_INTERRUPTIBLE)
+	if (flags & SLEEPQ_INTERRUPTIBLE) {
 		td->td_flags |= TDF_SINTR;
+		td->td_flags &= ~TDF_SLEEPABORT;
+	}
 	mtx_unlock_spin(&sched_lock);
 }
 
@@ -345,55 +347,64 @@ sleepq_set_timeout(void *wchan, int timo)
 /*
  * Marks the pending sleep of the current thread as interruptible and
  * makes an initial check for pending signals before putting a thread
- * to sleep.
+ * to sleep. Return with sleep queue and scheduler lock held.
  */
-int
+static int
 sleepq_catch_signals(void *wchan)
 {
 	struct sleepqueue_chain *sc;
 	struct sleepqueue *sq;
 	struct thread *td;
 	struct proc *p;
-	int sig;
+	struct sigacts *ps;
+	int sig, ret;
 
 	td = curthread;
-	p = td->td_proc;
+	p = curproc;
 	sc = SC_LOOKUP(wchan);
 	mtx_assert(&sc->sc_lock, MA_OWNED);
-	MPASS(td->td_sleepqueue == NULL);
 	MPASS(wchan != NULL);
 	CTR3(KTR_PROC, "sleepq catching signals: thread %p (pid %ld, %s)",
-	    (void *)td, (long)p->p_pid, p->p_comm);
+		(void *)td, (long)p->p_pid, p->p_comm);
 
-	/* Mark thread as being in an interruptible sleep. */
 	MPASS(td->td_flags & TDF_SINTR);
-	MPASS(TD_ON_SLEEPQ(td));
-	sleepq_release(wchan);
+	mtx_unlock_spin(&sc->sc_lock);
 
 	/* See if there are any pending signals for this thread. */
 	PROC_LOCK(p);
-	mtx_lock(&p->p_sigacts->ps_mtx);
+	ps = p->p_sigacts;
+	mtx_lock(&ps->ps_mtx);
 	sig = cursig(td);
-	mtx_unlock(&p->p_sigacts->ps_mtx);
-	if (sig == 0 && thread_suspend_check(1))
-		sig = SIGSTOP;
-	PROC_UNLOCK(p);
+	if (sig == 0) {
+		mtx_unlock(&ps->ps_mtx);
+		ret = thread_suspend_check(1);
+		MPASS(ret == 0 || ret == EINTR || ret == ERESTART);
+	} else {
+		if (SIGISMEMBER(ps->ps_sigintr, sig))
+			ret = EINTR;
+		else
+			ret = ERESTART;
+		mtx_unlock(&ps->ps_mtx);
+	}
 
-	/*
-	 * If there were pending signals and this thread is still on
-	 * the sleep queue, remove it from the sleep queue.  If the
-	 * thread was removed from the sleep queue while we were blocked
-	 * above, then clear TDF_SINTR before returning.
-	 */
-	sleepq_lock(wchan);
-	sq = sleepq_lookup(wchan);
-	mtx_lock_spin(&sched_lock);
-	if (TD_ON_SLEEPQ(td) && sig != 0)
-		sleepq_resume_thread(sq, td, -1);
-	else if (!TD_ON_SLEEPQ(td) && sig == 0)
+	if (ret) {
+		PROC_UNLOCK(p);
+		/*
+		 * If there were pending signals and this thread is still on
+		 * the sleep queue, remove it from the sleep queue.
+		 */
+		mtx_lock_spin(&sc->sc_lock);
+		sq = sleepq_lookup(wchan);
+		mtx_lock_spin(&sched_lock);
+		if (TD_ON_SLEEPQ(td))
+			sleepq_resume_thread(sq, td, -1);
 		td->td_flags &= ~TDF_SINTR;
-	mtx_unlock_spin(&sched_lock);
-	return (sig);
+	} else {
+		mtx_lock_spin(&sc->sc_lock);
+		mtx_lock_spin(&sched_lock);
+		PROC_UNLOCK(p);
+	}
+	return (ret);
 }
 
 /*
@@ -409,6 +420,7 @@ sleepq_switch(void *wchan)
 	td = curthread;
 	sc = SC_LOOKUP(wchan);
 	mtx_assert(&sc->sc_lock, MA_OWNED);
+	mtx_assert(&sched_lock, MA_OWNED);
 
 	/* 
 	 * If we have a sleep queue, then we've already been woken up, so
@@ -417,16 +429,13 @@ sleepq_switch(void *wchan)
 	if (td->td_sleepqueue != NULL) {
 		MPASS(!TD_ON_SLEEPQ(td));
 		mtx_unlock_spin(&sc->sc_lock);
-		mtx_lock_spin(&sched_lock);
 		return;
 	}
 
 	/*
 	 * Otherwise, actually go to sleep.
 	 */
-	mtx_lock_spin(&sched_lock);
 	mtx_unlock_spin(&sc->sc_lock);
-
 	sched_sleep(td);
 	TD_SET_SLEEPING(td);
 	mi_switch(SW_VOL, NULL);
@@ -485,52 +494,19 @@ sleepq_check_signals(void)
 	mtx_assert(&sched_lock, MA_OWNED);
 	td = curthread;
 
-	/*
-	 * If TDF_SINTR is clear, then we were awakened while executing
-	 * sleepq_catch_signals().
-	 */
-	if (!(td->td_flags & TDF_SINTR))
-		return (0);
-
 	/* We are no longer in an interruptible sleep. */
-	td->td_flags &= ~TDF_SINTR;
+	if (td->td_flags & TDF_SINTR)
+		td->td_flags &= ~TDF_SINTR;
 
-	if (td->td_flags & TDF_INTERRUPT)
+	if (td->td_flags & TDF_SLEEPABORT) {
+		td->td_flags &= ~TDF_SLEEPABORT;
 		return (td->td_intrval);
-	return (0);
-}
+	}
 
-/*
- * If we were in an interruptible sleep and we weren't interrupted and
- * didn't timeout, check to see if there are any pending signals and
- * which return value we should use if so.  The return value from an
- * earlier call to sleepq_catch_signals() should be passed in as the
- * argument.
- */
-int
-sleepq_calc_signal_retval(int sig)
-{
-	struct thread *td;
-	struct proc *p;
-	int rval;
+	if (td->td_flags & TDF_INTERRUPT)
+		return (td->td_intrval);
 
-	td = curthread;
-	p = td->td_proc;
-	PROC_LOCK(p);
-	mtx_lock(&p->p_sigacts->ps_mtx);
-	/* XXX: Should we always be calling cursig()? */
-	if (sig == 0)
-		sig = cursig(td);
-	if (sig != 0) {
-		if (SIGISMEMBER(p->p_sigacts->ps_sigintr, sig))
-			rval = EINTR;
-		else
-			rval = ERESTART;
-	} else
-		rval = 0;
-	mtx_unlock(&p->p_sigacts->ps_mtx);
-	PROC_UNLOCK(p);
-	return (rval);
+	return (0);
 }
 
 /*
@@ -541,6 +517,7 @@ sleepq_wait(void *wchan)
 {
 
 	MPASS(!(curthread->td_flags & TDF_SINTR));
+	mtx_lock_spin(&sched_lock);
 	sleepq_switch(wchan);
 	mtx_unlock_spin(&sched_lock);
 }
@@ -552,11 +529,18 @@ sleepq_wait(void *wchan)
 int
 sleepq_wait_sig(void *wchan)
 {
+	int rcatch;
 	int rval;
 
-	sleepq_switch(wchan);
+	rcatch = sleepq_catch_signals(wchan);
+	if (rcatch == 0)
+		sleepq_switch(wchan);
+	else
+		sleepq_release(wchan);
 	rval = sleepq_check_signals();
 	mtx_unlock_spin(&sched_lock); 
+	if (rcatch)
+		return (rcatch);
 	return (rval);
 }
 
@@ -570,6 +554,7 @@ sleepq_timedwait(void *wchan)
 	int rval;
 
 	MPASS(!(curthread->td_flags & TDF_SINTR));
+	mtx_lock_spin(&sched_lock);
 	sleepq_switch(wchan);
 	rval = sleepq_check_timeout();
 	mtx_unlock_spin(&sched_lock);
@@ -581,18 +566,23 @@ sleepq_timedwait(void *wchan)
  * it is interrupted by a signal, or it times out waiting to be awakened.
  */
 int
-sleepq_timedwait_sig(void *wchan, int signal_caught)
+sleepq_timedwait_sig(void *wchan)
 {
-	int rvalt, rvals;
+	int rcatch, rvalt, rvals;
 
-	sleepq_switch(wchan);
+	rcatch = sleepq_catch_signals(wchan);
+	if (rcatch == 0)
+		sleepq_switch(wchan);
+	else
+		sleepq_release(wchan);
 	rvalt = sleepq_check_timeout();
 	rvals = sleepq_check_signals();
 	mtx_unlock_spin(&sched_lock);
-	if (signal_caught || rvalt == 0)
+	if (rcatch)
+		return (rcatch);
+	if (rvals)
 		return (rvals);
-	else
-		return (rvalt);
+	return (rvalt);
 }
 
 /*
@@ -825,13 +815,14 @@ sleepq_remove(struct thread *td, void *wchan)
  * Also, whatever the signal code does...
  */
 void
-sleepq_abort(struct thread *td)
+sleepq_abort(struct thread *td, int intrval)
 {
 	void *wchan;
 
 	mtx_assert(&sched_lock, MA_OWNED);
 	MPASS(TD_ON_SLEEPQ(td));
 	MPASS(td->td_flags & TDF_SINTR);
+	MPASS(intrval == EINTR || intrval == ERESTART);
 
 	/*
 	 * If the TDF_TIMEOUT flag is set, just leave. A
@@ -843,6 +834,10 @@ sleepq_abort(struct thread *td)
 	CTR3(KTR_PROC, "sleepq_abort: thread %p (pid %ld, %s)",
 	    (void *)td, (long)td->td_proc->p_pid, (void *)td->td_proc->p_comm);
 	wchan = td->td_wchan;
+	if (wchan != NULL) {
+		td->td_intrval = intrval;
+		td->td_flags |= TDF_SLEEPABORT;
+	}
 	mtx_unlock_spin(&sched_lock);
 	sleepq_remove(td, wchan);
 	mtx_lock_spin(&sched_lock);
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index a2a141c..5de9b97 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -343,7 +343,7 @@ struct thread {
 #define	TDF_TIMEOUT	0x00000010 /* Timing out during sleep. */
 #define	TDF_IDLETD	0x00000020 /* This is a per-CPU idle thread. */
 #define	TDF_SELECT	0x00000040 /* Selecting; wakeup/waiting danger. */
-#define	TDF_UNUSED7	0x00000080 /* --available -- */
+#define	TDF_SLEEPABORT	0x00000080 /* sleepq_abort was called. */
 #define	TDF_TSNOBLOCK	0x00000100 /* Don't block on a turnstile due to race. */
 #define	TDF_UNUSED9	0x00000200 /* --available -- */
 #define	TDF_BOUNDARY	0x00000400 /* Thread suspended at user boundary */
diff --git a/sys/sys/sleepqueue.h b/sys/sys/sleepqueue.h
index 7f36029..6bdc6ca 100644
--- a/sys/sys/sleepqueue.h
+++ b/sys/sys/sleepqueue.h
@@ -87,12 +87,10 @@ struct thread;
 #define	SLEEPQ_INTERRUPTIBLE	0x100		/* Sleep is interruptible. */
 
 void	init_sleepqueues(void);
-void	sleepq_abort(struct thread *td);
+void	sleepq_abort(struct thread *td, int intrval);
 void	sleepq_add(void *, struct mtx *, const char *, int);
 struct sleepqueue *sleepq_alloc(void);
 void	sleepq_broadcast(void *, int, int);
-int	sleepq_calc_signal_retval(int sig);
-int	sleepq_catch_signals(void *wchan);
 void	sleepq_free(struct sleepqueue *);
 void	sleepq_lock(void *);
 struct sleepqueue *sleepq_lookup(void *);
@@ -101,7 +99,7 @@ void	sleepq_remove(struct thread *, void *);
 void	sleepq_signal(void *, int, int);
 void	sleepq_set_timeout(void *wchan, int timo);
 int	sleepq_timedwait(void *wchan);
-int	sleepq_timedwait_sig(void *wchan, int signal_caught);
+int	sleepq_timedwait_sig(void *wchan);
 void	sleepq_wait(void *);
 int	sleepq_wait_sig(void *wchan);