svn commit: r268087 - head/sys/kern

[Konstantin Belousov]

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 = 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.
And we can lock it only there.

> 
> 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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20140701/aa59e2b2/attachment-0001.sig>