From owner-svn-src-head@FreeBSD.ORG Tue Jul 1 18:09:08 2014 Return-Path: Delivered-To: svn-src-head@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 53D45C01; Tue, 1 Jul 2014 18:09:08 +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 CF0F82DA9; Tue, 1 Jul 2014 18:09:07 +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 s61I93ug059379 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 1 Jul 2014 21:09:03 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.8.3 kib.kiev.ua s61I93ug059379 Received: (from kostik@localhost) by tom.home (8.14.9/8.14.9/Submit) id s61I93is059378; Tue, 1 Jul 2014 21:09:03 +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:09:03 +0300 From: Konstantin Belousov To: Mateusz Guzik Subject: Re: svn commit: r268087 - head/sys/kern Message-ID: <20140701180903.GT93733@kib.kiev.ua> References: <201407010921.s619LXHL063077@svn.freebsd.org> <20140701114245.GO93733@kib.kiev.ua> <20140701115612.GA26696@dft-labs.eu> <20140701123058.GP93733@kib.kiev.ua> <20140701125410.GB26696@dft-labs.eu> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="8i1mnYwBm74ExquZ" Content-Disposition: inline In-Reply-To: <20140701125410.GB26696@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, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Jul 2014 18:09:08 -0000 --8i1mnYwBm74ExquZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 01, 2014 at 02:54:10PM +0200, Mateusz Guzik wrote: > 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 =3D fdcheckstd(td); > > > - vn_lock(imgp->vp, LK_SHARED | LK_RETRY); > > > if (error !=3D 0) > > > goto done1; > > > newcred =3D crdup(oldcred); > > > euip =3D uifind(attr.va_uid); > > > + vn_lock(imgp->vp, LK_SHARED | LK_RETRY); > > > PROC_LOCK(p); > > > /* > > > * Set the new credentials. > > This is definitely fine. > >=20 > > > @@ -764,7 +764,9 @@ interpret: > > > if (oldcred->cr_svuid !=3D oldcred->cr_uid || > > > oldcred->cr_svgid !=3D oldcred->cr_gid) { > > > PROC_UNLOCK(p); > > > + VOP_UNLOCK(imgp->vp, 0); > > > newcred =3D 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. > >=20 > > 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. > >=20 >=20 > It is passed down to MAC (mac_vnode_execve_will_transition and > mac_vnode_execve_transition) and then vfs_mark_atime. And we can lock it only there. >=20 > I don't see how to easily reorganize the code to simplify stuff and > reduce relocking. >=20 > > > @@ -841,6 +843,7 @@ interpret: > > > =20 > > > SDT_PROBE(proc, kernel, , exec__success, args->fname, 0, 0, 0, 0); > > > =20 > > > + 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. > >=20 >=20 > 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. >=20 > --=20 > Mateusz Guzik --8i1mnYwBm74ExquZ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJTsvk+AAoJEJDCuSvBvK1BoAAP/is7K60DhgvmqVn7QP+rkxLP X30Oe0Wd6yWjweVKL9XQrkHhrdUL4cryV8ZHTTpN5gnSPqOLaNkvNxCbc9bUd6gC Ja4JD77ujnTAYkFhg+3h/mf74ZaP2R2GhLEqiij95pw9lYWo3HqjREw6tiwu9gpx +9LbYASQnWvTtY/ihxTuw8b1pIM1IykcxzFZwYO17Ny2hD5cV7W+ylKGPVH3EkXD Rh9+qnFuRWa0UtCrtgrVVI6c9fYySmNdZyevpUYSepSLqC6DZP+Ly3ppq93uJq2y +6+aO2WGiecfkvqdWtEt7oDXQGflGx4XoZjfsgXYHkM6axv61/stFQBhFXJZnZ1W t7ltyNVp1T/IAU15aGucZTYzOyPh0UukxaGPG9DdlXJ44OnEpQjMGSexhIqnayoa XP5jctJebmkF0MSbfeJzdErTdMvXpkkB6uIol+WSXRLbaExd0niaFUzos+yYAOq9 MH41wHu5tPTqPXLLVUiwQM+t5b+vDd2TCDhg7NI1Q8M3mie+kTT9G5q8pOlrqRju W+TLsOW+F27LOtQdhjJq+5dB/Ag5opvumA2IMmNeGc/4et1VvLNEjgwVwOXR3sH7 ZL2I2JYJG4H5Xc/cGhEVc3piQFcyPqKA2MvPGPm0jLggSx7gi0K7u3CLLJ692Pl5 U99ExJTppKEBJZIkgjhy =rv5x -----END PGP SIGNATURE----- --8i1mnYwBm74ExquZ--