From: mvs <mvs@openbsd.org>
Date: Sun, 11 Aug 2024 15:10:53 +0000 (+0000)
Subject: Make exit1() wait sysctl(2) `allprocess' loops.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=1636884352af65901b58f3fdffa7015e5a90939f;p=openbsd

Make exit1() wait sysctl(2) `allprocess' loops.

Regardless on wired userland memory, KERN_FILE_BYPID and KERN_FILE_BYUID
`allprocess' loops have netlock provided sleep points, so concurrent
process exit(1) could crash kernel.

The main exit1() problem is that process teardown begins while process
is still linked to `allprocess' list, and current code doesn't allow to
unlink it first. Wait for concurrent sysctl(2) `allprocess' loops
between PS_EXITING bit setting and list unlinking. Both KERN_FILE_BYPID
and KERN_FILE_BYUID loops do PS_EXITING check and won't deal with dying
process. Concurrent exit1() thread will wait loops keeping process
linked to `allprocess' list.

Tested with i386 dpb(1) run.
Stress tests and ok bluhm.
---

diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c
index 280af8ce006..a1d1456328d 100644
--- a/sys/kern/kern_exit.c
+++ b/sys/kern/kern_exit.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_exit.c,v 1.230 2024/08/06 18:41:20 claudio Exp $	*/
+/*	$OpenBSD: kern_exit.c,v 1.231 2024/08/11 15:10:53 mvs Exp $	*/
 /*	$NetBSD: kern_exit.c,v 1.39 1996/04/22 01:38:25 christos Exp $	*/
 
 /*
@@ -151,6 +151,9 @@ exit1(struct proc *p, int xexit, int xsig, int flags)
 			    PS_ISPWAIT);
 			wakeup(pr->ps_pptr);
 		}
+
+		/* Wait for concurrent `allprocess' loops */
+		refcnt_finalize(&pr->ps_refcnt, "psdtor");
 	}
 
 	/* unlink ourselves from the active threads */
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 52a7c2cd1b6..e148c814380 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_fork.c,v 1.261 2024/08/06 08:44:54 claudio Exp $	*/
+/*	$OpenBSD: kern_fork.c,v 1.262 2024/08/11 15:10:53 mvs Exp $	*/
 /*	$NetBSD: kern_fork.c,v 1.29 1996/02/09 18:59:34 christos Exp $	*/
 
 /*
@@ -178,6 +178,8 @@ thread_new(struct proc *parent, vaddr_t uaddr)
 void
 process_initialize(struct process *pr, struct proc *p)
 {
+	refcnt_init(&pr->ps_refcnt);
+
 	/* initialize the thread links */
 	pr->ps_mainproc = p;
 	TAILQ_INIT(&pr->ps_threads);
diff --git a/sys/kern/kern_sysctl.c b/sys/kern/kern_sysctl.c
index 74b697ec706..8cb477265fd 100644
--- a/sys/kern/kern_sysctl.c
+++ b/sys/kern/kern_sysctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_sysctl.c,v 1.436 2024/08/08 15:02:36 bluhm Exp $	*/
+/*	$OpenBSD: kern_sysctl.c,v 1.437 2024/08/11 15:10:53 mvs Exp $	*/
 /*	$NetBSD: kern_sysctl.c,v 1.17 1996/05/20 17:49:05 mrg Exp $	*/
 
 /*-
@@ -1686,6 +1686,9 @@ sysctl_file(int *name, u_int namelen, char *where, size_t *sizep,
 				/* not the pid we are looking for */
 				continue;
 			}
+
+			refcnt_take(&pr->ps_refcnt);
+
 			matched = 1;
 			fdp = pr->ps_fd;
 			if (pr->ps_textvp)
@@ -1702,6 +1705,9 @@ sysctl_file(int *name, u_int namelen, char *where, size_t *sizep,
 				FILLIT(fp, fdp, i, NULL, pr);
 				FRELE(fp, p);
 			}
+
+			refcnt_rele_wake(&pr->ps_refcnt);
+
 			/* pid is unique, stop searching */
 			if (arg >= 0)
 				break;
@@ -1721,6 +1727,9 @@ sysctl_file(int *name, u_int namelen, char *where, size_t *sizep,
 				/* not the uid we are looking for */
 				continue;
 			}
+
+			refcnt_take(&pr->ps_refcnt);
+
 			fdp = pr->ps_fd;
 			if (fdp->fd_cdir)
 				FILLIT(NULL, NULL, KERN_FILE_CDIR, fdp->fd_cdir, pr);
@@ -1734,6 +1743,8 @@ sysctl_file(int *name, u_int namelen, char *where, size_t *sizep,
 				FILLIT(fp, fdp, i, NULL, pr);
 				FRELE(fp, p);
 			}
+
+			refcnt_rele_wake(&pr->ps_refcnt);
 		}
 		break;
 	default:
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 8527bb1f623..ad025b1da15 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: proc.h,v 1.367 2024/08/06 08:44:54 claudio Exp $	*/
+/*	$OpenBSD: proc.h,v 1.368 2024/08/11 15:10:53 mvs Exp $	*/
 /*	$NetBSD: proc.h,v 1.44 1996/04/22 01:23:21 christos Exp $	*/
 
 /*-
@@ -142,6 +142,8 @@ struct pinsyscall {
  *	T	itimer_mtx
  */
 struct process {
+	struct refcnt ps_refcnt;
+
 	/*
 	 * ps_mainproc is the original thread in the process.
 	 * It's only still special for the handling of