Revert commitid: yfAefyNWibUyjkU2, ESyyH5EKxtrXGkS6 and itscfpFvJLOj8mHB;

The change to the single thread API results in crashes inside exit1()
as found by Syzkaller. There seems to be a race in the exit codepath.
What exactly fails is not really clear therefor revert for now.

This should fix the following Syzkaller reports:
Reported-by: syzbot+38efb425eada701ca8bb@syzkaller.appspotmail.com
Reported-by: syzbot+ecc0e8628b3db39b5b17@syzkaller.appspotmail.com
and maybe more.

Reverted commits:
----------------------------
Protect ps_single, ps_singlecnt and ps_threadcnt by the process mutex.

The single thread API needs to lock the process to enter single thread
mode and does not need to stop the scheduler.

This code changes ps_singlecount from a count down to zero to ps_singlecnt
which counts up until equal to ps_threadcnt (in which case all threads
are properly asleep).

Tested by phessler@, OK mpi@ cheloha@
----------------------------
Change how ps_threads and p_thr_link are locked away from using SCHED_LOCK.

The per process thread list can be traversed (read) by holding either
the KERNEL_LOCK or the per process ps_mtx (instead of SCHED_LOCK).
Abusing the SCHED_LOCK for this makes it impossible to split up the
scheduler lock into something more fine grained.

Tested by phessler@, ok mpi@
----------------------------
Fix SCHED_LOCK() leak in single_thread_set()

In the (q->p_flag & P_WEXIT) branch is a continue that did not release
the SCHED_LOCK. Refactor the code a bit to simplify the places SCHED_LOCK
is grabbed and released.

Reported-by: syzbot+ea26d351acfad3bb3f15@syzkaller.appspotmail.com
OK kettenis@

6 files changed, 64 insertions, 90 deletions

diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c
index 460833afc4f..4d7bfd5fddb 100644
--- a/sys/kern/kern_exit.c
+++ b/sys/kern/kern_exit.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_exit.c,v 1.214 2023/09/08 09:06:31 claudio Exp $	*/
+/*	$OpenBSD: kern_exit.c,v 1.215 2023/09/13 14:25:49 claudio Exp $	*/
 /*	$NetBSD: kern_exit.c,v 1.39 1996/04/22 01:38:25 christos Exp $	*/
 
 /*
@@ -119,7 +119,7 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
 	struct process *pr, *qr, *nqr;
 	struct rusage *rup;
 	struct timespec ts;
-	int s, wake;
+	int s;
 
 	atomic_setbits_int(&p->p_flag, P_WEXIT);
 
@@ -157,22 +157,14 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
 	}
 
 	/* unlink ourselves from the active threads */
-	mtx_enter(&pr->ps_mtx);
+	SCHED_LOCK(s);
 	TAILQ_REMOVE(&pr->ps_threads, p, p_thr_link);
-	pr->ps_threadcnt--;
-
-	wake = (pr->ps_single && pr->ps_singlecnt == pr->ps_threadcnt);
-	mtx_leave(&pr->ps_mtx);
-	if (wake)
-		wakeup(&pr->ps_singlecnt);
+	SCHED_UNLOCK(s);
 
 	if ((p->p_flag & P_THREAD) == 0) {
 		/* main thread gotta wait because it has the pid, et al */
-		mtx_enter(&pr->ps_mtx);
-		while (pr->ps_threadcnt > 0)
-			msleep_nsec(&pr->ps_threads, &pr->ps_mtx, PWAIT,
-			    "thrdeath", INFSLP);
-		mtx_leave(&pr->ps_mtx);
+		while (pr->ps_threadcnt > 1)
+			tsleep_nsec(&pr->ps_threads, PWAIT, "thrdeath", INFSLP);
 		if (pr->ps_flags & PS_PROFIL)
 			stopprofclock(pr);
 	}
@@ -345,10 +337,9 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
 	/* just a thread? detach it from its process */
 	if (p->p_flag & P_THREAD) {
 		/* scheduler_wait_hook(pr->ps_mainproc, p); XXX */
-		mtx_enter(&pr->ps_mtx);
-		if (pr->ps_threadcnt == 0)
+		if (--pr->ps_threadcnt == 1)
 			wakeup(&pr->ps_threads);
-		mtx_leave(&pr->ps_mtx);
+		KASSERT(pr->ps_threadcnt > 0);
 	}
 
 	/* Release the thread's read reference of resource limit structure. */
@@ -832,7 +823,7 @@ process_zap(struct process *pr)
 	if (otvp)
 		vrele(otvp);
 
-	KASSERT(pr->ps_threadcnt == 0);
+	KASSERT(pr->ps_threadcnt == 1);
 	if (pr->ps_ptstat != NULL)
 		free(pr->ps_ptstat, M_SUBPROC, sizeof(*pr->ps_ptstat));
 	pool_put(&rusage_pool, pr->ps_ru);
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index f3eca8918e6..5c6019409f3 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_fork.c,v 1.251 2023/09/08 09:06:31 claudio Exp $	*/
+/*	$OpenBSD: kern_fork.c,v 1.252 2023/09/13 14:25:49 claudio Exp $	*/
 /*	$NetBSD: kern_fork.c,v 1.29 1996/02/09 18:59:34 christos Exp $	*/
 
 /*
@@ -519,7 +519,7 @@ thread_fork(struct proc *curp, void *stack, void *tcb, pid_t *tidptr,
 	struct proc *p;
 	pid_t tid;
 	vaddr_t uaddr;
-	int error;
+	int s, error;
 
 	if (stack == NULL)
 		return EINVAL;
@@ -543,6 +543,7 @@ thread_fork(struct proc *curp, void *stack, void *tcb, pid_t *tidptr,
 
 	/* other links */
 	p->p_p = pr;
+	pr->ps_threadcnt++;
 
 	/* local copies */
 	p->p_fd		= pr->ps_fd;
@@ -561,17 +562,18 @@ thread_fork(struct proc *curp, void *stack, void *tcb, pid_t *tidptr,
 	LIST_INSERT_HEAD(&allproc, p, p_list);
 	LIST_INSERT_HEAD(TIDHASH(p->p_tid), p, p_hash);
 
-	mtx_enter(&pr->ps_mtx);
+	SCHED_LOCK(s);
 	TAILQ_INSERT_TAIL(&pr->ps_threads, p, p_thr_link);
-	pr->ps_threadcnt++;
 
 	/*
 	 * if somebody else wants to take us to single threaded mode,
 	 * count ourselves in.
 	 */
-	if (pr->ps_single)
+	if (pr->ps_single) {
+		atomic_inc_int(&pr->ps_singlecount);
 		atomic_setbits_int(&p->p_flag, P_SUSPSINGLE);
-	mtx_leave(&pr->ps_mtx);
+	}
+	SCHED_UNLOCK(s);
 
 	/*
 	 * Return tid to parent thread and copy it out to userspace
diff --git a/sys/kern/kern_resource.c b/sys/kern/kern_resource.c
index 2237ed4c749..9885396b811 100644
--- a/sys/kern/kern_resource.c
+++ b/sys/kern/kern_resource.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_resource.c,v 1.79 2023/09/08 09:06:31 claudio Exp $	*/
+/*	$OpenBSD: kern_resource.c,v 1.80 2023/09/13 14:25:49 claudio Exp $	*/
 /*	$NetBSD: kern_resource.c,v 1.38 1996/10/23 07:19:38 matthias Exp $	*/
 
 /*-
@@ -212,13 +212,11 @@ donice(struct proc *curp, struct process *chgpr, int n)
 	if (n < chgpr->ps_nice && suser(curp))
 		return (EACCES);
 	chgpr->ps_nice = n;
-	mtx_enter(&chgpr->ps_mtx);
+	SCHED_LOCK(s);
 	TAILQ_FOREACH(p, &chgpr->ps_threads, p_thr_link) {
-		SCHED_LOCK(s);
 		setpriority(p, p->p_estcpu, n);
-		SCHED_UNLOCK(s);
 	}
-	mtx_leave(&chgpr->ps_mtx);
+	SCHED_UNLOCK(s);
 	return (0);
 }
 
@@ -478,9 +476,8 @@ dogetrusage(struct proc *p, int who, struct rusage *rup)
 	struct process *pr = p->p_p;
 	struct proc *q;
 
-	KERNEL_ASSERT_LOCKED();
-
 	switch (who) {
+
 	case RUSAGE_SELF:
 		/* start with the sum of dead threads, if any */
 		if (pr->ps_ru != NULL)
diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index c2a153054d2..06b6a9e27d0 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_sig.c,v 1.316 2023/09/09 14:50:09 claudio Exp $	*/
+/*	$OpenBSD: kern_sig.c,v 1.317 2023/09/13 14:25:49 claudio Exp $	*/
 /*	$NetBSD: kern_sig.c,v 1.54 1996/04/22 01:38:32 christos Exp $	*/
 
 /*
@@ -2008,12 +2008,11 @@ userret(struct proc *p)
 }
 
 int
-single_thread_check_locked(struct proc *p, int deep)
+single_thread_check_locked(struct proc *p, int deep, int s)
 {
 	struct process *pr = p->p_p;
-	int s, wake;
 
-	MUTEX_ASSERT_LOCKED(&pr->ps_mtx);
+	SCHED_ASSERT_LOCKED();
 
 	if (pr->ps_single == NULL || pr->ps_single == p)
 		return (0);
@@ -2027,24 +2026,19 @@ single_thread_check_locked(struct proc *p, int deep)
 				return (EINTR);
 		}
 
+		if (atomic_dec_int_nv(&pr->ps_singlecount) == 0)
+			wakeup(&pr->ps_singlecount);
+
 		if (pr->ps_flags & PS_SINGLEEXIT) {
-			mtx_leave(&pr->ps_mtx);
+			SCHED_UNLOCK(s);
 			KERNEL_LOCK();
 			exit1(p, 0, 0, EXIT_THREAD_NOCHECK);
 			/* NOTREACHED */
 		}
 
 		/* not exiting and don't need to unwind, so suspend */
-		wake = (++pr->ps_singlecnt == pr->ps_threadcnt);
-		mtx_leave(&pr->ps_mtx);
-		if (wake)
-			wakeup(&pr->ps_singlecnt);
-
-		SCHED_LOCK(s);
 		p->p_stat = SSTOP;
 		mi_switch();
-		SCHED_UNLOCK(s);
-		mtx_enter(&pr->ps_mtx);
 	} while (pr->ps_single != NULL);
 
 	return (0);
@@ -2053,11 +2047,11 @@ single_thread_check_locked(struct proc *p, int deep)
 int
 single_thread_check(struct proc *p, int deep)
 {
-	int error;
+	int s, error;
 
-	mtx_enter(&p->p_p->ps_mtx);
-	error = single_thread_check_locked(p, deep);
-	mtx_leave(&p->p_p->ps_mtx);
+	SCHED_LOCK(s);
+	error = single_thread_check_locked(p, deep, s);
+	SCHED_UNLOCK(s);
 
 	return error;
 }
@@ -2077,14 +2071,13 @@ single_thread_set(struct proc *p, enum single_thread_mode mode, int wait)
 	struct process *pr = p->p_p;
 	struct proc *q;
 	int error, s;
-	u_int count = 0;
 
 	KASSERT(curproc == p);
 
-	mtx_enter(&pr->ps_mtx);
-	error = single_thread_check_locked(p, (mode == SINGLE_UNWIND));
+	SCHED_LOCK(s);
+	error = single_thread_check_locked(p, (mode == SINGLE_UNWIND), s);
 	if (error) {
-		mtx_leave(&pr->ps_mtx);
+		SCHED_UNLOCK(s);
 		return error;
 	}
 
@@ -2103,25 +2096,26 @@ single_thread_set(struct proc *p, enum single_thread_mode mode, int wait)
 		panic("single_thread_mode = %d", mode);
 #endif
 	}
-	pr->ps_singlecnt = 1; /* count ourselfs in already */
+	pr->ps_singlecount = 0;
+	membar_producer();
 	pr->ps_single = p;
-
 	TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
 		if (q == p)
 			continue;
 		if (q->p_flag & P_WEXIT) {
-			SCHED_LOCK(s);
-			if (mode == SINGLE_EXIT && q->p_stat == SSTOP)
-				setrunnable(q);
-			SCHED_UNLOCK(s);
+			if (mode == SINGLE_EXIT) {
+				if (q->p_stat == SSTOP) {
+					setrunnable(q);
+					atomic_inc_int(&pr->ps_singlecount);
+				}
+			}
 			continue;
 		}
-		SCHED_LOCK(s);
 		atomic_setbits_int(&q->p_flag, P_SUSPSINGLE);
 		switch (q->p_stat) {
 		case SIDL:
-		case SDEAD:
 		case SRUN:
+			atomic_inc_int(&pr->ps_singlecount);
 			break;
 		case SSLEEP:
 			/* if it's not interruptible, then just have to wait */
@@ -2129,29 +2123,28 @@ single_thread_set(struct proc *p, enum single_thread_mode mode, int wait)
 				/* merely need to suspend?  just stop it */
 				if (mode == SINGLE_SUSPEND) {
 					q->p_stat = SSTOP;
-					count++;
 					break;
 				}
 				/* need to unwind or exit, so wake it */
 				setrunnable(q);
 			}
+			atomic_inc_int(&pr->ps_singlecount);
 			break;
 		case SSTOP:
 			if (mode == SINGLE_EXIT) {
 				setrunnable(q);
-				break;
+				atomic_inc_int(&pr->ps_singlecount);
 			}
-			count++;
+			break;
+		case SDEAD:
 			break;
 		case SONPROC:
+			atomic_inc_int(&pr->ps_singlecount);
 			signotify(q);
 			break;
 		}
-		SCHED_UNLOCK(s);
 	}
-
-	pr->ps_singlecnt += count;
-	mtx_leave(&pr->ps_mtx);
+	SCHED_UNLOCK(s);
 
 	if (wait)
 		single_thread_wait(pr, 1);
@@ -2170,16 +2163,14 @@ single_thread_wait(struct process *pr, int recheck)
 	int wait;
 
 	/* wait until they're all suspended */
-	mtx_enter(&pr->ps_mtx);
-	wait = pr->ps_singlecnt < pr->ps_threadcnt;
+	wait = pr->ps_singlecount > 0;
 	while (wait) {
-		msleep_nsec(&pr->ps_singlecnt, &pr->ps_mtx, PWAIT, "suspend",
-		    INFSLP);
+		sleep_setup(&pr->ps_singlecount, PWAIT, "suspend");
+		wait = pr->ps_singlecount > 0;
+		sleep_finish(0, wait);
 		if (!recheck)
 			break;
-		wait = pr->ps_singlecnt < pr->ps_threadcnt;
 	}
-	mtx_leave(&pr->ps_mtx);
 
 	return wait;
 }
@@ -2194,10 +2185,9 @@ single_thread_clear(struct proc *p, int flag)
 	KASSERT(pr->ps_single == p);
 	KASSERT(curproc == p);
 
-	mtx_enter(&pr->ps_mtx);
+	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) {
 		if (q == p || (q->p_flag & P_SUSPSINGLE) == 0)
 			continue;
@@ -2208,7 +2198,6 @@ 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 == NULL)
 				setrunnable(q);
@@ -2217,9 +2206,8 @@ single_thread_clear(struct proc *p, int flag)
 				q->p_stat = SSLEEP;
 			}
 		}
-		SCHED_UNLOCK(s);
 	}
-	mtx_leave(&pr->ps_mtx);
+	SCHED_UNLOCK(s);
 }
 
 void
diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
index 06db10f0488..b4f0c274234 100644
--- a/sys/kern/kern_synch.c
+++ b/sys/kern/kern_synch.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_synch.c,v 1.199 2023/09/08 09:06:31 claudio Exp $	*/
+/*	$OpenBSD: kern_synch.c,v 1.200 2023/09/13 14:25:49 claudio Exp $	*/
 /*	$NetBSD: kern_synch.c,v 1.37 1996/04/22 01:38:37 christos Exp $	*/
 
 /*
@@ -566,18 +566,15 @@ sys_sched_yield(struct proc *p, void *v, register_t *retval)
 	uint8_t newprio;
 	int s;
 
+	SCHED_LOCK(s);
 	/*
 	 * If one of the threads of a multi-threaded process called
 	 * sched_yield(2), drop its priority to ensure its siblings
 	 * can make some progress.
 	 */
-	mtx_enter(&p->p_p->ps_mtx);
 	newprio = p->p_usrpri;
 	TAILQ_FOREACH(q, &p->p_p->ps_threads, p_thr_link)
 		newprio = max(newprio, q->p_runpri);
-	mtx_leave(&p->p_p->ps_mtx);
-
-	SCHED_LOCK(s);
 	setrunqueue(p->p_cpu, p, newprio);
 	p->p_ru.ru_nvcsw++;
 	mi_switch();
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 9484a8bdb16..34104386f6f 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: proc.h,v 1.350 2023/09/08 09:06:31 claudio Exp $	*/
+/*	$OpenBSD: proc.h,v 1.351 2023/09/13 14:25:49 claudio Exp $	*/
 /*	$NetBSD: proc.h,v 1.44 1996/04/22 01:23:21 christos Exp $	*/
 
 /*-
@@ -138,7 +138,7 @@ struct process {
 	struct	ucred *ps_ucred;	/* Process owner's identity. */
 
 	LIST_ENTRY(process) ps_list;	/* List of all processes. */
-	TAILQ_HEAD(,proc) ps_threads;	/* [K|m] Threads in this process. */
+	TAILQ_HEAD(,proc) ps_threads;	/* [K|S] Threads in this process. */
 
 	LIST_ENTRY(process) ps_pglist;	/* List of processes in pgrp. */
 	struct	process *ps_pptr; 	/* Pointer to parent process. */
@@ -173,8 +173,8 @@ struct process {
 	u_int	ps_flags;		/* [a] PS_* flags. */
 	int	ps_siglist;		/* Signals pending for the process. */
 
-	struct	proc *ps_single;	/* [m] Thread for single-threading. */
-	u_int	ps_singlecnt;		/* [m] Number of suspended threads. */
+	struct	proc *ps_single;	/* [S] Thread for single-threading. */
+	u_int	ps_singlecount;		/* [a] Not yet suspended threads. */
 
 	int	ps_traceflag;		/* Kernel trace points. */
 	struct	vnode *ps_tracevp;	/* Trace to vnode. */
@@ -242,7 +242,7 @@ struct process {
 
 /* End area that is copied on creation. */
 #define ps_endcopy	ps_threadcnt
-	u_int	ps_threadcnt;		/* [m] Number of threads. */
+	u_int	ps_threadcnt;		/* Number of threads. */
 
 	struct	timespec ps_start;	/* starting uptime. */
 	struct	timeout ps_realit_to;	/* [m] ITIMER_REAL timeout */
@@ -310,14 +310,13 @@ struct p_inentry {
  *	U	uidinfolk
  *	l	read only reference, see lim_read_enter()
  *	o	owned (read/modified only) by this thread
- *	m	this proc's' `p->p_p->ps_mtx'
  */
 struct proc {
 	TAILQ_ENTRY(proc) p_runq;	/* [S] current run/sleep queue */
 	LIST_ENTRY(proc) p_list;	/* List of all threads. */
 
 	struct	process *p_p;		/* [I] The process of this thread. */
-	TAILQ_ENTRY(proc) p_thr_link;	/* [K|m] Threads in a process linkage. */
+	TAILQ_ENTRY(proc) p_thr_link;	/* Threads in a process linkage. */
 
 	TAILQ_ENTRY(proc) p_fut_link;	/* Threads in a futex linkage. */
 	struct	futex	*p_futex;	/* Current sleeping futex. */