From owner-svn-src-all@FreeBSD.ORG Tue Jul 1 14:32:53 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 8E45029E; Tue, 1 Jul 2014 14:32:53 +0000 (UTC) Received: from mail-wi0-x22e.google.com (mail-wi0-x22e.google.com [IPv6:2a00:1450:400c:c05::22e]) (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 88D6D2775; Tue, 1 Jul 2014 14:32:52 +0000 (UTC) Received: by mail-wi0-f174.google.com with SMTP id bs8so7966393wib.1 for ; Tue, 01 Jul 2014 07:32:47 -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=ugZvKhvdiQmQXqbtfmLWeEKe9eGUOMEx4Mw0EOWWNuc=; b=uSk5aKpnYMkRnm6h26O/eHv9FO5NNoJUVqMGWdqnsRaXtvcXpuiwetI7W/UXLph5Cp OumbBFlTo6cA7WTcFPfMYlDyBSeLOjJyri+T/HD79BMRk+2dQATNK9pGslXCsWUKquh/ DtdA//BMSSKZnx6UKuYcbm8pWQbddj8Km50BFu3og8nHjw3fb+VLWKLvreVGduGvEVpT Mt5SETi2/fybTnGPPzsUfjAdlvbr+whn73qM9didpWR89EBU563AmpL6qkGgYQSFIvp6 Gozpg7nvhM8fYf605k6VYgTvLBcwetdXsM1awZHMgVtPyZCYO2tncMofvP+5Loil4Sou OvUQ== X-Received: by 10.194.200.37 with SMTP id jp5mr3539535wjc.120.1404225167099; Tue, 01 Jul 2014 07:32:47 -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 wp6sm48547088wjb.9.2014.07.01.07.32.45 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Tue, 01 Jul 2014 07:32:46 -0700 (PDT) Date: Tue, 1 Jul 2014 16:32:38 +0200 From: Mateusz Guzik To: Matthew Fleming Subject: Re: svn commit: r268087 - head/sys/kern Message-ID: <20140701143238.GD26696@dft-labs.eu> References: <201407010921.s619LXHL063077@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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 14:32:53 -0000 On Tue, Jul 01, 2014 at 07:07:28AM -0700, Matthew Fleming wrote: > On Tue, Jul 1, 2014 at 2:21 AM, Mateusz Guzik wrote: > > Author: mjg > > Date: Tue Jul 1 09:21:32 2014 > > New Revision: 268087 > > URL: http://svnweb.freebsd.org/changeset/base/268087 > > > > Log: > > Don't call crcopysafe or uifind unnecessarily in execve. > > I'm not sure the code works. > > It gets a copy of the pointer p_ucred under the PROC_LOCK. The > PROC_LOCK is released before newcred = crdup(oldcred) is called. Thus > you may be copying an old version of the credentials if any of the > other functions that modify them run in the meantime. > > Maybe this can't happen because the process is single-threaded at the > time and all the other sets of p_ucred come via a syscall. I didn't > look at all the functions in the kernel which set p_ucred. But only > in the case that none of them can run during do_execve this code would > be safe. In which case it at least deserves a comment indicating the > code is violating the normal locking and safety on p_ucred. > All other threads have to be blocked, otherwise there are more dangerous races - for instance we support sharing file descriptor tables, so execve makes sure to unshare the table (fdunshare()), which is especially important for suid binaries. If other threads could execute, they could fork off after fdunshare() and then have a table shared with now privileged process. That said, I can add appropriate comment + assertion at top of do_execve later, just wanted to note the code was assuming that the process is left alone prior to my changes. > Also, what is the motivation to avoid the crcopy? Is there a > measurable performance impact? > It's just a minor cleanup. -- Mateusz Guzik