Date: Tue, 1 Jul 2014 21:09:03 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik <mjg@FreeBSD.org> Subject: Re: svn commit: r268087 - head/sys/kern Message-ID: <20140701180903.GT93733@kib.kiev.ua> In-Reply-To: <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> <20140701125410.GB26696@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <mjguzik gmail.com>
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140701180903.GT93733>