From owner-svn-src-all@FreeBSD.ORG Tue Jul 1 11:56:18 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 035E11E9; Tue, 1 Jul 2014 11:56:18 +0000 (UTC) Received: from mail-wi0-x233.google.com (mail-wi0-x233.google.com [IPv6:2a00:1450:400c:c05::233]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 23A0126D8; Tue, 1 Jul 2014 11:56:16 +0000 (UTC) Received: by mail-wi0-f179.google.com with SMTP id cc10so7580129wib.12 for ; Tue, 01 Jul 2014 04:56:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=r+hzMBOX8HWz2d4Jx+FNj68qLtRG9Mmncty6AtKMw9Q=; b=VqJj/WaUzGsixb6FDhzNWblF0S3zUftiNg6cYudx7jR6TJx3ywgacXA3NrqVPoLVkS 321YxgRQepkaorGEGYHhLWvbieh7QgTZ+hxxJrW8YLWZjdfcmS6v5WAITLcBJoD2PKAe uLaMCeOE19ZigrshER8jybXOpfvJHbPs+H784AwJ+hG9CB3PMV6vgt6KCtQoVKRcz/pv qV0FuZLU/2L/xGrSDU7/z5+n0OP4OuXCkpGVFsmrXYnG6syWIBQYEPHIFZuhB6kdw97O tZZMq6sJAcUCht8BL/xiU2psxbU6Pr0ZYUqeN4fiP1wj6ubJWw+dWGAQM2PzMUlhxiir Y9AQ== X-Received: by 10.180.84.226 with SMTP id c2mr16124840wiz.50.1404215775273; Tue, 01 Jul 2014 04:56:15 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id nf11sm8729244wic.9.2014.07.01.04.56.14 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Tue, 01 Jul 2014 04:56:14 -0700 (PDT) Date: Tue, 1 Jul 2014 13:56:12 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: svn commit: r268087 - head/sys/kern Message-ID: <20140701115612.GA26696@dft-labs.eu> References: <201407010921.s619LXHL063077@svn.freebsd.org> <20140701114245.GO93733@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140701114245.GO93733@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18 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: Tue, 01 Jul 2014 11:56:18 -0000 On Tue, Jul 01, 2014 at 02:42:45PM +0300, Konstantin Belousov wrote: > Old code did the malloc(M_WAITOK) call in crget() before the text vnode > was locked. After your change, the crdup() is called with the vnode locked. > Witness would not tell you that anything is wrong there, but the new > code is worse than the previous structure, even if malloc() was sometimes > done when not needed. > > To satisfy the memory request from malloc(), pagedaemon or laundry may > need to lock the vnode, which creates a circular dependency. Pagedaemon > locks vnodes with timeout, which just means that it would not be able > to clean pages while execve() is stuck in malloc(M_WAITOK), while > laundry takes the vnode lock without timeout, hanging until the malloc > request is satisfied. > > The rule is, do not allocate memory while vnodes are locked. It is not > always followed, but it makes no sense to change existing correct code > to broke the pattern. Right, my bad. This was intended to be a minor cleanup, I'm happy to revert if you want. Note that current code relocks the vnode already, so there should be no harm doing the same in 'else' case. (Although LK_RETRY looks somewhat fishy in here.) That said I propose the following: diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c index cce687b..9b3a99d 100644 --- a/sys/kern/kern_exec.c +++ b/sys/kern/kern_exec.c @@ -716,11 +716,11 @@ interpret: VOP_UNLOCK(imgp->vp, 0); setugidsafety(td); error = fdcheckstd(td); - vn_lock(imgp->vp, LK_SHARED | LK_RETRY); if (error != 0) goto done1; newcred = crdup(oldcred); euip = uifind(attr.va_uid); + vn_lock(imgp->vp, LK_SHARED | LK_RETRY); PROC_LOCK(p); /* * Set the new credentials. @@ -764,7 +764,9 @@ interpret: if (oldcred->cr_svuid != oldcred->cr_uid || oldcred->cr_svgid != oldcred->cr_gid) { PROC_UNLOCK(p); + VOP_UNLOCK(imgp->vp, 0); newcred = crdup(oldcred); + vn_lock(imgp->vp, LK_SHARED | LK_RETRY); PROC_LOCK(p); change_svuid(newcred, newcred->cr_uid); change_svgid(newcred, newcred->cr_gid); @@ -841,6 +843,7 @@ interpret: SDT_PROBE(proc, kernel, , exec__success, args->fname, 0, 0, 0, 0); + VOP_UNLOCK(imgp->vp, 0); done1: /* * Free any resources malloc'd earlier that we didn't use. @@ -849,7 +852,6 @@ done1: uifree(euip); if (newcred != NULL) crfree(oldcred); - VOP_UNLOCK(imgp->vp, 0); /* * Handle deferred decrement of ref counts. -- Mateusz Guzik