Kernel: Synchronize removals from TmpFS inode map

Previously we were uncaching inodes from TmpFSInode::one_ref_left().
This was not safe, since one_ref_left() was effectively being called
on a raw pointer after decrementing the local ref count and observing
it become 1. There was a race here where someone else could trigger
the destructor by unreffing to 0 before one_ref_left() got called,
causing us to call one_ref_left() on a deleted inode.

We fix this by using the new remove_from_secondary_lists() mechanism
in ListedRefCounted and synchronizing all access to the TmpFS inode
map with the main Inode::all_instances() lock.

There's probably a nicer way to solve this.

3 files changed, 18 insertions, 17 deletions

diff --git a/Kernel/FileSystem/Inode.h b/Kernel/FileSystem/Inode.h
index 6ad3d0dd81..a502b6dc8b 100644
--- a/Kernel/FileSystem/Inode.h
+++ b/Kernel/FileSystem/Inode.h
@@ -31,7 +31,7 @@ class Inode : public ListedRefCounted<Inode, LockType::Spinlock>
 public:
     virtual ~Inode();
 
-    virtual void one_ref_left() { }
+    virtual void remove_from_secondary_lists() { }
 
     FileSystem& fs() { return m_file_system; }
     FileSystem const& fs() const { return m_file_system; }
diff --git a/Kernel/FileSystem/TmpFS.cpp b/Kernel/FileSystem/TmpFS.cpp
index 440e63cd4c..1d9c2efc41 100644
--- a/Kernel/FileSystem/TmpFS.cpp
+++ b/Kernel/FileSystem/TmpFS.cpp
@@ -37,19 +37,21 @@ Inode& TmpFS::root_inode()
 
 void TmpFS::register_inode(TmpFSInode& inode)
 {
-    MutexLocker locker(m_lock);
     VERIFY(inode.identifier().fsid() == fsid());
 
-    auto index = inode.identifier().index();
-    m_inodes.set(index, inode);
+    Inode::all_instances().with([&](auto&) {
+        auto index = inode.identifier().index();
+        m_inodes.set(index, &inode);
+    });
 }
 
 void TmpFS::unregister_inode(InodeIdentifier identifier)
 {
-    MutexLocker locker(m_lock);
     VERIFY(identifier.fsid() == fsid());
 
-    m_inodes.remove(identifier.index());
+    Inode::all_instances().with([&](auto&) {
+        m_inodes.remove(identifier.index());
+    });
 }
 
 unsigned TmpFS::next_inode_index()
@@ -61,13 +63,13 @@ unsigned TmpFS::next_inode_index()
 
 ErrorOr<NonnullRefPtr<Inode>> TmpFS::get_inode(InodeIdentifier identifier) const
 {
-    MutexLocker locker(m_lock, Mutex::Mode::Shared);
-    VERIFY(identifier.fsid() == fsid());
-
-    auto it = m_inodes.find(identifier.index());
-    if (it == m_inodes.end())
-        return ENOENT;
-    return it->value;
+    return Inode::all_instances().with([&](auto&) -> ErrorOr<NonnullRefPtr<Inode>> {
+        VERIFY(identifier.fsid() == fsid());
+        auto it = m_inodes.find(identifier.index());
+        if (it == m_inodes.end())
+            return ENOENT;
+        return *it->value;
+    });
 }
 
 TmpFSInode::TmpFSInode(TmpFS& fs, const InodeMetadata& metadata, InodeIdentifier parent)
@@ -363,9 +365,8 @@ ErrorOr<void> TmpFSInode::set_mtime(time_t t)
     return {};
 }
 
-void TmpFSInode::one_ref_left()
+void TmpFSInode::remove_from_secondary_lists()
 {
-    // Destroy ourselves.
     fs().unregister_inode(identifier());
 }
 
diff --git a/Kernel/FileSystem/TmpFS.h b/Kernel/FileSystem/TmpFS.h
index 741575f2ca..d4c0f10d24 100644
--- a/Kernel/FileSystem/TmpFS.h
+++ b/Kernel/FileSystem/TmpFS.h
@@ -33,7 +33,7 @@ private:
 
     RefPtr<TmpFSInode> m_root_inode;
 
-    HashMap<InodeIndex, NonnullRefPtr<TmpFSInode>> m_inodes;
+    HashMap<InodeIndex, TmpFSInode*> m_inodes;
     ErrorOr<NonnullRefPtr<Inode>> get_inode(InodeIdentifier identifier) const;
     void register_inode(TmpFSInode&);
     void unregister_inode(InodeIdentifier);
@@ -67,7 +67,7 @@ public:
     virtual ErrorOr<void> set_atime(time_t) override;
     virtual ErrorOr<void> set_ctime(time_t) override;
     virtual ErrorOr<void> set_mtime(time_t) override;
-    virtual void one_ref_left() override;
+    virtual void remove_from_secondary_lists() override;
 
 private:
     TmpFSInode(TmpFS& fs, const InodeMetadata& metadata, InodeIdentifier parent);