Date: Mon, 1 Dec 2008 23:32:17 +0200 From: Kostik Belousov <kostikbel@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r185170 - head/sys/ufs/ufs Message-ID: <20081201213217.GR3045@deviant.kiev.zoral.com.ua> In-Reply-To: <200812011336.37636.jhb@freebsd.org> References: <200811221311.mAMDBBU8018510@svn.freebsd.org> <200812011336.37636.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--v4cNTr+tRGSs1txX Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > >=20 > > 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. Si= nce > > 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. > > =20 > > 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. >=20 > I'm not really sure it matters if the parent directory goes away because = it=20 > 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. >=20 > Also, do you really need to grab the VI_LOCK just to check VI_DOOMED? O= ther=20 > places in the kernel check that flag while holding the vnode lock w/o=20 > acquiring the interlock. Since you are just doing a single atomic read t= he=20 > interlock doesn't actually close any races anyway. I think it just adds= =20 > overhead. Yes, VI_DOOMED is set when both lock and interlock is held. I will remove interlock around the check. >=20 > > Reviewed by: tegge, attilio (previous version) > > Tested by: pho > > MFC after: 1 month > >=20 > > Modified: > > head/sys/ufs/ufs/ufs_lookup.c > >=20 > > Modified: head/sys/ufs/ufs/ufs_lookup.c > >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > > --- 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 =3D cnp->cn_nameiop; > > ino_t ino; > > int ltype; > > + int pdoomed; > > + struct mount *mp; > > =20 > > bp =3D NULL; > > slotoffset =3D -1; > > @@ -578,9 +580,32 @@ found: > > pdp =3D vdp; > > if (flags & ISDOTDOT) { > > ltype =3D VOP_ISLOCKED(pdp); > > + mp =3D pdp->v_mount; > > + for (;;) { > > + error =3D vfs_busy(mp, MBF_NOWAIT); > > + if (error =3D=3D 0) > > + break; > > + VOP_UNLOCK(pdp, 0); > > + pause("ufs_dd", 1); > > + vn_lock(pdp, ltype | LK_RETRY); > > + VI_LOCK(pdp); > > + pdoomed =3D pdp->v_iflag & VI_DOOMED; > > + VI_UNLOCK(pdp); > > + if (pdoomed) > > + return (ENOENT); > > + } > > VOP_UNLOCK(pdp, 0); /* race to get the inode */ > > - error =3D VFS_VGET(pdp->v_mount, ino, cnp->cn_lkflags, &tdp); > > + error =3D VFS_VGET(mp, ino, cnp->cn_lkflags, &tdp); > > + vfs_unbusy(mp); > > vn_lock(pdp, ltype | LK_RETRY); > > + VI_LOCK(pdp); > > + pdoomed =3D pdp->v_iflag & VI_DOOMED; > > + VI_UNLOCK(pdp); > > + if (pdoomed) { > > + if (error =3D=3D 0) > > + vput(tdp); > > + error =3D ENOENT; > > + } > > if (error) > > return (error); > > *vpp =3D tdp; > >=20 >=20 >=20 >=20 > --=20 > John Baldwin --v4cNTr+tRGSs1txX Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (FreeBSD) iEYEARECAAYFAkk0V+EACgkQC3+MBN1Mb4gTLgCgp1rQjMHM3kgoMCtr9z4rRZj3 WXcAmwTnDuAxe5QYEjvnKQArxRDdxOf0 =EIHL -----END PGP SIGNATURE----- --v4cNTr+tRGSs1txX--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081201213217.GR3045>