nfs4: Fix race condition between inode creation and removal

If the underlying file system reuses inode numbers it is possible that
an entry of inode with reused id in InodeIdMap will be removed.
This patch should also improve behavior when one of many hard links
is removed.

src/add-ons/kernel/file_systems/nfs4/FileSystem.cpp

@@ -41,6 +41,7 @@ FileSystem::FileSystem(const MountConfiguration& configuration)
 	mutex_init(&fOpenOwnerLock, NULL);
 	mutex_init(&fOpenLock, NULL);
 	mutex_init(&fDelegationLock, NULL);
+	rw_lock_init(&fRemoveNodeLock, NULL);
 }
 
 
@@ -48,6 +49,7 @@ FileSystem::~FileSystem()
 {
 	NFSServer()->RemoveFileSystem(this);
 
+	rw_lock_destroy(&fRemoveNodeLock);
 	mutex_destroy(&fDelegationLock);
 	mutex_destroy(&fOpenLock);
 	mutex_destroy(&fOpenOwnerLock);

src/add-ons/kernel/file_systems/nfs4/FileSystem.h

@@ -78,6 +78,8 @@ public:
 
 			FileSystem*			fNext;
 			FileSystem*			fPrev;
+
+			rw_lock				fRemoveNodeLock;
 private:
 								FileSystem(const MountConfiguration& config);
 

src/add-ons/kernel/file_systems/nfs4/Inode.cpp

@@ -281,6 +281,8 @@ Inode::Remove(const char* name, FileType type, ino_t* id)
 	status_t result = NFS4Inode::RemoveObject(name, type, &changeInfo, &fileID);
 	if (result != B_OK)
 		return result;
+	if (type != NF4NAMEDATTR)
+		fFileSystem->InoIdMap()->MarkRemoved(fileID);
 
 	DirectoryCache* cache = type != NF4NAMEDATTR ? fCache : fAttrCache;
 	cache->Lock();

src/add-ons/kernel/file_systems/nfs4/InodeIdMap.h

@@ -16,6 +16,10 @@
 
 #include "FileInfo.h"
 
+struct InodeIdMapEntry {
+	FileInfo	fFileInfo;
+	bool		fRemoved;
+};
 
 class InodeIdMap {
 public:
@@ -24,11 +28,15 @@ public:
 
 	inline	status_t						AddEntry(const FileInfo& fi,
 												ino_t id, bool weak = false);
+	inline	status_t						MarkRemoved(ino_t id);
 	inline	status_t						RemoveEntry(ino_t id);
 	inline	status_t						GetFileInfo(FileInfo* fi, ino_t id);
 
+protected:
+	inline	bool							_IsEntryRemoved(ino_t id);
+
 private:
-			AVLTreeMap<ino_t, FileInfo>		fMap;
+			AVLTreeMap<ino_t, InodeIdMapEntry>	fMap;
 			mutex							fLock;
 
 };
@@ -51,10 +59,29 @@ InodeIdMap::~InodeIdMap()
 inline status_t
 InodeIdMap::AddEntry(const FileInfo& fi, ino_t id, bool weak)
 {
+	InodeIdMapEntry entry;
+
 	MutexLocker _(fLock);
-	//if (weak)
+	if (weak || _IsEntryRemoved(id))
 		fMap.Remove(id);
-	return fMap.Insert(id, fi);
+
+	entry.fFileInfo = fi;
+	entry.fRemoved = false;
+
+	return fMap.Insert(id, entry);
+}
+
+
+inline status_t
+InodeIdMap::MarkRemoved(ino_t id)
+{
+	MutexLocker _(fLock);
+	AVLTreeMap<ino_t, InodeIdMapEntry>::Iterator it = fMap.Find(id);
+	if (!it.HasCurrent())
+		return B_ENTRY_NOT_FOUND;
+
+	it.CurrentValuePointer()->fRemoved = true;
+	return B_OK;
 }
 
 
@@ -62,7 +89,9 @@ inline status_t
 InodeIdMap::RemoveEntry(ino_t id)
 {
 	MutexLocker _(fLock);
-	return fMap.Remove(id);
+	if (_IsEntryRemoved(id))
+		return fMap.Remove(id);
+	return B_OK;
 }
 
 
@@ -72,14 +101,26 @@ InodeIdMap::GetFileInfo(FileInfo* fi, ino_t id)
 	ASSERT(fi != NULL);
 
 	MutexLocker _(fLock);
-	AVLTreeMap<ino_t, FileInfo>::Iterator it = fMap.Find(id);
+	AVLTreeMap<ino_t, InodeIdMapEntry>::Iterator it = fMap.Find(id);
 	if (!it.HasCurrent())
 		return B_ENTRY_NOT_FOUND;
 
-	*fi = it.Current();
+	*fi = it.Current().fFileInfo;
 	return B_OK;
 }
 
 
+// Caller must hold fLock
+inline bool
+InodeIdMap::_IsEntryRemoved(ino_t id)
+{
+	AVLTreeMap<ino_t, InodeIdMapEntry>::Iterator it = fMap.Find(id);
+	if (!it.HasCurrent())
+		return true;
+
+	return it.Current().fRemoved;
+}
+
+
 #endif	// INODEIDMAP_H
 

src/add-ons/kernel/file_systems/nfs4/kernel_interface.cpp

@@ -25,7 +25,9 @@
 #include "RPCServer.h"
 #include "WorkQueue.h"
 
+#ifdef DEBUG
 #define TRACE_NFS4
+#endif
 
 #ifdef TRACE_NFS4
 static mutex	gTraceLock	= MUTEX_INITIALIZER(NULL);
@@ -33,7 +35,7 @@ static mutex	gTraceLock	= MUTEX_INITIALIZER(NULL);
 #define TRACE(x...)	\
 	{	\
 		mutex_lock(&gTraceLock);	\
-		dprintf("%s(): ", __FUNCTION__);	\
+		dprintf("nfs4: %s(): ", __FUNCTION__);	\
 		dprintf(x);	\
 		dprintf("\n");	\
 		mutex_unlock(&gTraceLock);	\
@@ -251,10 +253,13 @@ nfs4_read_fs_info(fs_volume* volume, struct fs_info* info)
 static status_t
 nfs4_lookup(fs_volume* volume, fs_vnode* dir, const char* name, ino_t* _id)
 {
+	FileSystem* fs = reinterpret_cast<FileSystem*>(volume->private_volume);
 	Inode* inode = reinterpret_cast<Inode*>(dir->private_node);
-
+	
 	TRACE("volume = %p, dir = %llu, name = %s", volume, inode->ID(), name);
 
+	ReadLocker(fs->fRemoveNodeLock);
+
 	status_t result = inode->LookUp(name, _id);
 	if (result != B_OK)
 		return result;
@@ -262,7 +267,9 @@ nfs4_lookup(fs_volume* volume, fs_vnode* dir, const char* name, ino_t* _id)
 	void* ptr;
 
 	TRACE("*_id = %llu", *_id);
-	return get_vnode(volume, *_id, &ptr);
+	result = get_vnode(volume, *_id, &ptr);
+	unremove_vnode(volume, *_id);
+	return result;
 }
 
 
@@ -485,9 +492,12 @@ static status_t
 nfs4_unlink(fs_volume* volume, fs_vnode* dir, const char* name)
 {
 	Inode* inode = reinterpret_cast<Inode*>(dir->private_node);
+	FileSystem* fs = reinterpret_cast<FileSystem*>(volume->private_volume);
 
 	TRACE("volume = %p, dir = %llu, name = %s", volume, inode->ID(), name);
 
+	WriteLocker(fs->fRemoveNodeLock);
+
 	ino_t id;
 	status_t result = inode->Remove(name, NF4REG, &id);
 	if (result != B_OK)
@@ -503,11 +513,14 @@ nfs4_rename(fs_volume* volume, fs_vnode* fromDir, const char* fromName,
 {
 	Inode* fromInode = reinterpret_cast<Inode*>(fromDir->private_node);
 	Inode* toInode = reinterpret_cast<Inode*>(toDir->private_node);
+	FileSystem* fs = reinterpret_cast<FileSystem*>(volume->private_volume);
 
 	TRACE("volume = %p, fromDir = %llu, toDir = %llu, fromName = %s, "	\
 		"toName = %s", volume, fromInode->ID(), toInode->ID(), fromName,
 		toName);
 
+	ReadLocker(fs->fRemoveNodeLock);
+
 	ino_t id;
 	status_t result = Inode::Rename(fromInode, toInode, fromName, toName, false,
 		&id);
@@ -517,6 +530,8 @@ nfs4_rename(fs_volume* volume, fs_vnode* fromDir, const char* fromName,
 	Inode* child;
 	result = get_vnode(volume, id, reinterpret_cast<void**>(&child));
 	if (result == B_OK) {
+		unremove_vnode(volume, id);
+
 		child->fInfo.fParent = toInode->fInfo.fHandle;
 		child->fInfo.CreateName(toInode->fInfo.fPath, toName);
 	}
@@ -564,10 +579,13 @@ nfs4_create(fs_volume* volume, fs_vnode* dir, const char* name, int openMode,
 	*_cookie = cookie;
 
 	Inode* inode = reinterpret_cast<Inode*>(dir->private_node);
+	FileSystem* fs = reinterpret_cast<FileSystem*>(volume->private_volume);
 
 	TRACE("volume = %p, dir = %llu, name = %s, openMode = %d, perms = %d",
 		volume, inode->ID(), name, openMode, perms);
 
+	ReadLocker(fs->fRemoveNodeLock);
+
 	OpenDelegationData data;
 	status_t result = inode->Create(name, openMode, perms, cookie, &data,
 		_newVnodeID);
@@ -591,7 +609,8 @@ nfs4_create(fs_volume* volume, fs_vnode* dir, const char* name, int openMode,
 			delete cookie;
 			return result;
 		}
-	}
+	} else
+		unremove_vnode(volume, *_newVnodeID);
 
 	child->SetOpenState(cookie->fOpenState);
 
@@ -732,7 +751,11 @@ static status_t
 nfs4_remove_dir(fs_volume* volume, fs_vnode* parent, const char* name)
 {
 	Inode* inode = reinterpret_cast<Inode*>(parent->private_node);
+	FileSystem* fs = reinterpret_cast<FileSystem*>(volume->private_volume);
+
 	TRACE("volume = %p, parent = %llu, name = %s", volume, inode->ID(), name);
+	WriteLocker(fs->fRemoveNodeLock);
+
 	ino_t id;
 	status_t result = inode->Remove(name, NF4DIR, &id);
 	if (result != B_OK)