From abe3f515b16a9bf32dd642a123698f70aab76bbe Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 3 Feb 2019 01:36:25 +0100 Subject: [PATCH] Make font loading use mmap(). This exposed a serious race condition in page_in_from_inode(). Reordered the logic and added a paging lock to VMObject. Now, only one process can page in from a VMObject at a time. There are definitely ways to optimize this, for instance by making the locking be per-page instead. It's not something that I'm going to worry about right now though. --- Kernel/MemoryManager.cpp | 51 +++++++++++++++++++++++++++------------- Kernel/MemoryManager.h | 2 ++ SharedGraphics/Font.cpp | 49 ++++++++++++++++++++------------------ SharedGraphics/Font.h | 2 ++ 4 files changed, 65 insertions(+), 39 deletions(-) diff --git a/Kernel/MemoryManager.cpp b/Kernel/MemoryManager.cpp index c7524e7ab5..a992bbf519 100644 --- a/Kernel/MemoryManager.cpp +++ b/Kernel/MemoryManager.cpp @@ -301,9 +301,41 @@ bool MemoryManager::page_in_from_inode(Region& region, unsigned page_index_in_re auto& vmo = region.vmo(); ASSERT(!vmo.is_anonymous()); ASSERT(vmo.inode()); - auto& inode = *vmo.inode(); + auto& vmo_page = vmo.physical_pages()[region.first_page_index() + page_index_in_region]; - ASSERT(vmo_page.is_null()); + + bool interrupts_were_enabled = are_interrupts_enabled(); + + if (!interrupts_were_enabled) + sti(); + + LOCKER(vmo.m_paging_lock); + + if (!interrupts_were_enabled) + cli(); + + if (!vmo_page.is_null()) { + kprintf("MM: page_in_from_inode was served by someone else while lock was held\n"); + remap_region_page(region, page_index_in_region, true); + return true; + } + +#ifdef MM_DEBUG + dbgprintf("MM: page_in_from_inode ready to read from inode, will write to L%x!\n", dest_ptr); +#endif + sti(); // Oh god here we go... + byte page_buffer[PAGE_SIZE]; + auto& inode = *vmo.inode(); + auto nread = inode.read_bytes(vmo.inode_offset() + ((region.first_page_index() + page_index_in_region) * PAGE_SIZE), PAGE_SIZE, page_buffer, nullptr); + if (nread < 0) { + kprintf("MM: page_in_from_inode had error (%d) while reading!\n", nread); + return false; + } + if (nread < PAGE_SIZE) { + // If we read less than a page, zero out the rest to avoid leaking uninitialized data. + memset(page_buffer + nread, 0, PAGE_SIZE - nread); + } + cli(); vmo_page = allocate_physical_page(ShouldZeroFill::No); if (vmo_page.is_null()) { kprintf("MM: page_in_from_inode was unable to allocate a physical page\n"); @@ -311,20 +343,7 @@ bool MemoryManager::page_in_from_inode(Region& region, unsigned page_index_in_re } remap_region_page(region, page_index_in_region, true); byte* dest_ptr = region.laddr().offset(page_index_in_region * PAGE_SIZE).as_ptr(); -#ifdef MM_DEBUG - dbgprintf("MM: page_in_from_inode ready to read from inode, will write to L%x!\n", dest_ptr); -#endif - sti(); // Oh god here we go... - auto nread = inode.read_bytes(vmo.inode_offset() + ((region.first_page_index() + page_index_in_region) * PAGE_SIZE), PAGE_SIZE, dest_ptr, nullptr); - if (nread < 0) { - kprintf("MM: page_in_from_inode had error (%d) while reading!\n", nread); - return false; - } - if (nread < PAGE_SIZE) { - // If we read less than a page, zero out the rest to avoid leaking uninitialized data. - memset(dest_ptr + nread, 0, PAGE_SIZE - nread); - } - cli(); + memcpy(dest_ptr, page_buffer, PAGE_SIZE); return true; } diff --git a/Kernel/MemoryManager.h b/Kernel/MemoryManager.h index 215ffc1a2e..de189e9f8d 100644 --- a/Kernel/MemoryManager.h +++ b/Kernel/MemoryManager.h @@ -77,6 +77,7 @@ private: }; class VMObject : public Retainable { + friend class MemoryManager; public: static RetainPtr create_file_backed(RetainPtr&&, size_t); static RetainPtr create_anonymous(size_t); @@ -108,6 +109,7 @@ private: size_t m_size { 0 }; RetainPtr m_inode; Vector> m_physical_pages; + Lock m_paging_lock; }; class Region : public Retainable { diff --git a/SharedGraphics/Font.cpp b/SharedGraphics/Font.cpp index 7dee9bc68c..bc146b97fb 100644 --- a/SharedGraphics/Font.cpp +++ b/SharedGraphics/Font.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #endif #define DEFAULT_FONT_NAME Liza8x10 @@ -101,43 +102,26 @@ struct FontFileHeader { char name[64]; } PACKED; -RetainPtr Font::load_from_file(const String& path) +RetainPtr Font::load_from_memory(const byte* data) { - int fd = open(path.characters(), O_RDONLY, 0644); - if (fd < 0) { - dbgprintf("open(%s) got fd=%d, failed: %s\n", path.characters(), fd, strerror(errno)); - perror("open"); - return nullptr; - } - - FontFileHeader header; - ssize_t nread = read(fd, &header, sizeof(FontFileHeader)); - if (nread != sizeof(FontFileHeader)) { - dbgprintf("nread != sizeof(FontFileHeader)=%u\n", sizeof(FontFileHeader)); - return nullptr; - } - + auto& header = *reinterpret_cast(data); if (memcmp(header.magic, "!Fnt", 4)) { - dbgprintf("header.magic != '!Fnt'\n"); + dbgprintf("header.magic != '!Fnt', instead it's '%c%c%c%c'\n", header.magic[0], header.magic[1], header.magic[2], header.magic[3]); return nullptr; } - if (header.name[63] != '\0') { dbgprintf("Font name not fully null-terminated\n"); return nullptr; } + auto* glyphs_ptr = reinterpret_cast(data + sizeof(FontFileHeader)); + char** new_glyphs = static_cast(kmalloc(sizeof(char*) * 256)); for (unsigned glyph_index = 0; glyph_index < 256; ++glyph_index) { new_glyphs[glyph_index] = static_cast(kmalloc(header.glyph_width * header.glyph_height)); char* bitptr = new_glyphs[glyph_index]; for (unsigned y = 0; y < header.glyph_height; ++y) { - unsigned pattern; - ssize_t nread = read(fd, &pattern, sizeof(unsigned)); - if (nread != sizeof(unsigned)) { - // FIXME: free() things and return nullptr. - ASSERT_NOT_REACHED(); - } + unsigned pattern = *(glyphs_ptr++); for (unsigned x = 0; x < header.glyph_width; ++x) { if (pattern & (1u << x)) { *(bitptr++) = '#'; @@ -151,6 +135,25 @@ RetainPtr Font::load_from_file(const String& path) return adopt(*new Font(String(header.name), new_glyphs, header.glyph_width, header.glyph_height, 0, 255)); } +RetainPtr Font::load_from_file(const String& path) +{ + int fd = open(path.characters(), O_RDONLY, 0644); + if (fd < 0) { + dbgprintf("open(%s) got fd=%d, failed: %s\n", path.characters(), fd, strerror(errno)); + perror("open"); + return nullptr; + } + + auto* mapped_file = (byte*)mmap(nullptr, 4096 * 3, PROT_READ, MAP_SHARED, fd, 0); + if (mapped_file == MAP_FAILED) + return nullptr; + + auto font = load_from_memory(mapped_file); + int rc = munmap(mapped_file, 4096 * 3); + ASSERT(rc == 0); + return font; +} + bool Font::write_to_file(const String& path) { int fd = open(path.characters(), O_WRONLY | O_CREAT, 0644); diff --git a/SharedGraphics/Font.h b/SharedGraphics/Font.h index 847a368533..08a7cb3ec3 100644 --- a/SharedGraphics/Font.h +++ b/SharedGraphics/Font.h @@ -12,6 +12,8 @@ public: RetainPtr clone() const; + static RetainPtr load_from_memory(const byte*); + #ifdef USERLAND static RetainPtr load_from_file(const String& path); bool write_to_file(const String& path);