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 > > _at__at_ -153,6 +153,7 _at__at_ > > 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; > > _at__at_ -557,8 +558,9 _at__at_ > > */ > > 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_at_FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.orgReceived on Fri Oct 14 2005 - 16:45:02 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:38:45 UTC