From 81f2b97177995973754f099b35a0cdb66762508d Mon Sep 17 00:00:00 2001 From: Pawel Dziepak Date: Thu, 1 Nov 2012 18:31:04 +0100 Subject: [PATCH] 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. --- .../kernel/file_systems/nfs4/FileSystem.cpp | 2 + .../kernel/file_systems/nfs4/FileSystem.h | 2 + .../kernel/file_systems/nfs4/Inode.cpp | 2 + .../kernel/file_systems/nfs4/InodeIdMap.h | 53 ++++++++++++++++--- .../file_systems/nfs4/kernel_interface.cpp | 31 +++++++++-- 5 files changed, 80 insertions(+), 10 deletions(-) diff --git a/src/add-ons/kernel/file_systems/nfs4/FileSystem.cpp b/src/add-ons/kernel/file_systems/nfs4/FileSystem.cpp index 66bb163f33..c68bfeda33 100644 --- a/src/add-ons/kernel/file_systems/nfs4/FileSystem.cpp +++ b/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); diff --git a/src/add-ons/kernel/file_systems/nfs4/FileSystem.h b/src/add-ons/kernel/file_systems/nfs4/FileSystem.h index 2937066e41..f99232d315 100644 --- a/src/add-ons/kernel/file_systems/nfs4/FileSystem.h +++ b/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); diff --git a/src/add-ons/kernel/file_systems/nfs4/Inode.cpp b/src/add-ons/kernel/file_systems/nfs4/Inode.cpp index ab9cbf4cb3..f303dfda43 100644 --- a/src/add-ons/kernel/file_systems/nfs4/Inode.cpp +++ b/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(); diff --git a/src/add-ons/kernel/file_systems/nfs4/InodeIdMap.h b/src/add-ons/kernel/file_systems/nfs4/InodeIdMap.h index a6bbd48704..2bad259d95 100644 --- a/src/add-ons/kernel/file_systems/nfs4/InodeIdMap.h +++ b/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 fMap; + AVLTreeMap 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::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::Iterator it = fMap.Find(id); + AVLTreeMap::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::Iterator it = fMap.Find(id); + if (!it.HasCurrent()) + return true; + + return it.Current().fRemoved; +} + + #endif // INODEIDMAP_H diff --git a/src/add-ons/kernel/file_systems/nfs4/kernel_interface.cpp b/src/add-ons/kernel/file_systems/nfs4/kernel_interface.cpp index 498d1e5f2c..66302a8801 100644 --- a/src/add-ons/kernel/file_systems/nfs4/kernel_interface.cpp +++ b/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(volume->private_volume); Inode* inode = reinterpret_cast(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(dir->private_node); + FileSystem* fs = reinterpret_cast(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(fromDir->private_node); Inode* toInode = reinterpret_cast(toDir->private_node); + FileSystem* fs = reinterpret_cast(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(&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(dir->private_node); + FileSystem* fs = reinterpret_cast(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(parent->private_node); + FileSystem* fs = reinterpret_cast(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)