Re: kern/60568: panic locking against myself (p->p_lock) in NFS from sysctl_vmproc

# HG changeset patch
# User Taylor R Campbell <[email protected]>
# Date 1786327290 0
#      Mon Aug 10 02:01:30 2026 +0000
# Branch trunk
# Node ID 627f345a67f1beb9c8d462266402bb4226768c88
# Parent  b125bad3265e13dfd5a772b3676744fca85ce488
# EXP-Topic riastradh-pr60568-sysctlvmproclock
WIP: sysctl vm.proc.*: Fix locking protocol.

Take p_lock only long enough for proc_vmspace_getref, and then rely
only on a read lock on p_reflock.

New subroutine proc_find_reflocked is like proc_find_locked but also
returns with p_reflock held, taking locks in the correct order
(proc_lock -> p_reflock -> p_lock) to make this work safely.  (It
could just as well return with only p_reflock held, not p_lock, but
it presumably needs p_lock for KAUTH_PROCESS_CANSEE, so why bother
unlocking and relocking in the caller immediately?)

PR kern/60568: panic locking against myself (p->p_lock) in NFS from
sysctl_vmproc

diff -r b125bad3265e -r 627f345a67f1 sys/kern/kern_proc.c
--- a/sys/kern/kern_proc.c	Mon Aug 03 19:24:48 2026 +0000
+++ b/sys/kern/kern_proc.c	Mon Aug 10 02:01:30 2026 +0000
@@ -1789,7 +1789,7 @@ int
 proc_vmspace_getref(struct proc *p, struct vmspace **vm)
 {
 
-	/* XXXCDC: how should locking work here? */
+	KASSERT(p == curproc || mutex_owned(p->p_lock));
 
 	/* curproc exception is for coredump. */
 
@@ -2946,7 +2946,20 @@ fill_kproc2(struct proc *p, struct kinfo
 	}
 }
 
-
+/*
+ * proc_find_reflocked(l, &p, pid, op)
+ *
+ *	Look up a process p by pid, taking l->l_proc if pid == -1, and
+ *	take the mutex p->p_lock if pid != -1.  Verify whether the
+ *	caller is allowed to see the target process before succeeding.
+ *	Return nonzero error on failure.
+ *
+ *	Invariant on successful return:
+ *
+ *		pid == -1 || mutex_owned(p->p_lock)
+ *
+ *	Caller is responsible for mutex_exit(p->p_lock) if pid != -1.
+ */
 int
 proc_find_locked(struct lwp *l, struct proc **p, pid_t pid)
 {
@@ -2961,7 +2974,8 @@ proc_find_locked(struct lwp *l, struct p
 	if (*p == NULL) {
 		if (pid != -1)
 			mutex_exit(&proc_lock);
-		return SET_ERROR(ESRCH);
+		error = SET_ERROR(ESRCH);
+		goto out;
 	}
 	if (pid != -1)
 		mutex_enter((*p)->p_lock);
@@ -2973,7 +2987,70 @@ proc_find_locked(struct lwp *l, struct p
 	if (error) {
 		if (pid != -1)
 			mutex_exit((*p)->p_lock);
+		goto out;
 	}
+out:	KASSERT(error != 0 || pid != -1 || mutex_owned((*p)->p_lock));
+	return error;
+}
+
+/*
+ * proc_find_reflocked(l, &p, pid, op)
+ *
+ *	Look up a process p by pid, taking l->l_proc if pid == -1, and
+ *	take _both_ the mutex p->p_lock _and_ p->p_reflock as a reader
+ *	or writer according to op if pid != -1.  Verify whether the
+ *	caller is allowed to see the target process before succeeding.
+ *	Return nonzero error on failure.
+ *
+ *	Invariants on successful return:
+ *
+ *		pid == -1 || mutex_owned(p->p_lock)
+ *		pid == -1 || rw_lock_held(p->p_lock)
+ *		pid == -1 || op != RW_READER || rw_read_held(p->p_lock)
+ *		pid == -1 || op != RW_WRITER || rw_write_held(p->p_lock)
+ *
+ *	Caller is responsible for mutex_exit(p->p_lock) _and_
+ *	rw_exit(&p->p_reflock) if pid != -1.
+ */
+int
+proc_find_reflocked(struct lwp *l, struct proc **p, pid_t pid, krw_t op)
+{
+	int error;
+
+	mutex_enter(&proc_lock);
+	if (pid == -1)
+		*p = l->l_proc;
+	else
+		*p = proc_find(pid);
+
+	if (*p == NULL) {
+		if (pid != -1)
+			mutex_exit(&proc_lock);
+		error = SET_ERROR(ESRCH);
+		goto out;
+	}
+	if (pid != -1) {
+		rw_enter(&(*p)->p_reflock, op);
+		mutex_enter((*p)->p_lock);
+	}
+	mutex_exit(&proc_lock);
+
+	error = kauth_authorize_process(l->l_cred,
+	    KAUTH_PROCESS_CANSEE, *p,
+	    KAUTH_ARG(KAUTH_REQ_PROCESS_CANSEE_ENTRY), NULL, NULL);
+	if (error) {
+		if (pid != -1) {
+			mutex_exit((*p)->p_lock);
+			rw_exit(&(*p)->p_reflock);
+		}
+		goto out;
+	}
+out:	KASSERT(error != 0 || pid == -1 || mutex_owned((*p)->p_lock));
+	KASSERT(error != 0 || pid == -1 || rw_lock_held(&(*p)->p_reflock));
+	KASSERT(error != 0 || pid == -1 || op != RW_READER ||
+	    rw_read_held(&(*p)->p_reflock));
+	KASSERT(error != 0 || pid == -1 || op != RW_WRITER ||
+	    rw_write_held(&(*p)->p_reflock));
 	return error;
 }
 
diff -r b125bad3265e -r 627f345a67f1 sys/sys/proc.h
--- a/sys/sys/proc.h	Mon Aug 03 19:24:48 2026 +0000
+++ b/sys/sys/proc.h	Mon Aug 10 02:01:30 2026 +0000
@@ -496,6 +496,8 @@ extern struct proc	*initproc;	/* Process
 extern const struct proclist_desc proclists[];
 
 int		proc_find_locked(struct lwp *, struct proc **, pid_t);
+int		proc_find_reflocked(struct lwp *, struct proc **, pid_t,
+		    krw_t);
 proc_t *	proc_find_raw(pid_t);
 proc_t *	proc_find(pid_t);		/* Find process by ID */
 proc_t *	proc_find_lwpid(pid_t);		/* Find process by LWP ID */
diff -r b125bad3265e -r 627f345a67f1 sys/uvm/uvm_map.c
--- a/sys/uvm/uvm_map.c	Mon Aug 03 19:24:48 2026 +0000
+++ b/sys/uvm/uvm_map.c	Mon Aug 10 02:01:30 2026 +0000
@@ -5377,6 +5377,7 @@ fill_vmentries(struct lwp *l, pid_t pid,
 	struct vm_map_entry *entry;
 	char *dp;
 	size_t count, vmesize;
+	bool locked = false;
 
 	if (elem_size == 0 || elem_size > 2 * sizeof(*vme))
 		return EINVAL;
@@ -5391,15 +5392,42 @@ fill_vmentries(struct lwp *l, pid_t pid,
 	} else
 		vmesize = 0;
 
-	if ((error = proc_find_locked(l, &p, pid)) != 0)
+	/*
+	 * Look up the pid and:
+	 *
+	 * 1. take p->p_lock so we can proc_vmspace_getref,
+	 *
+	 * 2. take a read lock on p->p_reflock so the process cannot
+	 *    exit while we're doing things that can't hold p->p_lock.
+	 */
+	if ((error = proc_find_reflocked(l, &p, pid, RW_READER)) != 0)
 		return error;
+	KASSERT(pid == -1 || mutex_owned(p->p_lock));
+	KASSERT(pid == -1 || rw_read_held(&p->p_reflock));
+	locked = true;
 
 	vme = NULL;
 	count = 0;
 
+	/*
+	 * Get a reference to the vmspace while we still hold
+	 * p->p_lock.  Then release p->p_lock so we can safely allocate
+	 * memory, do vnode_to_path, &c., without deadlocking against
+	 * the page daemon or proc lookup shenanigans (e.g., nfs intr
+	 * signal catching) in vnode_to_path.
+	 */
 	if ((error = proc_vmspace_getref(p, &vm)) != 0)
 		goto out;
-
+	if (pid != -1) {
+		mutex_exit(p->p_lock);
+		locked = false;
+	}
+
+	/*
+	 * Take a read lock on the VM map to iterate over it.
+	 *
+	 * XXX Should we kmem_alloc before locking the VM map?
+	 */
 	map = &vm->vm_map;
 	vm_map_lock_read(map);
 
@@ -5416,12 +5444,16 @@ fill_vmentries(struct lwp *l, pid_t pid,
 		}
 		count++;
 	}
+
 	vm_map_unlock_read(map);
 	uvmspace_free(vm);
 
 out:
-	if (pid != -1)
-		mutex_exit(p->p_lock);
+	if (pid != -1) {
+		if (locked)
+			mutex_exit(p->p_lock);
+		rw_exit(&p->p_reflock);
+	}
 	if (error == 0) {
 		const u_int esize = uimin(sizeof(*vme), elem_size);
 		dp = oldp;