From 38c267b4c80e757e093d8dffdd448e8ff4e048e5 Mon Sep 17 00:00:00 2001 From: Bananymous Date: Thu, 26 Oct 2023 02:05:05 +0300 Subject: [PATCH] Kernel: Fix ext2 inode deletion fsck now reports clean filesystem even after deleting files --- kernel/include/kernel/FS/Ext2/FileSystem.h | 2 +- kernel/include/kernel/FS/Ext2/Inode.h | 4 +- kernel/kernel/FS/Ext2/FileSystem.cpp | 32 ++++- kernel/kernel/FS/Ext2/Inode.cpp | 143 +++++++++++++++++---- kernel/kernel/FS/Inode.cpp | 2 + 5 files changed, 152 insertions(+), 31 deletions(-) diff --git a/kernel/include/kernel/FS/Ext2/FileSystem.h b/kernel/include/kernel/FS/Ext2/FileSystem.h index ff0ff6e9a7..b911a0b6a6 100644 --- a/kernel/include/kernel/FS/Ext2/FileSystem.h +++ b/kernel/include/kernel/FS/Ext2/FileSystem.h @@ -58,7 +58,7 @@ namespace Kernel BAN::ErrorOr initialize_root_inode(); BAN::ErrorOr create_inode(const Ext2::Inode&); - void delete_inode(uint32_t); + void delete_inode(uint32_t ino); BAN::ErrorOr resize_inode(uint32_t, size_t); void read_block(uint32_t, BlockBufferWrapper&); diff --git a/kernel/include/kernel/FS/Ext2/Inode.h b/kernel/include/kernel/FS/Ext2/Inode.h index b42fe34de0..d93d68d8cc 100644 --- a/kernel/include/kernel/FS/Ext2/Inode.h +++ b/kernel/include/kernel/FS/Ext2/Inode.h @@ -47,11 +47,13 @@ namespace Kernel uint32_t fs_block_of_data_block_index(uint32_t data_block_index); BAN::ErrorOr link_inode_to_directory(Ext2Inode&, BAN::StringView name); + BAN::ErrorOr is_directory_empty(); + BAN::ErrorOr cleanup_default_links(); void cleanup_from_fs(); BAN::ErrorOr allocate_new_block(); - BAN::ErrorOr sync(); + void sync(); uint32_t block_group() const; diff --git a/kernel/kernel/FS/Ext2/FileSystem.cpp b/kernel/kernel/FS/Ext2/FileSystem.cpp index 8a0dccd809..8a2bb6af77 100644 --- a/kernel/kernel/FS/Ext2/FileSystem.cpp +++ b/kernel/kernel/FS/Ext2/FileSystem.cpp @@ -184,6 +184,8 @@ namespace Kernel write_block(bgd->inode_bitmap, inode_bitmap); bgd->free_inodes_count--; + if (Inode::Mode(ext2_inode.mode).ifdir()) + bgd->used_dirs_count++; write_block(bgd_location.block, bgd_buffer); const uint32_t inode_table_offset = ino_index * superblock().inode_size; @@ -216,34 +218,49 @@ namespace Kernel { LockGuard _(m_lock); + ASSERT(ino >= superblock().first_ino); ASSERT(ino <= superblock().inodes_count); auto bgd_buffer = get_block_buffer(); auto bitmap_buffer = get_block_buffer(); + auto inode_buffer = get_block_buffer(); const uint32_t inode_group = (ino - 1) / superblock().inodes_per_group; const uint32_t inode_index = (ino - 1) % superblock().inodes_per_group; auto bgd_location = locate_block_group_descriptior(inode_group); read_block(bgd_location.block, bgd_buffer); - auto& bgd = bgd_buffer.span().slice(bgd_location.offset).as(); + + // update inode bitmap read_block(bgd.inode_bitmap, bitmap_buffer); - const uint32_t byte = inode_index / 8; - const uint32_t bit = inode_index % 8; + const uint32_t bit = inode_index % 8; ASSERT(bitmap_buffer[byte] & (1 << bit)); - bitmap_buffer[byte] &= ~(1 << bit); write_block(bgd.inode_bitmap, bitmap_buffer); + // memset inode to zero or fsck will complain + auto inode_location = locate_inode(ino); + read_block(inode_location.block, inode_buffer); + auto& inode = inode_buffer.span().slice(inode_location.offset).as(); + bool is_directory = Inode::Mode(inode.mode).ifdir(); + memset(&inode, 0x00, m_superblock.inode_size); + write_block(inode_location.block, inode_buffer); + + // update bgd counts bgd.free_inodes_count++; + if (is_directory) + bgd.used_dirs_count--; write_block(bgd_location.block, bgd_buffer); + // update superblock inode count + m_superblock.free_inodes_count++; + sync_superblock(); + + // remove inode from cache if (m_inode_cache.contains(ino)) m_inode_cache.remove(ino); - - dprintln("succesfully deleted inode {}", ino); } void Ext2FS::read_block(uint32_t block, BlockBufferWrapper& buffer) @@ -395,7 +412,8 @@ namespace Kernel bgd.free_blocks_count++; write_block(bgd_location.block, bgd_buffer); - dprintln("successfully freed block {}", block); + m_superblock.free_blocks_count++; + sync_superblock(); } Ext2FS::BlockLocation Ext2FS::locate_inode(uint32_t ino) diff --git a/kernel/kernel/FS/Ext2/Inode.cpp b/kernel/kernel/FS/Ext2/Inode.cpp index 570e3ec6fd..29b81bc7db 100644 --- a/kernel/kernel/FS/Ext2/Inode.cpp +++ b/kernel/kernel/FS/Ext2/Inode.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -43,7 +44,7 @@ namespace Kernel Ext2Inode::~Ext2Inode() { - if ((mode().ifdir() && m_inode.links_count == 1) || m_inode.links_count == 0) + if (m_inode.links_count == 0) cleanup_from_fs(); } @@ -220,7 +221,7 @@ namespace Kernel if (new_size < m_inode.size) { m_inode.size = new_size; - TRY(sync()); + sync(); return {}; } @@ -243,7 +244,7 @@ namespace Kernel } m_inode.size = new_size; - TRY(sync()); + sync(); return {}; } @@ -254,19 +255,12 @@ namespace Kernel if (m_inode.mode == mode) return {}; m_inode.mode = (m_inode.mode & Inode::Mode::TYPE_MASK) | mode; - TRY(sync()); + sync(); return {}; } void Ext2Inode::cleanup_from_fs() { - if (mode().ifdir()) - { - // FIXME: cleanup entires '.' and '..' and verify - // there are no other entries - ASSERT_NOT_REACHED(); - } - ASSERT(m_inode.links_count == 0); for (uint32_t i = 0; i < blocks(); i++) m_fs.release_block(fs_block_of_data_block_index(i)); @@ -383,8 +377,6 @@ namespace Kernel BAN::ErrorOr Ext2Inode::create_file_impl(BAN::StringView name, mode_t mode, uid_t uid, gid_t gid) { - // FIXME: handle errors - ASSERT(this->mode().ifdir()); if (!(Mode(mode).ifreg())) @@ -408,8 +400,6 @@ namespace Kernel BAN::ErrorOr Ext2Inode::create_directory_impl(BAN::StringView name, mode_t mode, uid_t uid, gid_t gid) { - // FIXME: handle errors - ASSERT(this->mode().ifdir()); ASSERT(Mode(mode).ifdir()); @@ -423,11 +413,15 @@ namespace Kernel } auto inode = inode_or_error.release_value(); + BAN::ScopeGuard cleanup([&] { inode->cleanup_from_fs(); }); + TRY(inode->link_inode_to_directory(*inode, "."sv)); TRY(inode->link_inode_to_directory(*this, ".."sv)); TRY(link_inode_to_directory(*inode, name)); + cleanup.disable(); + return {}; } @@ -476,7 +470,7 @@ namespace Kernel memcpy(new_entry.name, name.data(), name.size()); inode.m_inode.links_count++; - MUST(inode.sync()); + inode.sync(); }; uint32_t block_index = 0; @@ -531,9 +525,100 @@ needs_new_block: return {}; } + BAN::ErrorOr Ext2Inode::is_directory_empty() + { + ASSERT(mode().ifdir()); + if (m_inode.flags & Ext2::Enum::INDEX_FL) + { + dwarnln("deletion of indexed directory is not supported"); + return BAN::Error::from_errno(ENOTSUP); + } + + auto block_buffer = m_fs.get_block_buffer(); + + // Confirm that this doesn't contain anything else than '.' or '..' + for (uint32_t i = 0; i < blocks(); i++) + { + const uint32_t block = fs_block_of_data_block_index(i); + m_fs.read_block(block, block_buffer); + + blksize_t offset = 0; + while (offset < blksize()) + { + auto& entry = block_buffer.span().slice(offset).as(); + + if (entry.inode) + { + BAN::StringView entry_name(entry.name, entry.name_len); + if (entry_name != "."sv && entry_name != ".."sv) + return false; + } + + offset += entry.rec_len; + } + } + + return true; + } + + BAN::ErrorOr Ext2Inode::cleanup_default_links() + { + ASSERT(mode().ifdir()); + + auto block_buffer = m_fs.get_block_buffer(); + + for (uint32_t i = 0; i < blocks(); i++) + { + const uint32_t block = fs_block_of_data_block_index(i); + m_fs.read_block(block, block_buffer); + + bool modified = false; + + blksize_t offset = 0; + while (offset < blksize()) + { + auto& entry = block_buffer.span().slice(offset).as(); + + if (entry.inode) + { + BAN::StringView entry_name(entry.name, entry.name_len); + + if (entry_name == "."sv) + { + m_inode.links_count--; + sync(); + } + else if (entry_name == ".."sv) + { + auto parent = TRY(Ext2Inode::create(m_fs, entry.inode)); + parent->m_inode.links_count--; + parent->sync(); + } + else + ASSERT_NOT_REACHED(); + + modified = true; + entry.inode = 0; + } + + offset += entry.rec_len; + } + + if (modified) + m_fs.write_block(block, block_buffer); + } + + return {}; + } + BAN::ErrorOr Ext2Inode::unlink_impl(BAN::StringView name) { ASSERT(mode().ifdir()); + if (m_inode.flags & Ext2::Enum::INDEX_FL) + { + dwarnln("deletion from indexed directory is not supported"); + return BAN::Error::from_errno(ENOTSUP); + } auto block_buffer = m_fs.get_block_buffer(); @@ -546,15 +631,31 @@ needs_new_block: while (offset < blksize()) { auto& entry = block_buffer.span().slice(offset).as(); - if (entry.inode && name == entry.name) + if (entry.inode && name == BAN::StringView(entry.name, entry.name_len)) { auto inode = TRY(Ext2Inode::create(m_fs, entry.inode)); + if (inode->mode().ifdir()) + { + if (!TRY(inode->is_directory_empty())) + return BAN::Error::from_errno(ENOTEMPTY); + TRY(inode->cleanup_default_links()); + } + if (inode->nlink() == 0) dprintln("Corrupted filesystem. Deleting inode with 0 links"); else inode->m_inode.links_count--; - TRY(sync()); + sync(); + + // NOTE: If this was the last link to inode we must + // remove it from inode cache to trigger cleanup + if (inode->nlink() == 0) + { + auto& cache = m_fs.inode_cache(); + if (cache.contains(inode->ino())) + cache.remove(inode->ino()); + } // FIXME: This should expand the last inode if exists entry.inode = 0; @@ -620,7 +721,7 @@ needs_new_block: { if (mode().ifdir()) m_inode.size += blksize(); - MUST(sync()); + sync(); }; // direct block @@ -676,7 +777,7 @@ needs_new_block: #undef READ_OR_ALLOCATE_INDIRECT_BLOCK #undef WRITE_BLOCK_AND_RETURN - BAN::ErrorOr Ext2Inode::sync() + void Ext2Inode::sync() { auto inode_location = m_fs.locate_inode(ino()); auto block_buffer = m_fs.get_block_buffer(); @@ -687,8 +788,6 @@ needs_new_block: memcpy(block_buffer.data() + inode_location.offset, &m_inode, sizeof(Ext2::Inode)); m_fs.write_block(inode_location.block, block_buffer); } - - return {}; } BAN::ErrorOr> Ext2Inode::find_inode_impl(BAN::StringView file_name) diff --git a/kernel/kernel/FS/Inode.cpp b/kernel/kernel/FS/Inode.cpp index 535a9d354e..7227934285 100644 --- a/kernel/kernel/FS/Inode.cpp +++ b/kernel/kernel/FS/Inode.cpp @@ -103,6 +103,8 @@ namespace Kernel Thread::TerminateBlocker blocker(Thread::current()); if (!mode().ifdir()) return BAN::Error::from_errno(ENOTDIR); + if (name == "."sv || name == ".."sv) + return BAN::Error::from_errno(EINVAL); return unlink_impl(name); }