Kernel: Make Inode::lookup() return a RefPtr<Inode>

Previously this API would return an InodeIdentifier, which meant that
there was a race in path resolution where an inode could be unlinked
in between finding the InodeIdentifier for a path component, and
actually resolving that to an Inode object.

Attaching a test that would quickly trip an assertion before.

Test: Kernel/path-resolution-race.cpp

Kernel/FileSystem/DevPtsFS.cpp

@@ -169,17 +169,17 @@ size_t DevPtsFSInode::directory_entry_count() const
     return 2 + ptys->size();
 }
 
-InodeIdentifier DevPtsFSInode::lookup(StringView name)
+RefPtr<Inode> DevPtsFSInode::lookup(StringView name)
 {
     ASSERT(identifier().index() == 1);
 
     if (name == "." || name == "..")
-        return identifier();
+        return fs().get_inode(identifier());
 
     bool ok;
     unsigned pty_index = name.to_uint(ok);
     if (ok && ptys->contains(pty_index)) {
-        return { fsid(), pty_index_to_inode_index(pty_index) };
+        return fs().get_inode({ fsid(), pty_index_to_inode_index(pty_index) });
     }
 
     return {};

Kernel/FileSystem/DevPtsFS.h

@@ -67,7 +67,7 @@ private:
     virtual ssize_t read_bytes(off_t, ssize_t, u8* buffer, FileDescription*) const override;
     virtual InodeMetadata metadata() const override;
     virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) const override;
-    virtual InodeIdentifier lookup(StringView name) override;
+    virtual RefPtr<Inode> lookup(StringView name) override;
     virtual void flush_metadata() override;
     virtual ssize_t write_bytes(off_t, ssize_t, const u8* buffer, FileDescription*) override;
     virtual KResult add_child(InodeIdentifier child_id, const StringView& name, mode_t) override;

Kernel/FileSystem/Ext2FileSystem.cpp

@@ -1493,14 +1493,14 @@ void Ext2FSInode::populate_lookup_cache() const
     m_lookup_cache = move(children);
 }
 
-InodeIdentifier Ext2FSInode::lookup(StringView name)
+RefPtr<Inode> Ext2FSInode::lookup(StringView name)
 {
     ASSERT(is_directory());
     populate_lookup_cache();
     LOCKER(m_lock);
     auto it = m_lookup_cache.find(name.hash(), [&](auto& entry) { return entry.key == name; });
     if (it != m_lookup_cache.end())
-        return { fsid(), (*it).value };
+        return fs().get_inode({ fsid(), (*it).value });
     return {};
 }
 

Kernel/FileSystem/Ext2FileSystem.h

@@ -57,7 +57,7 @@ private:
     virtual ssize_t read_bytes(off_t, ssize_t, u8* buffer, FileDescription*) const override;
     virtual InodeMetadata metadata() const override;
     virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) const override;
-    virtual InodeIdentifier lookup(StringView name) override;
+    virtual RefPtr<Inode> lookup(StringView name) override;
     virtual void flush_metadata() override;
     virtual ssize_t write_bytes(off_t, ssize_t, const u8* data, FileDescription*) override;
     virtual KResult add_child(InodeIdentifier child_id, const StringView& name, mode_t) override;

Kernel/FileSystem/Inode.h

@@ -72,7 +72,7 @@ public:
 
     virtual ssize_t read_bytes(off_t, ssize_t, u8* buffer, FileDescription*) const = 0;
     virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) const = 0;
-    virtual InodeIdentifier lookup(StringView name) = 0;
+    virtual RefPtr<Inode> lookup(StringView name) = 0;
     virtual ssize_t write_bytes(off_t, ssize_t, const u8* data, FileDescription*) = 0;
     virtual KResult add_child(InodeIdentifier child_id, const StringView& name, mode_t) = 0;
     virtual KResult remove_child(const StringView& name) = 0;

Kernel/FileSystem/ProcFS.cpp

@@ -1298,13 +1298,13 @@ bool ProcFSInode::traverse_as_directory(Function<bool(const FS::DirectoryEntry&)
     return true;
 }
 
-InodeIdentifier ProcFSInode::lookup(StringView name)
+RefPtr<Inode> ProcFSInode::lookup(StringView name)
 {
     ASSERT(is_directory());
     if (name == ".")
-        return identifier();
+        return fs().get_inode(identifier());
     if (name == "..")
-        return to_parent_id(identifier());
+        return fs().get_inode(to_parent_id(identifier()));
 
     auto proc_file_type = to_proc_file_type(identifier());
 
@@ -1314,7 +1314,7 @@ InodeIdentifier ProcFSInode::lookup(StringView name)
                 continue;
             if (entry.proc_file_type > __FI_Root_Start && entry.proc_file_type < __FI_Root_End) {
                 if (name == entry.name) {
-                    return to_identifier(fsid(), PDI_Root, 0, (ProcFileType)entry.proc_file_type);
+                    return fs().get_inode(to_identifier(fsid(), PDI_Root, 0, (ProcFileType)entry.proc_file_type));
                 }
             }
         }
@@ -1327,7 +1327,7 @@ InodeIdentifier ProcFSInode::lookup(StringView name)
                 process_exists = Process::from_pid(name_as_number);
             }
             if (process_exists)
-                return to_identifier(fsid(), PDI_Root, name_as_number, FI_PID);
+                return fs().get_inode(to_identifier(fsid(), PDI_Root, name_as_number, FI_PID));
         }
         return {};
     }
@@ -1336,22 +1336,22 @@ InodeIdentifier ProcFSInode::lookup(StringView name)
         for (int i = 1; i < sys_variables().size(); ++i) {
             auto& variable = sys_variables()[i];
             if (name == variable.name)
-                return sys_var_to_identifier(fsid(), i);
+                return fs().get_inode(sys_var_to_identifier(fsid(), i));
         }
         return {};
     }
 
     if (proc_file_type == FI_Root_net) {
         if (name == "adapters")
-            return to_identifier(fsid(), PDI_Root, 0, FI_Root_net_adapters);
+            return fs().get_inode(to_identifier(fsid(), PDI_Root, 0, FI_Root_net_adapters));
         if (name == "arp")
-            return to_identifier(fsid(), PDI_Root, 0, FI_Root_net_arp);
+            return fs().get_inode(to_identifier(fsid(), PDI_Root, 0, FI_Root_net_arp));
         if (name == "tcp")
-            return to_identifier(fsid(), PDI_Root, 0, FI_Root_net_tcp);
+            return fs().get_inode(to_identifier(fsid(), PDI_Root, 0, FI_Root_net_tcp));
         if (name == "udp")
-            return to_identifier(fsid(), PDI_Root, 0, FI_Root_net_udp);
+            return fs().get_inode(to_identifier(fsid(), PDI_Root, 0, FI_Root_net_udp));
         if (name == "local")
-            return to_identifier(fsid(), PDI_Root, 0, FI_Root_net_local);
+            return fs().get_inode(to_identifier(fsid(), PDI_Root, 0, FI_Root_net_local));
         return {};
     }
 
@@ -1367,7 +1367,7 @@ InodeIdentifier ProcFSInode::lookup(StringView name)
                 if (entry.name == nullptr)
                     continue;
                 if (name == entry.name) {
-                    return to_identifier(fsid(), PDI_PID, to_pid(identifier()), (ProcFileType)entry.proc_file_type);
+                    return fs().get_inode(to_identifier(fsid(), PDI_PID, to_pid(identifier()), (ProcFileType)entry.proc_file_type));
                 }
             }
         }
@@ -1385,7 +1385,7 @@ InodeIdentifier ProcFSInode::lookup(StringView name)
                     fd_exists = process->file_description(name_as_number);
             }
             if (fd_exists)
-                return to_identifier_with_fd(fsid(), to_pid(identifier()), name_as_number);
+                return fs().get_inode(to_identifier_with_fd(fsid(), to_pid(identifier()), name_as_number));
         }
     }
     return {};
@@ -1529,7 +1529,7 @@ KResult ProcFSProxyInode::remove_child(const StringView& name)
     return m_fd->inode()->remove_child(name);
 }
 
-InodeIdentifier ProcFSProxyInode::lookup(StringView name)
+RefPtr<Inode> ProcFSProxyInode::lookup(StringView name)
 {
     if (!m_fd->inode())
         return {};

Kernel/FileSystem/ProcFS.h

@@ -99,7 +99,7 @@ private:
     virtual ssize_t read_bytes(off_t, ssize_t, u8* buffer, FileDescription*) const override;
     virtual InodeMetadata metadata() const override;
     virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) const override;
-    virtual InodeIdentifier lookup(StringView name) override;
+    virtual RefPtr<Inode> lookup(StringView name) override;
     virtual void flush_metadata() override;
     virtual ssize_t write_bytes(off_t, ssize_t, const u8* buffer, FileDescription*) override;
     virtual KResult add_child(InodeIdentifier child_id, const StringView& name, mode_t) override;
@@ -125,7 +125,7 @@ private:
     virtual ssize_t read_bytes(off_t, ssize_t, u8*, FileDescription*) const override { ASSERT_NOT_REACHED(); }
     virtual InodeMetadata metadata() const override;
     virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) const override { ASSERT_NOT_REACHED(); }
-    virtual InodeIdentifier lookup(StringView name) override;
+    virtual RefPtr<Inode> lookup(StringView name) override;
     virtual void flush_metadata() override {};
     virtual ssize_t write_bytes(off_t, ssize_t, const u8*, FileDescription*) override { ASSERT_NOT_REACHED(); }
     virtual KResult add_child(InodeIdentifier child_id, const StringView& name, mode_t) override;

Kernel/FileSystem/TmpFS.cpp

@@ -233,20 +233,20 @@ ssize_t TmpFSInode::write_bytes(off_t offset, ssize_t size, const u8* buffer, Fi
     return size;
 }
 
-InodeIdentifier TmpFSInode::lookup(StringView name)
+RefPtr<Inode> TmpFSInode::lookup(StringView name)
 {
     LOCKER(m_lock);
     ASSERT(is_directory());
 
     if (name == ".")
-        return identifier();
+        return fs().get_inode(identifier());
     if (name == "..")
-        return m_parent;
+        return fs().get_inode(m_parent);
 
     auto it = m_children.find(name);
     if (it == m_children.end())
         return {};
-    return it->value.entry.inode;
+    return fs().get_inode(it->value.entry.inode);
 }
 
 size_t TmpFSInode::directory_entry_count() const

Kernel/FileSystem/TmpFS.h

@@ -77,7 +77,7 @@ public:
     virtual ssize_t read_bytes(off_t, ssize_t, u8* buffer, FileDescription*) const override;
     virtual InodeMetadata metadata() const override;
     virtual bool traverse_as_directory(Function<bool(const FS::DirectoryEntry&)>) const override;
-    virtual InodeIdentifier lookup(StringView name) override;
+    virtual RefPtr<Inode> lookup(StringView name) override;
     virtual void flush_metadata() override;
     virtual ssize_t write_bytes(off_t, ssize_t, const u8* buffer, FileDescription*) override;
     virtual KResult add_child(InodeIdentifier child_id, const StringView& name, mode_t) override;

Kernel/FileSystem/VirtualFileSystem.cpp

@@ -811,9 +811,8 @@ KResultOr<NonnullRefPtr<Custody>> VFS::resolve_path(StringView path, Custody& ba
         }
 
         // Okay, let's look up this part.
-        auto child_id = parent.inode().lookup(part);
+        auto child_inode = parent.inode().lookup(part);
-
+        if (!child_inode) {
-        if (!child_id.is_valid()) {
             if (out_parent) {
                 // ENOENT with a non-null parent custody signals to caller that
                 // we found the immediate parent of the file, but the file itself
@@ -824,14 +823,13 @@ KResultOr<NonnullRefPtr<Custody>> VFS::resolve_path(StringView path, Custody& ba
         }
 
         int mount_flags_for_child = parent.mount_flags();
+
         // See if there's something mounted on the child; in that case
         // we would need to return the guest inode, not the host inode.
-        if (auto mount = find_mount_for_host(child_id)) {
+        if (auto mount = find_mount_for_host(child_inode->identifier())) {
-            child_id = mount->guest();
+            child_inode = get_inode(mount->guest());
             mount_flags_for_child = mount->flags();
         }
-        auto child_inode = get_inode(child_id);
-        ASSERT(child_inode);
 
         custody = Custody::create(&parent, part, *child_inode, mount_flags_for_child);
 

Tests/Kernel/path-resolution-race.cpp

@@ -0,0 +1,16 @@
+#include <unistd.h>
+#include <sys/stat.h>
+
+int main()
+{
+    if (!fork()) {
+        for (;;) {
+            mkdir("/tmp/x", 0666);
+            rmdir("/tmp/x");
+        }
+    }
+    for (;;) {
+        chdir("/tmp/x");
+    }
+    return 0;
+}