Date: Tue, 1 Jul 2014 21:07:17 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, Matthew Fleming <mdf@FreeBSD.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, Mateusz Guzik <mjg@freebsd.org> Subject: Re: svn commit: r268087 - head/sys/kern Message-ID: <20140701180717.GS93733@kib.kiev.ua> In-Reply-To: <20140701143238.GD26696@dft-labs.eu> References: <201407010921.s619LXHL063077@svn.freebsd.org> <CAMBSHm-T1mjXXevOe=EMy2WuMsXE6Y=VoFBD-4mN_er0PB2b7w@mail.gmail.com> <20140701143238.GD26696@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <mjg@freebsd.org> 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 <mjguzik gmail.com> --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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140701180717.GS93733>