From owner-svn-src-all@freebsd.org Fri May 29 07:44:03 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 45624331F7E; Fri, 29 May 2020 07:44:03 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49YGm31251z4FMj; Fri, 29 May 2020 07:44:03 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 1E87A14745; Fri, 29 May 2020 07:44:03 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 04T7i30s064587; Fri, 29 May 2020 07:44:03 GMT (envelope-from avg@FreeBSD.org) Received: (from avg@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 04T7i2ZD064586; Fri, 29 May 2020 07:44:02 GMT (envelope-from avg@FreeBSD.org) Message-Id: <202005290744.04T7i2ZD064586@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: avg set sender to avg@FreeBSD.org using -f From: Andriy Gapon Date: Fri, 29 May 2020 07:44:02 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r361620 - head/sys/kern X-SVN-Group: head X-SVN-Commit-Author: avg X-SVN-Commit-Paths: head/sys/kern X-SVN-Commit-Revision: 361620 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 May 2020 07:44:03 -0000 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) {