Fix a race in single-thread mode switching

Extend the scope of SCHED_LOCK() to better synchronize
single_thread_set(), single_thread_clear() and single_thread_check().
This prevents threads from suspending before single_thread_set() has
finished. If a thread suspended early, ps_singlecount might get
decremented too much, which in turn could make single_thread_wait()
get stuck.

The race could be triggered for example by trying to stop
a multithreaded process with a debugger. When triggered, the race
prevents the debugger from finishing a wait4(2) call on the debuggee.
This kind of gdb hang was reported by Julian Smith on misc@.

Unfortunately, single-thread mode switching still has issues and hangs
are still possible.

OK mpi@

1 files changed, 15 insertions, 14 deletions

diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 6c1c0b5caa0..1900d9044c6 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_sig.c,v 1.259 2020/08/19 10:10:57 mpi Exp $	*/
+/*	$OpenBSD: kern_sig.c,v 1.260 2020/08/26 03:16:53 visa Exp $	*/
 /*	$NetBSD: kern_sig.c,v 1.54 1996/04/22 01:38:32 christos Exp $	*/
 
 /*
@@ -1917,16 +1917,22 @@ single_thread_check(struct proc *p, int deep)
 					return (EINTR);
 			}
 
+			SCHED_LOCK(s);
+			if (pr->ps_single == NULL) {
+				SCHED_UNLOCK(s);
+				continue;
+			}
+
 			if (atomic_dec_int_nv(&pr->ps_singlecount) == 0)
 				wakeup(&pr->ps_singlecount);
 			if (pr->ps_flags & PS_SINGLEEXIT) {
+				SCHED_UNLOCK(s);
 				KERNEL_LOCK();
 				exit1(p, 0, 0, EXIT_THREAD_NOCHECK);
-				KERNEL_UNLOCK();
+				/* NOTREACHED */
 			}
 
 			/* not exiting and don't need to unwind, so suspend */
-			SCHED_LOCK(s);
 			p->p_stat = SSTOP;
 			mi_switch();
 			SCHED_UNLOCK(s);
@@ -1952,7 +1958,7 @@ single_thread_set(struct proc *p, enum single_thread_mode mode, int deep)
 {
 	struct process *pr = p->p_p;
 	struct proc *q;
-	int error;
+	int error, s;
 
 	KERNEL_ASSERT_LOCKED();
 	KASSERT(curproc == p);
@@ -1976,26 +1982,22 @@ single_thread_set(struct proc *p, enum single_thread_mode mode, int deep)
 		panic("single_thread_mode = %d", mode);
 #endif
 	}
+	SCHED_LOCK(s);
 	pr->ps_singlecount = 0;
 	membar_producer();
 	pr->ps_single = p;
 	TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
-		int s;
-
 		if (q == p)
 			continue;
 		if (q->p_flag & P_WEXIT) {
 			if (mode == SINGLE_EXIT) {
-				SCHED_LOCK(s);
 				if (q->p_stat == SSTOP) {
 					setrunnable(q);
 					atomic_inc_int(&pr->ps_singlecount);
 				}
-				SCHED_UNLOCK(s);
 			}
 			continue;
 		}
-		SCHED_LOCK(s);
 		atomic_setbits_int(&q->p_flag, P_SUSPSINGLE);
 		switch (q->p_stat) {
 		case SIDL:
@@ -2029,8 +2031,8 @@ single_thread_set(struct proc *p, enum single_thread_mode mode, int deep)
 			signotify(q);
 			break;
 		}
-		SCHED_UNLOCK(s);
 	}
+	SCHED_UNLOCK(s);
 
 	if (mode != SINGLE_PTRACE)
 		single_thread_wait(pr, 1);
@@ -2067,16 +2069,16 @@ single_thread_clear(struct proc *p, int flag)
 {
 	struct process *pr = p->p_p;
 	struct proc *q;
+	int s;
 
 	KASSERT(pr->ps_single == p);
 	KASSERT(curproc == p);
 	KERNEL_ASSERT_LOCKED();
 
+	SCHED_LOCK(s);
 	pr->ps_single = NULL;
 	atomic_clearbits_int(&pr->ps_flags, PS_SINGLEUNWIND | PS_SINGLEEXIT);
 	TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
-		int s;
-
 		if (q == p || (q->p_flag & P_SUSPSINGLE) == 0)
 			continue;
 		atomic_clearbits_int(&q->p_flag, P_SUSPSINGLE);
@@ -2086,15 +2088,14 @@ single_thread_clear(struct proc *p, int flag)
 		 * then clearing that either makes it runnable or puts
 		 * it back into some sleep queue
 		 */
-		SCHED_LOCK(s);
 		if (q->p_stat == SSTOP && (q->p_flag & flag) == 0) {
 			if (q->p_wchan == 0)
 				setrunnable(q);
 			else
 				q->p_stat = SSLEEP;
 		}
-		SCHED_UNLOCK(s);
 	}
+	SCHED_UNLOCK(s);
 }
 
 void