Date: Fri, 12 Jun 2020 07:23:28 +0000 (UTC) From: Andriy Gapon <avg@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r362086 - stable/12/sys/kern Message-ID: <202006120723.05C7NSAC049894@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: avg Date: Fri Jun 12 07:23:27 2020 New Revision: 362086 URL: https://svnweb.freebsd.org/changeset/base/362086 Log: MFC r361620: corefile_open_last: don't keep a locked vnode while locking other ones Consider this scenario: - kern.corefile=/var/coredumps/%N.%U.%I.core - multiple processes with the same name crash at the same time It's possible that one process selects existing file N as oldvp while it keeps looking for an unused file number. Another process scans through files and stumbles upon N. That process would be blocked on the vnode lock while holding the directory vnode exclusively locked. The first process would, thus, get blocked on the directory's vnode lock. More generally, holding a file's vnode lock (oldvp) while trying to lock its directory (for the next lookup) is a violation of the vnode locking order. I have observed this deadlock in the wild. So, the change to keep oldvp "opened" but unlocked and to lock it again only if it's to be returned as the result. As kib noted, an alternative would be to keep the directory locked and to use VOP_LOOKUP directly for scanning through existing core files. Modified: stable/12/sys/kern/kern_sig.c Directory Properties: stable/12/ (props changed) Modified: stable/12/sys/kern/kern_sig.c ============================================================================== --- stable/12/sys/kern/kern_sig.c Fri Jun 12 06:10:27 2020 (r362085) +++ stable/12/sys/kern/kern_sig.c Fri Jun 12 07:23:27 2020 (r362086) @@ -3423,8 +3423,9 @@ corefile_open_last(struct thread *td, char *name, int (lasttime.tv_sec == vattr.va_mtime.tv_sec && lasttime.tv_nsec >= vattr.va_mtime.tv_nsec)) { if (oldvp != NULL) - vnode_close_locked(td, oldvp); + vn_close(oldvp, FWRITE, td->td_ucred, td); oldvp = vp; + VOP_UNLOCK(oldvp); lasttime = vattr.va_mtime; } else { vnode_close_locked(td, vp); @@ -3435,12 +3436,18 @@ corefile_open_last(struct thread *td, char *name, int if (nextvp == NULL) { if ((td->td_proc->p_flag & P_SUGID) != 0) { error = EFAULT; - vnode_close_locked(td, oldvp); + vn_close(oldvp, FWRITE, td->td_ucred, td); } else { nextvp = oldvp; + error = vn_lock(nextvp, LK_EXCLUSIVE); + if (error != 0) { + vn_close(nextvp, FWRITE, td->td_ucred, + td); + nextvp = NULL; + } } } else { - vnode_close_locked(td, oldvp); + vn_close(oldvp, FWRITE, td->td_ucred, td); } } if (error != 0) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202006120723.05C7NSAC049894>