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 = fdcheckstd(td);
> > - vn_lock(imgp->vp, LK_SHARED | LK_RETRY);
> > if (error != 0)
> > goto done1;
> > newcred = crdup(oldcred);
> > euip = uifind(attr.va_uid);
> > + vn_lock(imgp->vp, LK_SHARED | LK_RETRY);
> > PROC_LOCK(p);
> > /*
> > * Set the new credentials.
> This is definitely fine.
>
> > @@ -764,7 +764,9 @@ interpret:
> > if (oldcred->cr_svuid != oldcred->cr_uid ||
> > oldcred->cr_svgid != oldcred->cr_gid) {
> > PROC_UNLOCK(p);
> > + VOP_UNLOCK(imgp->vp, 0);
> > newcred = 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.
>
> 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.
>
It is passed down to MAC (mac_vnode_execve_will_transition and
mac_vnode_execve_transition) and then vfs_mark_atime.
I don't see how to easily reorganize the code to simplify stuff and
reduce relocking.
> > @@ -841,6 +843,7 @@ interpret:
> >
> > SDT_PROBE(proc, kernel, , exec__success, args->fname, 0, 0, 0, 0);
> >
> > + 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.
>
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.
--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
