From 74c4c864bdc625c3584cbe6107a1617b946d7a86 Mon Sep 17 00:00:00 2001 From: Liav A Date: Fri, 20 Aug 2021 08:49:07 +0300 Subject: [PATCH] Kernel+SystemServer: Simplify the DevTmpFS design We are no longer have a separate Inode object class for the pts directory. With a small exception to this, all chmod and chown code is now at one place. It's now possible to create any name of a sub-directory in the filesystem. --- Kernel/FileSystem/DevTmpFS.cpp | 229 +++++++++++------------- Kernel/FileSystem/DevTmpFS.h | 80 +++++---- Userland/Services/SystemServer/main.cpp | 37 ++-- 3 files changed, 171 insertions(+), 175 deletions(-) diff --git a/Kernel/FileSystem/DevTmpFS.cpp b/Kernel/FileSystem/DevTmpFS.cpp index f84b2fc132..5921b8209b 100644 --- a/Kernel/FileSystem/DevTmpFS.cpp +++ b/Kernel/FileSystem/DevTmpFS.cpp @@ -47,6 +47,13 @@ DevTmpFSInode::DevTmpFSInode(DevTmpFS& fs) { } +DevTmpFSInode::DevTmpFSInode(DevTmpFS& fs, unsigned major_number, unsigned minor_number) + : Inode(fs, fs.allocate_inode_index()) + , m_major_number(major_number) + , m_minor_number(minor_number) +{ +} + KResultOr DevTmpFSInode::read_bytes(off_t, size_t, UserOrKernelBuffer&, OpenFileDescription*) const { VERIFY_NOT_REACHED(); @@ -73,27 +80,84 @@ KResultOr DevTmpFSInode::write_bytes(off_t, size_t, const UserOrKernelBu KResultOr> DevTmpFSInode::create_child(StringView, mode_t, dev_t, UserID, GroupID) { - return EROFS; + VERIFY_NOT_REACHED(); } KResult DevTmpFSInode::add_child(Inode&, const StringView&, mode_t) { - return EROFS; + VERIFY_NOT_REACHED(); +} + +InodeMetadata DevTmpFSInode::metadata() const +{ + MutexLocker locker(m_inode_lock); + VERIFY((m_mode & 0777) == m_mode); + InodeMetadata metadata; + metadata.uid = m_uid; + metadata.gid = m_gid; + metadata.size = 0; + metadata.mtime = mepoch; + switch (node_type()) { + case Type::RootDirectory: + metadata.inode = { fsid(), 1 }; + metadata.mode = 0040555; + metadata.uid = 0; + metadata.gid = 0; + metadata.size = 0; + metadata.mtime = mepoch; + break; + case Type::Directory: + metadata.inode = { fsid(), index() }; + metadata.mode = S_IFDIR | m_mode; + break; + case Type::BlockDevice: + metadata.inode = { fsid(), index() }; + metadata.mode = S_IFBLK | m_mode; + metadata.major_device = m_major_number; + metadata.minor_device = m_minor_number; + break; + case Type::CharacterDevice: + metadata.inode = { fsid(), index() }; + metadata.mode = S_IFCHR | m_mode; + metadata.major_device = m_major_number; + metadata.minor_device = m_minor_number; + break; + case Type::Link: + metadata.inode = { fsid(), index() }; + // FIXME: For now, it might not be possible to change the m_mode due to + // it pointing to another device node which its absolute_path() is not + // recognizable by the VirtualFileSystem. + // When we resolve the issues with the clunky absolute_path() interface, + // this should work out of the box... + metadata.mode = S_IFLNK | m_mode; + break; + default: + VERIFY_NOT_REACHED(); + } + return metadata; } KResult DevTmpFSInode::remove_child(const StringView&) { - return EROFS; + VERIFY_NOT_REACHED(); } -KResult DevTmpFSInode::chmod(mode_t) +KResult DevTmpFSInode::chmod(mode_t mode) { - return EPERM; + MutexLocker locker(m_inode_lock); + mode &= 0777; + if (m_mode == mode) + return KSuccess; + m_mode = mode; + return KSuccess; } -KResult DevTmpFSInode::chown(UserID, GroupID) +KResult DevTmpFSInode::chown(UserID uid, GroupID gid) { - return EPERM; + MutexLocker locker(m_inode_lock); + m_uid = uid; + m_gid = gid; + return KSuccess; } KResult DevTmpFSInode::truncate(u64) @@ -125,18 +189,6 @@ KResultOr DevTmpFSLinkInode::read_bytes(off_t offset, size_t, UserOrKern return m_link->length(); } -InodeMetadata DevTmpFSLinkInode::metadata() const -{ - InodeMetadata metadata; - metadata.inode = { fsid(), index() }; - metadata.mode = S_IFLNK | 0555; - metadata.uid = 0; - metadata.gid = 0; - metadata.size = 0; - metadata.mtime = mepoch; - return metadata; -} - KResultOr DevTmpFSLinkInode::write_bytes(off_t offset, size_t count, UserOrKernelBuffer const& buffer, OpenFileDescription*) { auto new_string = TRY(buffer.try_copy_into_kstring(count)); @@ -152,30 +204,18 @@ DevTmpFSDirectoryInode::DevTmpFSDirectoryInode(DevTmpFS& fs) : DevTmpFSInode(fs) { } +DevTmpFSDirectoryInode::DevTmpFSDirectoryInode(DevTmpFS& fs, NonnullOwnPtr name) + : DevTmpFSInode(fs) + , m_name(move(name)) +{ +} DevTmpFSDirectoryInode::~DevTmpFSDirectoryInode() { } -InodeMetadata DevTmpFSDirectoryInode::metadata() const +KResult DevTmpFSDirectoryInode::traverse_as_directory(Function callback) const { - InodeMetadata metadata; - metadata.inode = { fsid(), 1 }; - metadata.mode = 0040555; - metadata.uid = 0; - metadata.gid = 0; - metadata.size = 0; - metadata.mtime = mepoch; - return metadata; -} - -DevTmpFSRootDirectoryInode::DevTmpFSRootDirectoryInode(DevTmpFS& fs) - : DevTmpFSDirectoryInode(fs) -{ -} - -KResult DevTmpFSRootDirectoryInode::traverse_as_directory(Function callback) const -{ - MutexLocker locker(fs().m_lock); + MutexLocker locker(m_inode_lock); callback({ ".", identifier(), 0 }); callback({ "..", identifier(), 0 }); for (auto& node : m_nodes) { @@ -185,19 +225,20 @@ KResult DevTmpFSRootDirectoryInode::traverse_as_directory(Function> DevTmpFSRootDirectoryInode::lookup(StringView name) +KResultOr> DevTmpFSDirectoryInode::lookup(StringView name) { - MutexLocker locker(fs().m_lock); + MutexLocker locker(m_inode_lock); for (auto& node : m_nodes) { - if (node.name() == name) + if (node.name() == name) { return node; + } } - return ENOENT; + return KResult(ENOENT); } -KResult DevTmpFSRootDirectoryInode::remove_child(const StringView& name) +KResult DevTmpFSDirectoryInode::remove_child(const StringView& name) { - MutexLocker locker(fs().m_lock); + MutexLocker locker(m_inode_lock); for (auto& node : m_nodes) { if (node.name() == name) { m_nodes.remove(node); @@ -207,10 +248,9 @@ KResult DevTmpFSRootDirectoryInode::remove_child(const StringView& name) return KResult(ENOENT); } -KResultOr> DevTmpFSRootDirectoryInode::create_child(StringView name, mode_t mode, dev_t device_mode, UserID, GroupID) +KResultOr> DevTmpFSDirectoryInode::create_child(StringView name, mode_t mode, dev_t device_mode, UserID, GroupID) { - MutexLocker locker(fs().m_lock); - + MutexLocker locker(m_inode_lock); for (auto& node : m_nodes) { if (node.name() == name) return KResult(EEXIST); @@ -219,9 +259,8 @@ KResultOr> DevTmpFSRootDirectoryInode::create_child(StringV InodeMetadata metadata; metadata.mode = mode; if (metadata.is_directory()) { - if (name != "pts") - return EROFS; - auto new_directory_inode = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) DevTmpFSPtsDirectoryInode(fs()))); + auto name_kstring = TRY(KString::try_create(name)); + auto new_directory_inode = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) DevTmpFSDirectoryInode(fs(), move(name_kstring)))); m_nodes.append(*new_directory_inode); return new_directory_inode; } @@ -237,32 +276,34 @@ KResultOr> DevTmpFSRootDirectoryInode::create_child(StringV if (metadata.is_symlink()) { auto name_kstring = TRY(KString::try_create(name)); auto new_link_inode = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) DevTmpFSLinkInode(fs(), move(name_kstring)))); + TRY(new_link_inode->chmod(mode)); m_nodes.append(*new_link_inode); return new_link_inode; } return EROFS; } +DevTmpFSRootDirectoryInode::DevTmpFSRootDirectoryInode(DevTmpFS& fs) + : DevTmpFSDirectoryInode(fs) +{ + m_mode = 0555; +} DevTmpFSRootDirectoryInode::~DevTmpFSRootDirectoryInode() { } -InodeMetadata DevTmpFSRootDirectoryInode::metadata() const +KResult DevTmpFSRootDirectoryInode::chmod(mode_t) { - InodeMetadata metadata; - metadata.inode = { fsid(), 1 }; - metadata.mode = 0040555; - metadata.uid = 0; - metadata.gid = 0; - metadata.size = 0; - metadata.mtime = mepoch; - return metadata; + return EPERM; +} + +KResult DevTmpFSRootDirectoryInode::chown(UserID, GroupID) +{ + return EPERM; } DevTmpFSDeviceInode::DevTmpFSDeviceInode(DevTmpFS& fs, unsigned major_number, unsigned minor_number, bool block_device, NonnullOwnPtr name) - : DevTmpFSInode(fs) + : DevTmpFSInode(fs, major_number, minor_number) , m_name(move(name)) - , m_major_number(major_number) - , m_minor_number(minor_number) , m_block_device(block_device) { } @@ -271,24 +312,6 @@ DevTmpFSDeviceInode::~DevTmpFSDeviceInode() { } -KResult DevTmpFSDeviceInode::chown(UserID uid, GroupID gid) -{ - MutexLocker locker(m_inode_lock); - m_uid = uid; - m_gid = gid; - return KSuccess; -} - -KResult DevTmpFSDeviceInode::chmod(mode_t mode) -{ - MutexLocker locker(m_inode_lock); - mode &= 0777; - if (m_required_mode == mode) - return KSuccess; - m_required_mode = mode; - return KSuccess; -} - StringView DevTmpFSDeviceInode::name() const { return m_name->view(); @@ -309,20 +332,6 @@ KResultOr DevTmpFSDeviceInode::read_bytes(off_t offset, size_t count, Us return result.value(); } -InodeMetadata DevTmpFSDeviceInode::metadata() const -{ - MutexLocker locker(m_inode_lock); - InodeMetadata metadata; - metadata.inode = { fsid(), index() }; - metadata.mode = (m_block_device ? S_IFBLK : S_IFCHR) | m_required_mode; - metadata.uid = m_uid; - metadata.gid = m_gid; - metadata.size = 0; - metadata.mtime = mepoch; - metadata.major_device = m_major_number; - metadata.minor_device = m_minor_number; - return metadata; -} KResultOr DevTmpFSDeviceInode::write_bytes(off_t offset, size_t count, const UserOrKernelBuffer& buffer, OpenFileDescription* description) { MutexLocker locker(m_inode_lock); @@ -338,36 +347,4 @@ KResultOr DevTmpFSDeviceInode::write_bytes(off_t offset, size_t count, c return result.value(); } -DevTmpFSPtsDirectoryInode::DevTmpFSPtsDirectoryInode(DevTmpFS& fs) - : DevTmpFSDirectoryInode(fs) -{ -} -KResult DevTmpFSPtsDirectoryInode::traverse_as_directory(Function callback) const -{ - MutexLocker locker(m_inode_lock); - callback({ ".", identifier(), 0 }); - callback({ "..", identifier(), 0 }); - return KSuccess; -} - -KResultOr> DevTmpFSPtsDirectoryInode::lookup(StringView) -{ - return ENOENT; -} - -DevTmpFSPtsDirectoryInode::~DevTmpFSPtsDirectoryInode() -{ -} -InodeMetadata DevTmpFSPtsDirectoryInode::metadata() const -{ - InodeMetadata metadata; - metadata.inode = { fsid(), index() }; - metadata.mode = 0040555; - metadata.uid = 0; - metadata.gid = 0; - metadata.size = 0; - metadata.mtime = mepoch; - return metadata; -} - } diff --git a/Kernel/FileSystem/DevTmpFS.h b/Kernel/FileSystem/DevTmpFS.h index 45a5f4b0d4..df7cfd2260 100644 --- a/Kernel/FileSystem/DevTmpFS.h +++ b/Kernel/FileSystem/DevTmpFS.h @@ -46,11 +46,13 @@ public: DevTmpFS const& fs() const { return static_cast(Inode::fs()); } protected: - DevTmpFSInode(DevTmpFS&); + explicit DevTmpFSInode(DevTmpFS&); + DevTmpFSInode(DevTmpFS&, unsigned, unsigned); virtual KResultOr read_bytes(off_t, size_t, UserOrKernelBuffer& buffer, OpenFileDescription*) const override; virtual KResult traverse_as_directory(Function) const override; virtual KResultOr> lookup(StringView name) override; virtual void flush_metadata() override; + virtual InodeMetadata metadata() const override final; virtual KResultOr write_bytes(off_t, size_t, const UserOrKernelBuffer& buffer, OpenFileDescription*) override; virtual KResultOr> create_child(StringView name, mode_t, dev_t, UserID, GroupID) override; virtual KResult add_child(Inode&, const StringView& name, mode_t) override; @@ -59,11 +61,27 @@ protected: virtual KResult chown(UserID, GroupID) override; virtual KResult truncate(u64) override; + mode_t m_mode { 0600 }; + UserID m_uid { 0 }; + GroupID m_gid { 0 }; + const unsigned m_major_number { 0 }; + const unsigned m_minor_number { 0 }; + +protected: + enum class Type { + BlockDevice, + CharacterDevice, + Directory, + RootDirectory, + Link + }; + virtual Type node_type() const = 0; + private: IntrusiveListNode> m_list_node; }; -class DevTmpFSDeviceInode : public DevTmpFSInode { +class DevTmpFSDeviceInode final : public DevTmpFSInode { friend class DevTmpFS; friend class DevTmpFSRootDirectoryInode; friend class DevTmpFSDirectoryInode; @@ -74,25 +92,20 @@ public: private: DevTmpFSDeviceInode(DevTmpFS&, unsigned, unsigned, bool, NonnullOwnPtr name); + // ^DevTmpFSInode + virtual Type node_type() const override { return m_block_device ? Type::BlockDevice : Type::CharacterDevice; } + // ^Inode virtual KResultOr read_bytes(off_t, size_t, UserOrKernelBuffer& buffer, OpenFileDescription*) const override; - virtual InodeMetadata metadata() const override; virtual KResultOr write_bytes(off_t, size_t, const UserOrKernelBuffer& buffer, OpenFileDescription*) override; - virtual KResult chown(UserID, GroupID) override; - virtual KResult chmod(mode_t) override; NonnullOwnPtr m_name; - const unsigned m_major_number; - const unsigned m_minor_number; const bool m_block_device; - mode_t m_required_mode; - UserID m_uid { 0 }; - GroupID m_gid { 0 }; }; -class DevTmpFSLinkInode : public DevTmpFSInode { +class DevTmpFSLinkInode final : public DevTmpFSInode { friend class DevTmpFS; - friend class DevTmpFSRootDirectoryInode; + friend class DevTmpFSDirectoryInode; public: virtual StringView name() const override; @@ -100,9 +113,11 @@ public: protected: DevTmpFSLinkInode(DevTmpFS&, NonnullOwnPtr); + // ^DevTmpFSInode + virtual Type node_type() const override { return Type::Link; } + // ^Inode virtual KResultOr read_bytes(off_t, size_t, UserOrKernelBuffer& buffer, OpenFileDescription*) const override; - virtual InodeMetadata metadata() const override; virtual KResultOr write_bytes(off_t, size_t, const UserOrKernelBuffer& buffer, OpenFileDescription*) override; NonnullOwnPtr m_name; @@ -115,28 +130,23 @@ class DevTmpFSDirectoryInode : public DevTmpFSInode { public: virtual ~DevTmpFSDirectoryInode() override; + virtual StringView name() const override { return m_name->view(); } protected: - DevTmpFSDirectoryInode(DevTmpFS&); - // ^Inode - virtual InodeMetadata metadata() const override; + // ^DevTmpFSInode + virtual Type node_type() const override { return Type::Directory; } - IntrusiveList, &DevTmpFSInode::m_list_node> m_nodes; -}; - -class DevTmpFSPtsDirectoryInode final : public DevTmpFSDirectoryInode { - friend class DevTmpFS; - friend class DevTmpFSRootDirectoryInode; - -public: - virtual ~DevTmpFSPtsDirectoryInode() override; - virtual StringView name() const override { return "pts"; }; - -private: - explicit DevTmpFSPtsDirectoryInode(DevTmpFS&); + virtual KResultOr> create_child(StringView name, mode_t, dev_t, UserID, GroupID) override; + virtual KResult remove_child(const StringView& name) override; virtual KResult traverse_as_directory(Function) const override; virtual KResultOr> lookup(StringView name) override; - virtual InodeMetadata metadata() const override; + DevTmpFSDirectoryInode(DevTmpFS&, NonnullOwnPtr name); + // ^Inode + OwnPtr m_name; + IntrusiveList, &DevTmpFSInode::m_list_node> m_nodes; + +private: + explicit DevTmpFSDirectoryInode(DevTmpFS&); }; class DevTmpFSRootDirectoryInode final : public DevTmpFSDirectoryInode { @@ -147,12 +157,12 @@ public: virtual StringView name() const override { return "."; } private: + // ^DevTmpFSInode + virtual Type node_type() const override { return Type::Directory; } + explicit DevTmpFSRootDirectoryInode(DevTmpFS&); - virtual KResultOr> create_child(StringView name, mode_t, dev_t, UserID, GroupID) override; - virtual KResult traverse_as_directory(Function) const override; - virtual KResultOr> lookup(StringView name) override; - virtual InodeMetadata metadata() const override; - virtual KResult remove_child(const StringView& name) override; + virtual KResult chmod(mode_t) override; + virtual KResult chown(UserID, GroupID) override; }; } diff --git a/Userland/Services/SystemServer/main.cpp b/Userland/Services/SystemServer/main.cpp index 4931a7677b..c37c001854 100644 --- a/Userland/Services/SystemServer/main.cpp +++ b/Userland/Services/SystemServer/main.cpp @@ -83,6 +83,13 @@ static void chown_wrapper(const char* path, uid_t uid, gid_t gid) VERIFY_NOT_REACHED(); } } +static void chmod_wrapper(const char* path, mode_t mode) +{ + int rc = chmod(path, mode); + if (rc < 0 && errno != ENOENT) { + VERIFY_NOT_REACHED(); + } +} static void chown_all_matching_device_nodes(group* group, unsigned major_number) { @@ -341,6 +348,21 @@ static void prepare_synthetic_filesystems() VERIFY_NOT_REACHED(); } + rc = symlink("/proc/self/fd/0", "/dev/stdin"); + if (rc < 0) { + VERIFY_NOT_REACHED(); + } + + rc = symlink("/proc/self/fd/1", "/dev/stdout"); + if (rc < 0) { + VERIFY_NOT_REACHED(); + } + + rc = symlink("/proc/self/fd/2", "/dev/stderr"); + if (rc < 0) { + VERIFY_NOT_REACHED(); + } + populate_devfs(); rc = mkdir("/dev/pts", 0755); @@ -357,6 +379,7 @@ static void prepare_synthetic_filesystems() if (rc < 0) { VERIFY_NOT_REACHED(); } + chmod_wrapper("/dev/urandom", 0666); auto phys_group = getgrnam("phys"); VERIFY(phys_group); @@ -376,19 +399,6 @@ static void prepare_synthetic_filesystems() VERIFY(audio_group); chown_wrapper("/dev/audio", 0, audio_group->gr_gid); - rc = symlink("/proc/self/fd/0", "/dev/stdin"); - if (rc < 0) { - VERIFY_NOT_REACHED(); - } - rc = symlink("/proc/self/fd/1", "/dev/stdout"); - if (rc < 0) { - VERIFY_NOT_REACHED(); - } - rc = symlink("/proc/self/fd/2", "/dev/stderr"); - if (rc < 0) { - VERIFY_NOT_REACHED(); - } - // Note: We open the /dev/null device and set file descriptors 0, 1, 2 to it // because otherwise these file descriptors won't have a custody, making // the ProcFS file descriptor links (at /proc/PID/fd/{0,1,2}) to have an @@ -396,7 +406,6 @@ static void prepare_synthetic_filesystems() // This affects also every other process that inherits the file descriptors // from SystemServer, so it is important for other things (also for ProcFS // tests that are running in CI mode). - int stdin_new_fd = open("/dev/null", O_NONBLOCK); if (stdin_new_fd < 0) { VERIFY_NOT_REACHED();