Date: Fri, 14 Oct 2005 14:33:11 -0400 From: John Baldwin <jhb@freebsd.org> To: Don Lewis <truckman@freebsd.org> Cc: mi+mx@aldan.algebra.com, freebsd-current@freebsd.org, re@freebsd.org, kris@obsecurity.org Subject: Re: 6.0 hangs (while building OOo) Message-ID: <200510141433.12987.jhb@freebsd.org> In-Reply-To: <200510141756.j9EHuahT054866@gw.catspoiler.org> References: <200510141756.j9EHuahT054866@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday 14 October 2005 01:56 pm, Don Lewis wrote:
> On 13 Oct, Don Lewis wrote:
> > The bug is that once we have unlocked pdp, another thread can do a
> > lookup and overwrite dp->i_ino, so instead of getting the vnode for the
> > ".." directory entry, VFS_VGET() will return the vnode for a
> > subdirectory of the current directory, and when we relock the current
> > directory we'll have a lock order reversal.
> >
> > Even if this doesn't result in a deadlock, it looks like it has the
> > potential for mucking up lookups that involve "..". I also don't
> > currently see a way for this to become a vnode lock leak.
>
> I think the leak happens when dp->i_ino gets overwritten by the inode
> number for ".". This causes ufs_lookup() to recurse on the lock for the
> current directory vnode (the lock is first acquired by VFS_VGET() and
> then recursed by vn_lock()). This isn't expected by lookup(), which
> compares whether the vnode returned by VOP_LOOKUP() is the same as the
> directory vnode and uses this information to decide whether to call
> vput() or vrele().
>
> > The fix is to preserve a copy of dp->d_ino before unlocking pdp,
> > and pass the saved value to VFS_VGET().
> >
> > Index: sys/ufs/ufs/ufs_lookup.c
> > ===================================================================
> > RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_lookup.c,v
> > retrieving revision 1.77
> > diff -u -r1.77 ufs_lookup.c
> > --- sys/ufs/ufs/ufs_lookup.c 13 Apr 2005 10:59:09 -0000 1.77
> > +++ sys/ufs/ufs/ufs_lookup.c 13 Oct 2005 23:20:59 -0000
> > @@ -153,6 +153,7 @@
> > int flags = cnp->cn_flags;
> > int nameiop = cnp->cn_nameiop;
> > struct thread *td = cnp->cn_thread;
> > + u_int32_t saved_ino;
> >
> > bp = NULL;
> > slotoffset = -1;
> > @@ -557,8 +558,9 @@
> > */
> > pdp = vdp;
> > if (flags & ISDOTDOT) {
> > + saved_ino = dp->i_ino;
> > VOP_UNLOCK(pdp, 0, td); /* race to get the inode */
> > - error = VFS_VGET(pdp->v_mount, dp->i_ino,
> > + error = VFS_VGET(pdp->v_mount, saved_ino,
> > cnp->cn_lkflags, &tdp);
> > vn_lock(pdp, LK_EXCLUSIVE | LK_RETRY, td);
> > if (error)
Sounds good to me. Good sleuthing!
--
John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve" = http://www.FreeBSD.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200510141433.12987.jhb>