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.

Reviewed by:	kib
MFC after:	2 weeks
Differential Revision: https://reviews.freebsd.org/D25027

1 files changed, 10 insertions, 3 deletions

diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 7a99de63c655..776e8a795b7c 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -3454,8 +3454,9 @@ corefile_open_last(struct thread *td, char *name, int indexpos,
 		    (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);
@@ -3466,12 +3467,18 @@ corefile_open_last(struct thread *td, char *name, int indexpos,
 		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) {