From owner-svn-src-all@FreeBSD.ORG Tue Jul 1 12:54:22 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 DA5DBDF8; Tue, 1 Jul 2014 12:54:21 +0000 (UTC) Received: from mail-we0-x235.google.com (mail-we0-x235.google.com [IPv6:2a00:1450:400c:c03::235]) (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 5E1692DC2; Tue, 1 Jul 2014 12:54:20 +0000 (UTC) Received: by mail-we0-f181.google.com with SMTP id q59so9578071wes.26 for ; Tue, 01 Jul 2014 05:54:18 -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=HU4JcN1pIld1QQEliNvgqOi7mseDYkPVZ0+hGBmddI0=; b=ehqDj2PBA+RawyNs220OB+kXxtLaO0fP43iAaypiw/SsKyDvuJc46b90QzGZeeHd7k aXRzh1MJNe8cCExc+WdY0+Efa8qNvQkoSMH7WL+TemzHHDQVWT7VcnZLKL6d2V5Spnhb 8qo36gKyq0exSIC9nwn7iGhL7xOkAv22WBLRRrA79PvPWyP+7mtVZhfLkZxgTlKfMSE1 mdLvWZ9OI3Tem4CntHnLSlSbO2koitmNO5n7AA84Zxgvd9mvS2iz4aeEdiWPcxRk0K+z +9LZzjO9JtuW5zpsM/eH6HHqd2GFVP6o1EGRE6j8WtwJGME5uGAmP0lczsAwP+RaVg+q 2g8g== X-Received: by 10.180.105.68 with SMTP id gk4mr35925804wib.24.1404219258587; Tue, 01 Jul 2014 05:54:18 -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 i8sm43032397wiw.22.2014.07.01.05.54.17 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Tue, 01 Jul 2014 05:54:17 -0700 (PDT) Date: Tue, 1 Jul 2014 14:54:10 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: svn commit: r268087 - head/sys/kern Message-ID: <20140701125410.GB26696@dft-labs.eu> References: <201407010921.s619LXHL063077@svn.freebsd.org> <20140701114245.GO93733@kib.kiev.ua> <20140701115612.GA26696@dft-labs.eu> <20140701123058.GP93733@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140701123058.GP93733@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 12:54:22 -0000 On Tue, Jul 01, 2014 at 03:30:58PM +0300, Konstantin Belousov wrote: > On Tue, Jul 01, 2014 at 01:56:12PM +0200, Mateusz Guzik wrote: > > 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. > This is definitely fine. > > > @@ -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); > Use of LK_RETRY is fine as far errors from VOPs which actually perform > accesses to the vnode are checked. It means that reclaimed vnode would > be detected later. > > In fact, could the vnode unlock moved much earlier, in particular, > to avoid the same unlock/lock in the pmc hook call ? The only use > for the vnode after the VREF() is done, as I see, is to check > for MNT_NOSUID. Can we test this earlier, and cache the result ? > I do not think that the possible race with flag changing under us > matter. > It is passed down to MAC (mac_vnode_execve_will_transition and mac_vnode_execve_transition) and then vfs_mark_atime. I don't see how to easily reorganize the code to simplify stuff and reduce relocking. > > @@ -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. > This change is fine but unrelated. There is no harm of calling free() > while holding vnode lock. > I moved this unlock so that there is no need to lock the vnode after failed fdcheckstd, which would also require additional cleanup for newcred and euip. -- Mateusz Guzik