svn commit: r185170 - head/sys/ufs/ufs

[Kostik Belousov]

On Mon, Dec 01, 2008 at 01:36:36PM -0500, John Baldwin wrote:
> On Saturday 22 November 2008 08:11:11 am Konstantin Belousov wrote:
> > Author: kib
> > Date: Sat Nov 22 13:11:11 2008
> > New Revision: 185170
> > URL: http://svn.freebsd.org/changeset/base/185170
> > 
> > Log:
> >   Busy ufs filesystem around block of code that does ".." lookup. Since
> >   mnt_lock is before lock of any vnode on the mp, it uses LK_NOWAIT. Since
> >   MNTK_UNMOUNT may be transient, pdp lock is dropped when vfs_busy()
> >   failed, and operation is retried after some time. This way, ffs_vget()
> >   is not called on the mp that may be in the process of being destroyed by
> >   unmount.
> >   
> >   Check for the VI_DOOMED flag on pdp after its lock is reacquired, to
> >   better detect some situations where directory containing ".."
> >   entry is removed during the lookup.
> 
> I'm not really sure it matters if the parent directory goes away because it 
> will have deadfs vops so any subsequent operations will already fail, yes?
Operations will fail. There is another race with parent directory
being removed while pdp is unlocked. This can creep without tripping
over deadfs operations for pdp. As Tor noted, the race may be considered
as a security issue, allowing to escape the chroot. Check for reclamation
cannot catch a move of pdp, this is why I specified the check as partial
measure.

> 
> Also, do you really need to grab the VI_LOCK just to check VI_DOOMED?   Other 
> places in the kernel check that flag while holding the vnode lock w/o 
> acquiring the interlock.  Since you are just doing a single atomic read the 
> interlock doesn't actually close any races anyway.  I think it just adds 
> overhead.
Yes, VI_DOOMED is set when both lock and interlock is held.
I will remove interlock around the check.

> 
> >   Reviewed by:	tegge, attilio (previous version)
> >   Tested by:	pho
> >   MFC after:	1 month
> > 
> > Modified:
> >   head/sys/ufs/ufs/ufs_lookup.c
> > 
> > Modified: head/sys/ufs/ufs/ufs_lookup.c
> > 
> ==============================================================================
> > --- head/sys/ufs/ufs/ufs_lookup.c	Sat Nov 22 12:36:15 2008	(r185169)
> > +++ head/sys/ufs/ufs/ufs_lookup.c	Sat Nov 22 13:11:11 2008	(r185170)
> > @@ -157,6 +157,8 @@ ufs_lookup(ap)
> >  	int nameiop = cnp->cn_nameiop;
> >  	ino_t ino;
> >  	int ltype;
> > +	int pdoomed;
> > +	struct mount *mp;
> >  
> >  	bp = NULL;
> >  	slotoffset = -1;
> > @@ -578,9 +580,32 @@ found:
> >  	pdp = vdp;
> >  	if (flags & ISDOTDOT) {
> >  		ltype = VOP_ISLOCKED(pdp);
> > +		mp = pdp->v_mount;
> > +		for (;;) {
> > +			error = vfs_busy(mp, MBF_NOWAIT);
> > +			if (error == 0)
> > +				break;
> > +			VOP_UNLOCK(pdp, 0);
> > +			pause("ufs_dd", 1);
> > +			vn_lock(pdp, ltype | LK_RETRY);
> > +			VI_LOCK(pdp);
> > +			pdoomed = pdp->v_iflag & VI_DOOMED;
> > +			VI_UNLOCK(pdp);
> > +			if (pdoomed)
> > +				return (ENOENT);
> > +		}
> >  		VOP_UNLOCK(pdp, 0);	/* race to get the inode */
> > -		error = VFS_VGET(pdp->v_mount, ino, cnp->cn_lkflags, &tdp);
> > +		error = VFS_VGET(mp, ino, cnp->cn_lkflags, &tdp);
> > +		vfs_unbusy(mp);
> >  		vn_lock(pdp, ltype | LK_RETRY);
> > +		VI_LOCK(pdp);
> > +		pdoomed = pdp->v_iflag & VI_DOOMED;
> > +		VI_UNLOCK(pdp);
> > +		if (pdoomed) {
> > +			if (error == 0)
> > +				vput(tdp);
> > +			error = ENOENT;
> > +		}
> >  		if (error)
> >  			return (error);
> >  		*vpp = tdp;
> > 
> 
> 
> 
> -- 
> John Baldwin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20081201/509bc803/attachment.pgp