From owner-svn-src-all@FreeBSD.ORG Tue Jul 1 18:07:23 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 60E0CA96; Tue, 1 Jul 2014 18:07:23 +0000 (UTC) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id DA7E82D91; Tue, 1 Jul 2014 18:07:22 +0000 (UTC) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.14.9/8.14.9) with ESMTP id s61I7HK4059347 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 1 Jul 2014 21:07:17 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.8.3 kib.kiev.ua s61I7HK4059347 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id s61I7Ha1059346; Tue, 1 Jul 2014 21:07:17 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 1 Jul 2014 21:07:17 +0300 From: Konstantin Belousov To: Mateusz Guzik Subject: Re: svn commit: r268087 - head/sys/kern Message-ID: <20140701180717.GS93733@kib.kiev.ua> References: <201407010921.s619LXHL063077@svn.freebsd.org> <20140701143238.GD26696@dft-labs.eu> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="fbLRCyggVpgIMbCc" Content-Disposition: inline In-Reply-To: <20140701143238.GD26696@dft-labs.eu> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.0 X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on tom.home Cc: "svn-src-head@freebsd.org" , Matthew Fleming , "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 18:07:23 -0000 --fbLRCyggVpgIMbCc Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 01, 2014 at 04:32:38PM +0200, Mateusz Guzik wrote: > 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. > >=20 > > I'm not sure the code works. > >=20 > > It gets a copy of the pointer p_ucred under the PROC_LOCK. The > > PROC_LOCK is released before newcred =3D 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. > >=20 > > 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. > >=20 >=20 > 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. In fact, other threads can execute, just not in this process. If rfork(2) was used, then the filedesc table can be shared, but not the address space. I somehow thought that you already ensured that this does not happen. >=20 > 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. >=20 > > Also, what is the motivation to avoid the crcopy? Is there a > > measurable performance impact? > >=20 >=20 > It's just a minor cleanup. >=20 > --=20 > Mateusz Guzik --fbLRCyggVpgIMbCc Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJTsvjUAAoJEJDCuSvBvK1B7ZoQAKCuVci2LNNImsU92e7kUUjY CedqfbEwM90nAsIpDw9AIGj33i2U2pef4Wqd+SkK0VuEuU8YHfiGKhzHSSFQ923A 8cx8ujES70UNphADX+FxWEbZCbk2Sd/2MxDqq/kEUfZvtWewe+EnrELkxqX9oQAC FRdOadUSftxTFGJEq6+kY232R0EgCISXd8wuN5z03WUqTJ4QC7XpBgo+Qc5TBfU+ aCcq1O7y5pEGE0Q0k3xxtqP3hZ6jxiLjTLNRFmoCZt+sVLyHl7QpJutQhOMnY4HP chUajvsRMP59V/FkzcciI1bEjwa2HN8ocGMHpwhqF4YVBs5hgFRvUWnYgqGUS9EG Ubr5Kvi0EXaIMIx4T3HVTO8e1s2Ls0SPSrHWHq7WhMFo8M7H5jWouPHNbdUth7EV 0UdCyxJ3hlIMugbTJtj6to9++5AijCDxAVRV8jWcWyOl7zp7m2jswHuR1DRxT3Gd 4uFT0PBq0L98CSKOi/QrEpWsPU5RRxzrZLx2ef3Pl+jXJxsFWROvRh2noYu6dUeg a7tOZyFnptKSvlNqfvZetUPyRJRfiviW61LN/miGO0LYWsZr7/e4mH4RXEpNSr8E DZs09icA690Sjt2k87SXJhMuxPWZSvhkVJinU0jSBnLB+tXh/bCy053B4hJ4Zzpr 0fkhT0oKpjJMt/qj34rx =GaO8 -----END PGP SIGNATURE----- --fbLRCyggVpgIMbCc--