kern/133439: [vfs][panic] Kernel Panic in kern_vfs

[Keith Sklower]

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

From: Keith Sklower <sklower at vangogh.CS.Berkeley.EDU>
To: bug-followup at FreeBSD.org, Jhickey at isi.edu
Cc:  
Subject: Re: kern/133439: [vfs][panic] Kernel Panic in kern_vfs
Date: Tue, 7 Apr 2009 08:03:38 -0700 (PDT)

 I would have otherwise entitled this :
     VFS_CHECKEXP inherently unsafe, even in 7.2 Beta?
 
 I'm writing after having been one of the users inconvenienced by the crashes
 reported by John Hickey in Problem report kerne/133439 who hypothesized
 that the panics were caused by referencing memory in an 8-way CPU
 in nfsrv_fhtovp that had been freed while revising the export list.
 
 I have some historical connection to this code, having written the
 original radix tree implementation, proposing its use to Kirk Mckusick for
 nfs exports, and providing him the 30 lines or so of code that at
 the time were need to switch from hash lists to the tree;
 so I was intensely curious about the nature of the crash.
 
 I spent a few hours enjoying some code spelunking yesterday afternoon,
 and have the following impressions:
 
 The VFS_CHECKEXP handle takes as inputs a mount point, and a network
 address, and returns as output flags which are written into storage
 provided by the caller, and a pointer to an internal u_cred which resides`
 in storage provided by the the file system underneath, and not by the caller.
 
 When the underlying interface handle is vfs_stdcheckexp, although
 FreeBSD 7.2 improves the locking around looking up the the appropriate
 credential, once that critical section is exited, nothing prevents
 another kernel thread from freeing the netred element that encompasses
 that credential.
 
 This leaves us with 3 unfortunate choices:
 
 1.) Acknowledging that the interface is SMP-unsafe and requiring 
     anybody using VFS_CHECKEXP to explicitly call
     lockmgr(&mp->mnt_explock, LK_EXCLUSIVE, NULL, curthread);
     and release it once having copied the u_cred structure
     (and, of course removing the lockmgr calls from vfs_stdcheckexp()
     so that it doesn't deadlock against itself)
 
 or
 
 2.) changing the definition of VFS_CHECKEXP so that the  *caller*
     provides storage for it to copy the u_cred structure into.
 
 or
 
 3.) defining a new handle in the interface, e.g. VFS_CHECKFREE so that
     VFS_CHECKEXP can lock the specific opaque elment returned, and
     VFS_CHECKFREE would unlock and do the actual freeing of it if
     it has evaporated since being looked up, analogous to the way
     rtalloc() and RTFREE() work.   (which actually work by reference 
     counts).
 
 I leave to your judgement which of the three to pick.
 
 Regards,
 
 Keith Sklower
 
 P.S.
 Given the comment at the bottom of vfs_export() in 7.2
 I would recommend moving the 
    lockmgr(&mp->mnt_explock, LK_RELEASE, NULL, curthread);
 from immediately after out:
 to immediately before the return(error);