svn commit: r361620 - head/sys/kern

[Andriy Gapon]

Author: avg
Date: Fri May 29 07:44:02 2020
New Revision: 361620
URL: https://svnweb.freebsd.org/changeset/base/361620

Log:
  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

Modified:
  head/sys/kern/kern_sig.c

Modified: head/sys/kern/kern_sig.c
==============================================================================
--- head/sys/kern/kern_sig.c	Fri May 29 07:38:57 2020	(r361619)
+++ head/sys/kern/kern_sig.c	Fri May 29 07:44:02 2020	(r361620)
@@ -3454,8 +3454,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);
@@ -3466,12 +3467,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) {