kern/92785: Using exported filesystem on OS/2 NFS client causes filesystem freeze

[Bruce Evans]

The following reply was made to PR kern/92785; it has been noted by GNATS.

From: Bruce Evans <bde at zeta.org.au>
To: Kostik Belousov <kostikbel at gmail.com>
Cc: Ulrich Spoerlein <uspoerlein at gmail.com>, bug-followup at FreeBSD.org
Subject: Re: kern/92785: Using exported filesystem on OS/2 NFS client causes
 filesystem freeze
Date: Thu, 28 Dec 2006 01:02:20 +1100 (EST)

 On Wed, 27 Dec 2006, Kostik Belousov wrote:
 
 > On Wed, Dec 27, 2006 at 11:43:36AM +0100, Ulrich Spoerlein wrote:
 >> While the patch for UFS seems to quiesce the lock-leak in nfsd I now
 >> exported a FAT32 filesystem. All Linux clients can access the export
 >> just fine, mounting it from OS/2 will result in the following panic,
 >> after issuing a 'dir' in the mounted drive. The dir-command will print
 >> the first line (the "."-DIR) and when trying to look up ".." the nfs
 >> server paniced.
 >>
 >> panic: lockmgr: locking against myself
 >
 > I did not read the code thoroughly, and I prefer not to touch
 > msdosfs. But, anyway, try this:
 
 Me too.
 
 > Index: sys/fs/msdosfs/msdosfs_lookup.c
 > ===================================================================
 > RCS file: /usr/local/arch/ncvs/src/sys/fs/msdosfs/msdosfs_lookup.c,v
 > retrieving revision 1.47
 > diff -u -r1.47 msdosfs_lookup.c
 > --- sys/fs/msdosfs/msdosfs_lookup.c	22 Jan 2006 21:09:38 -0000	1.47
 > +++ sys/fs/msdosfs/msdosfs_lookup.c	27 Dec 2006 10:59:17 -0000
 > @@ -520,16 +520,16 @@
 > 	 * that point backwards in the directory structure.
 > 	 */
 > 	pdp = vdp;
 > -	if (flags & ISDOTDOT) {
 > +	if (dp->de_StartCluster == scn && isadir) {
 > +		VREF(vdp);	/* we want ourself, ie "." */
 > +		*vpp = vdp;
 > +	} else if (flags & ISDOTDOT) {
 > 		VOP_UNLOCK(pdp, 0, td);
 > 		error = deget(pmp, cluster, blkoff,  &tdp);
 > 		vn_lock(pdp, LK_EXCLUSIVE | LK_RETRY, td);
 > 		if (error)
 > 			return (error);
 > 		*vpp = DETOV(tdp);
 > -	} else if (dp->de_StartCluster == scn && isadir) {
 > -		VREF(vdp);	/* we want ourself, ie "." */
 > -		*vpp = vdp;
 > 	} else {
 > 		if ((error = deget(pmp, cluster, blkoff, &tdp)) != 0)
 > 			return (error);
 >
 
 Funny, this looks like a bug that was first reported for msdosfs.  Here
 is some old mail about it.
 
 From bde at zeta.org.au Sun Aug  6 09:59:24 2006 +1000
 Date: Sun, 6 Aug 2006 09:59:22 +1000 (EST)
 From: Bruce Evans <bde at zeta.org.au>
 X-X-Sender: bde at delplex.bde.org
 To: Michiel Pelt <pelt22 at gmail.com>
 cc: freebsd-fs at freebsd.org
 Subject: Re: VOP_LOOKUP of .. in msdosfs
 In-Reply-To: <cee5e2ee0608050817u67f0c08cw73ec68f54400375b at mail.gmail.com>
 Message-ID: <20060806094905.V2709 at delplex.bde.org>
 References: <cee5e2ee0608050817u67f0c08cw73ec68f54400375b at mail.gmail.com>
 MIME-Version: 1.0
 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
 Status: O
 X-Status: 
 X-Keywords: 
 X-UID: 360
 
 On Sat, 5 Aug 2006, Michiel Pelt wrote:
 
 > ...
 > The msdosfs_lookup function in msdosfs_lookup.c has code dealing with the
 > rootdirectory at the label foundroot:. At line 523 (release 6.1 code) of the
 > release code is this piece of code:
 >
 > pdp = vdp;
 > if (flags & ISDOTDOT) {
 >  VOP_UNLOCK(pdp, 0, td);
 >  error = deget(pmp, cluster, blkoff, &tdp);
 >  vn_lock(pdp, LK_EXCLUSIVE | LK_RETRY, td);
 >  if (error)
 >     return (error);
 >  *vpp = DETOV(tdp);
 > }
 >
 > This code has been copied from ffs code, but fails for msdosfs.
 > For starters the introduction of the 'pdp' pointer has no function
 > whatsoever.
 >
 > Consider a lookup for .. in the root. In msdosfs the /. and /.. directories
 > are represented by one and the same vnode (see deget() in msdosfs_denode.c).
 > The deget gets and locks the vnode that was unlocked by VOP_UNLOCK. The
 > vn_lock that follows fails with a panic because the vnode is already locked.
 
 It seems to have been correct in 5.2:
 
 % 	pdp = vdp;
 % 	if (flags & ISDOTDOT) {
 % 		VOP_UNLOCK(pdp, 0, td);
 % 		cnp->cn_flags |= PDIRUNLOCK;
 % 		error = deget(pmp, cluster, blkoff,  &tdp);
 % 		if (error) {
 % 			vn_lock(pdp, LK_EXCLUSIVE | LK_RETRY, td); 
 % 			cnp->cn_flags &= ~PDIRUNLOCK;
 % 			return (error);
 % 		}
 
 Apparently, deget() didn't lock, and we only needed to lock in the error
 case.  Or maybe the problem went unnoticed because the error case is
 even rarer than the ISDOTDOT case.
 
 > So why does this not happen in practice? Because the ISDOTDOT case is
 > handled by vfs_lookup and never reaches VOP_LOOKUP.
 >
 > I did get the error because I forgot to set the VV_ROOT flag in the vnode
 > returned by VFS_ROOT ;-(.
 
 I think the whole ISDOTDOT case is unreachable for the root of an
 msdosfs file system, at least in 5.2, since msdosfs can't be mounted
 on "/" so ".." at the root is never in the current file system and so
 it must be handled by vfs_lookup() and VFS_ROOT must be set so that
 it is handled there.
 
 Bruce
 
 %%%
 
 I don't understand the changes near here since 5.2 (even in ffs).  If
 you do, then please touch a lot of file systems that have the bug :-).
 
 Bruce