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

The following reply was made to PR kern/60568; it has been noted by GNATS.

From: Taylor R Campbell <[email protected]>
To: "Greg A. Woods" <[email protected]>
Cc: [email protected], [email protected]
Subject: Re: kern/60568: panic locking against myself (p->p_lock) in NFS from sysctl_vmproc
Date: Mon, 10 Aug 2026 02:40:04 +0000

 This is a multi-part message in MIME format.
 --=_QSVhpQW7wL2t2ZyM0lNb6kI+m4HN6cIl
 
 Can you please try the attached patch?
 
 (Warning: untested on my end.  But if it makes your system panic,
 can't be worse than what you're already observing!)
 
 --=_QSVhpQW7wL2t2ZyM0lNb6kI+m4HN6cIl
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr60568-sysctlvmproclock"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr60568-sysctlvmproclock.patch"
 
 # 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)
  {
 =20
 -	/* XXXCDC: how should locking work here? */
 +	KASSERT(p =3D=3D curproc || mutex_owned(p->p_lock));
 =20
  	/* curproc exception is for coredump. */
 =20
 @@ -2946,7 +2946,20 @@ fill_kproc2(struct proc *p, struct kinfo
  	}
  }
 =20
 -
 +/*
 + * proc_find_reflocked(l, &p, pid, op)
 + *
 + *	Look up a process p by pid, taking l->l_proc if pid =3D=3D -1, and
 + *	take the mutex p->p_lock if pid !=3D -1.  Verify whether the
 + *	caller is allowed to see the target process before succeeding.
 + *	Return nonzero error on failure.
 + *
 + *	Invariant on successful return:
 + *
 + *		pid =3D=3D -1 || mutex_owned(p->p_lock)
 + *
 + *	Caller is responsible for mutex_exit(p->p_lock) if pid !=3D -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 =3D=3D NULL) {
  		if (pid !=3D -1)
  			mutex_exit(&proc_lock);
 -		return SET_ERROR(ESRCH);
 +		error =3D SET_ERROR(ESRCH);
 +		goto out;
  	}
  	if (pid !=3D -1)
  		mutex_enter((*p)->p_lock);
 @@ -2973,7 +2987,70 @@ proc_find_locked(struct lwp *l, struct p
  	if (error) {
  		if (pid !=3D -1)
  			mutex_exit((*p)->p_lock);
 +		goto out;
  	}
 +out:	KASSERT(error !=3D 0 || pid !=3D -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 =3D=3D -1, and
 + *	take _both_ the mutex p->p_lock _and_ p->p_reflock as a reader
 + *	or writer according to op if pid !=3D -1.  Verify whether the
 + *	caller is allowed to see the target process before succeeding.
 + *	Return nonzero error on failure.
 + *
 + *	Invariants on successful return:
 + *
 + *		pid =3D=3D -1 || mutex_owned(p->p_lock)
 + *		pid =3D=3D -1 || rw_lock_held(p->p_lock)
 + *		pid =3D=3D -1 || op !=3D RW_READER || rw_read_held(p->p_lock)
 + *		pid =3D=3D -1 || op !=3D 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 !=3D -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 =3D=3D -1)
 +		*p =3D l->l_proc;
 +	else
 +		*p =3D proc_find(pid);
 +
 +	if (*p =3D=3D NULL) {
 +		if (pid !=3D -1)
 +			mutex_exit(&proc_lock);
 +		error =3D SET_ERROR(ESRCH);
 +		goto out;
 +	}
 +	if (pid !=3D -1) {
 +		rw_enter(&(*p)->p_reflock, op);
 +		mutex_enter((*p)->p_lock);
 +	}
 +	mutex_exit(&proc_lock);
 +
 +	error =3D kauth_authorize_process(l->l_cred,
 +	    KAUTH_PROCESS_CANSEE, *p,
 +	    KAUTH_ARG(KAUTH_REQ_PROCESS_CANSEE_ENTRY), NULL, NULL);
 +	if (error) {
 +		if (pid !=3D -1) {
 +			mutex_exit((*p)->p_lock);
 +			rw_exit(&(*p)->p_reflock);
 +		}
 +		goto out;
 +	}
 +out:	KASSERT(error !=3D 0 || pid =3D=3D -1 || mutex_owned((*p)->p_lock));
 +	KASSERT(error !=3D 0 || pid =3D=3D -1 || rw_lock_held(&(*p)->p_reflock));
 +	KASSERT(error !=3D 0 || pid =3D=3D -1 || op !=3D RW_READER ||
 +	    rw_read_held(&(*p)->p_reflock));
 +	KASSERT(error !=3D 0 || pid =3D=3D -1 || op !=3D RW_WRITER ||
 +	    rw_write_held(&(*p)->p_reflock));
  	return error;
  }
 =20
 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[];
 =20
  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 =3D false;
 =20
  	if (elem_size =3D=3D 0 || elem_size > 2 * sizeof(*vme))
  		return EINVAL;
 @@ -5391,15 +5392,42 @@ fill_vmentries(struct lwp *l, pid_t pid,
  	} else
  		vmesize =3D 0;
 =20
 -	if ((error =3D proc_find_locked(l, &p, pid)) !=3D 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 =3D proc_find_reflocked(l, &p, pid, RW_READER)) !=3D 0)
  		return error;
 +	KASSERT(pid =3D=3D -1 || mutex_owned(p->p_lock));
 +	KASSERT(pid =3D=3D -1 || rw_read_held(&p->p_reflock));
 +	locked =3D true;
 =20
  	vme =3D NULL;
  	count =3D 0;
 =20
 +	/*
 +	 * 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 =3D proc_vmspace_getref(p, &vm)) !=3D 0)
  		goto out;
 -
 +	if (pid !=3D -1) {
 +		mutex_exit(p->p_lock);
 +		locked =3D false;
 +	}
 +
 +	/*
 +	 * Take a read lock on the VM map to iterate over it.
 +	 *
 +	 * XXX Should we kmem_alloc before locking the VM map?
 +	 */
  	map =3D &vm->vm_map;
  	vm_map_lock_read(map);
 =20
 @@ -5416,12 +5444,16 @@ fill_vmentries(struct lwp *l, pid_t pid,
  		}
  		count++;
  	}
 +
  	vm_map_unlock_read(map);
  	uvmspace_free(vm);
 =20
  out:
 -	if (pid !=3D -1)
 -		mutex_exit(p->p_lock);
 +	if (pid !=3D -1) {
 +		if (locked)
 +			mutex_exit(p->p_lock);
 +		rw_exit(&p->p_reflock);
 +	}
  	if (error =3D=3D 0) {
  		const u_int esize =3D uimin(sizeof(*vme), elem_size);
  		dp =3D oldp;
 
 --=_QSVhpQW7wL2t2ZyM0lNb6kI+m4HN6cIl--