Skip site navigation (1)Skip section navigation (2) 
Date:      Tue, 14 Feb 2017 17:13:23 +0000 (UTC)
From:      Eric Badger <badger@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r313733 - head/sys/kern
Message-ID:  <201702141713.v1EHDNo1051757@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: badger
Date: Tue Feb 14 17:13:23 2017
New Revision: 313733
URL: https://svnweb.freebsd.org/changeset/base/313733

Log:
  sleepq_catch_signals: do thread suspension before signal check
  
  Since locks are dropped when a thread suspends, it's possible for another
  thread to deliver a signal to the suspended thread. If the thread awakens from
  suspension without checking for signals, it may go to sleep despite having
  a pending signal that should wake it up. Therefore the suspension check is
  done first, so any signals sent while suspended will be caught in the
  subsequent signal check.
  
  Reviewed by:	kib
  Approved by:	kib (mentor)
  MFC after:	2 weeks
  Sponsored by:	Dell EMC
  Differential Revision:	https://reviews.freebsd.org/D9530

Modified:
  head/sys/kern/subr_sleepqueue.c

Modified: head/sys/kern/subr_sleepqueue.c
==============================================================================
--- head/sys/kern/subr_sleepqueue.c	Tue Feb 14 16:49:32 2017	(r313732)
+++ head/sys/kern/subr_sleepqueue.c	Tue Feb 14 17:13:23 2017	(r313733)
@@ -430,6 +430,7 @@ sleepq_catch_signals(void *wchan, int pr
 	struct sigacts *ps;
 	int sig, ret;
 
+	ret = 0;
 	td = curthread;
 	p = curproc;
 	sc = SC_LOOKUP(wchan);
@@ -443,53 +444,65 @@ sleepq_catch_signals(void *wchan, int pr
 	}
 
 	/*
-	 * See if there are any pending signals for this thread.  If not
-	 * we can switch immediately.  Otherwise do the signal processing
-	 * directly.
+	 * See if there are any pending signals or suspension requests for this
+	 * thread.  If not, we can switch immediately.
 	 */
 	thread_lock(td);
-	if ((td->td_flags & (TDF_NEEDSIGCHK | TDF_NEEDSUSPCHK)) == 0) {
-		sleepq_switch(wchan, pri);
-		return (0);
-	}
-	thread_unlock(td);
-	mtx_unlock_spin(&sc->sc_lock);
-	CTR3(KTR_PROC, "sleepq catching signals: thread %p (pid %ld, %s)",
-		(void *)td, (long)p->p_pid, td->td_name);
-	PROC_LOCK(p);
-	ps = p->p_sigacts;
-	mtx_lock(&ps->ps_mtx);
-	sig = cursig(td);
-	if (sig == -1) {
-		mtx_unlock(&ps->ps_mtx);
-		KASSERT((td->td_flags & TDF_SBDRY) != 0, ("lost TDF_SBDRY"));
-		KASSERT(TD_SBDRY_INTR(td),
-		    ("lost TDF_SERESTART of TDF_SEINTR"));
-		KASSERT((td->td_flags & (TDF_SEINTR | TDF_SERESTART)) !=
-		    (TDF_SEINTR | TDF_SERESTART),
-		    ("both TDF_SEINTR and TDF_SERESTART"));
-		ret = TD_SBDRY_ERRNO(td);
-	} else 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 ((td->td_flags & (TDF_NEEDSIGCHK | TDF_NEEDSUSPCHK)) != 0) {
+		thread_unlock(td);
+		mtx_unlock_spin(&sc->sc_lock);
+		CTR3(KTR_PROC, "sleepq catching signals: thread %p (pid %ld, %s)",
+			(void *)td, (long)p->p_pid, td->td_name);
+		PROC_LOCK(p);
+		/*
+		 * Check for suspension first. Checking for signals and then
+		 * suspending could result in a missed signal, since a signal
+		 * can be delivered while this thread is suspended.
+		 */
+		if ((td->td_flags & TDF_NEEDSUSPCHK) != 0) {
+			ret = thread_suspend_check(1);
+			MPASS(ret == 0 || ret == EINTR || ret == ERESTART);
+			if (ret != 0) {
+				PROC_UNLOCK(p);
+				mtx_lock_spin(&sc->sc_lock);
+				thread_lock(td);
+				goto out;
+			}
+		}
+		if ((td->td_flags & TDF_NEEDSIGCHK) != 0) {
+			ps = p->p_sigacts;
+			mtx_lock(&ps->ps_mtx);
+			sig = cursig(td);
+			if (sig == -1) {
+				mtx_unlock(&ps->ps_mtx);
+				KASSERT((td->td_flags & TDF_SBDRY) != 0,
+				    ("lost TDF_SBDRY"));
+				KASSERT(TD_SBDRY_INTR(td),
+				    ("lost TDF_SERESTART of TDF_SEINTR"));
+				KASSERT((td->td_flags &
+				    (TDF_SEINTR | TDF_SERESTART)) !=
+				    (TDF_SEINTR | TDF_SERESTART),
+				    ("both TDF_SEINTR and TDF_SERESTART"));
+				ret = TD_SBDRY_ERRNO(td);
+			} else if (sig != 0) {
+				ret = SIGISMEMBER(ps->ps_sigintr, sig) ?
+				    EINTR : ERESTART;
+				mtx_unlock(&ps->ps_mtx);
+			} else {
+				mtx_unlock(&ps->ps_mtx);
+			}
+		}
+		/*
+		 * Lock the per-process spinlock prior to dropping the PROC_LOCK
+		 * to avoid a signal delivery race.  PROC_LOCK, PROC_SLOCK, and
+		 * thread_lock() are currently held in tdsendsignal().
+		 */
+		PROC_SLOCK(p);
+		mtx_lock_spin(&sc->sc_lock);
+		PROC_UNLOCK(p);
+		thread_lock(td);
+		PROC_SUNLOCK(p);
 	}
-	/*
-	 * Lock the per-process spinlock prior to dropping the PROC_LOCK
-	 * to avoid a signal delivery race.  PROC_LOCK, PROC_SLOCK, and
-	 * thread_lock() are currently held in tdsendsignal().
-	 */
-	PROC_SLOCK(p);
-	mtx_lock_spin(&sc->sc_lock);
-	PROC_UNLOCK(p);
-	thread_lock(td);
-	PROC_SUNLOCK(p);
 	if (ret == 0) {
 		sleepq_switch(wchan, pri);
 		return (0);

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201702141713.v1EHDNo1051757>