From 28d2efa9830c6c4673833ad709bbc4246c4534f0 Mon Sep 17 00:00:00 2001
From: Eric Badger <badger@FreeBSD.org>
Date: Tue, 14 Feb 2017 17:13:23 +0000
Subject: [PATCH] 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
---
sys/kern/subr_sleepqueue.c | 101 +++++++++++++++++++++----------------
1 file changed, 57 insertions(+), 44 deletions(-)
diff --git a/sys/kern/subr_sleepqueue.c b/sys/kern/subr_sleepqueue.c
index 157283078288..8a7f4e7ef9ad 100644
--- a/sys/kern/subr_sleepqueue.c
+++ b/sys/kern/subr_sleepqueue.c
@@ -430,6 +430,7 @@ sleepq_catch_signals(void *wchan, int pri)
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 pri)
}
/*
- * 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);
+ 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);
}
- 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);
- }
- /*
- * 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);