NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
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 <riastradh%NetBSD.org@localhost>
To: "Greg A. Woods" <woods%planix.ca@localhost>
Cc: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
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 <riastradh%NetBSD.org@localhost>
# 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--
Home |
Main Index |
Thread Index |
Old Index