From owner-freebsd-current@FreeBSD.ORG Fri Oct 14 17:56:59 2005 Return-Path: X-Original-To: freebsd-current@FreeBSD.org Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5E21C16A420; Fri, 14 Oct 2005 17:56:59 +0000 (GMT) (envelope-from truckman@FreeBSD.org) Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id BC32843D5E; Fri, 14 Oct 2005 17:56:58 +0000 (GMT) (envelope-from truckman@FreeBSD.org) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.13.3/8.13.3) with ESMTP id j9EHuahT054866; Fri, 14 Oct 2005 10:56:40 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <200510141756.j9EHuahT054866@gw.catspoiler.org> Date: Fri, 14 Oct 2005 10:56:36 -0700 (PDT) From: Don Lewis To: jhb@FreeBSD.org In-Reply-To: <200510132325.j9DNPBif052696@gw.catspoiler.org> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii Cc: mi+mx@aldan.algebra.com, freebsd-current@FreeBSD.org, re@FreeBSD.org, kris@obsecurity.org Subject: Re: 6.0 hangs (while building OOo) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Oct 2005 17:56:59 -0000 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)