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: Tue, 11 Aug 2026 02:07:47 +0000
This is a multi-part message in MIME format.
--=_rFLsIlDMAmZS54PhaM3eE2ii5a8qA3Zz
Content-Transfer-Encoding: quoted-printable
> Date: Mon, 10 Aug 2026 14:59:19 -0700
> From: "Greg A. Woods" <woods%planix.ca@localhost>
>=20
> At Mon, 10 Aug 2026 02:40:04 +0000, Taylor R Campbell <riastradh@NetBSD.o=
rg> wrote:
> > Can you please try the attached patch?
>=20
> New and different panic:
>=20
> [ 256.3450500] panic: kernel diagnostic assertion "error !=3D 0 || pid !=
=3D -1 || mutex_owned((*p)->p_lock)" failed: file "/Volumes/work/woods/g-Ne=
tBSD-src/sys/kern/kern_proc.c", line 2992
Sorry about that -- typo on my part (`pid !=3D -1' should've been `pid
=3D=3D -1' in that assertion), along with some other issues I saw on
review. Try the attached one instead?
It's a bit tricky to provoke the path you hit originally: I think the
vnode for a parent directory of cwd has to be recycled, causing the
nfs client to wait for network traffic to look it up again.
I'm curious to know what you were doing that led to it? Debuggers
like gdb are likely to use the affected sysctl but I'm not sure what
else.
--=_rFLsIlDMAmZS54PhaM3eE2ii5a8qA3Zz
Content-Type: text/plain; charset="ISO-8859-1"; name="pr60568-sysctlvmproclock-v2"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr60568-sysctlvmproclock-v2.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 2ec7a3d2a384a345cd63e0fe8fc73fdcdd274188
# Parent b125bad3265e13dfd5a772b3676744fca85ce488
# EXP-Topic riastradh-pr60568-sysctlvmproclock
WIP: sysctl vm.proc.*: Fix locking protocol.
1. proc_vmspace_getref: Take p->p_lock for access to p_sflag.
2. New subroutine proc_find_reflocked is like proc_find_locked but
returns with p_reflock held rather than p_lock.
3. Change fill_vmentries to use proc_find_reflocked instead of
proc_find_locked so we don't hold p->p_lock across kmem_alloc
(potentially deadlocking the page daemon) or vnode_to_path
(potentially tripping locking-against-self when nfs with option
`intr' checks for signal delivery).
PR kern/60568: panic locking against myself (p->p_lock) in NFS from
sysctl_vmproc
diff -r b125bad3265e -r 2ec7a3d2a384 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
@@ -1785,23 +1785,44 @@ proclist_foreach_call(struct proclist *l
return ret;
}
=20
+/*
+ * proc_vmspace_getref(p, &vm)
+ *
+ * If p is not currently exiting, acquire a reference to its
+ * vmspace. Return 0, set vm, and increment its reference count
+ * on success. Return nonzero error code without on failure.
+ *
+ * Caller MUST NOT hold p->p_lock. Caller MAY hold p->p_reflock
+ * and/or proc_lock.
+ *
+ * On success, caller MUST call vmspace_free(vm) when done.
+ */
int
proc_vmspace_getref(struct proc *p, struct vmspace **vm)
{
-
- /* XXXCDC: how should locking work here? */
+ int error;
=20
/* curproc exception is for coredump. */
-
- if ((p !=3D curproc && (p->p_sflag & PS_WEXIT) !=3D 0) ||
- (p->p_vmspace->vm_refcnt < 1)) {
- return SET_ERROR(EFAULT);
+ if (p !=3D curproc) {
+ mutex_enter(p->p_lock);
+ if (p->p_sflag & PS_WEXIT) {
+ error =3D SET_ERROR(EFAULT);
+ goto out;
+ }
+ }
+
+ if (p->p_vmspace->vm_refcnt < 1) {
+ error =3D SET_ERROR(EFAULT);
+ goto out;
}
=20
uvmspace_addref(p->p_vmspace);
*vm =3D p->p_vmspace;
-
- return 0;
+ error =3D 0;
+
+out: if (p !=3D curproc)
+ mutex_exit(p->p_lock);
+ return error;
}
=20
/*
@@ -2946,7 +2967,20 @@ fill_kproc2(struct proc *p, struct kinfo
}
}
=20
-
+/*
+ * proc_find_locked(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)
{
@@ -2959,9 +2993,9 @@ proc_find_locked(struct lwp *l, struct p
*p =3D proc_find(pid);
=20
if (*p =3D=3D NULL) {
- if (pid !=3D -1)
- mutex_exit(&proc_lock);
- return SET_ERROR(ESRCH);
+ mutex_exit(&proc_lock);
+ error =3D SET_ERROR(ESRCH);
+ goto out;
}
if (pid !=3D -1)
mutex_enter((*p)->p_lock);
@@ -2973,7 +3007,64 @@ 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=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 iff pid =3D=3D -1, and
+ * take p->p_reflock as a reader or writer according to op iff 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 || 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 rw_exit(&p->p_reflock) iff 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) {
+ 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);
+
+out: if (pid !=3D -1 && *p !=3D NULL) {
+ mutex_exit((*p)->p_lock);
+ if (error)
+ rw_exit(&(*p)->p_reflock);
+ }
+ 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 2ec7a3d2a384 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 2ec7a3d2a384 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
@@ -5391,15 +5391,29 @@ 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 take a read lock on p->p_reflock so the
+ * process cannot exit while we're still working with it.
+ */
+ if ((error =3D proc_find_reflocked(l, &p, pid, RW_READER)) !=3D 0)
return error;
+ KASSERT(pid =3D=3D -1 || rw_read_held(&p->p_reflock));
=20
vme =3D NULL;
count =3D 0;
=20
+ /*
+ * XXX Is this still necessary, now that we hold p_reflock to
+ * prevent concurrent exit?
+ */
if ((error =3D proc_vmspace_getref(p, &vm)) !=3D 0)
goto out;
=20
+ /*
+ * 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 +5430,13 @@ 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);
+ rw_exit(&p->p_reflock);
if (error =3D=3D 0) {
const u_int esize =3D uimin(sizeof(*vme), elem_size);
dp =3D oldp;
--=_rFLsIlDMAmZS54PhaM3eE2ii5a8qA3Zz--
Home |
Main Index |
Thread Index |
Old Index