From 50ad7ba1f6298cb5b8bea0b10ab3e23378d27b50 Mon Sep 17 00:00:00 2001 From: Eladash Date: Sat, 11 Sep 2021 08:26:08 +0300 Subject: [PATCH] vm: Fix vm::unmap * Make vm::unmap atomic, squash the memory unmapping process inside this function while still using the same VM mutex ownership. * Make vm::unmap not fail due to random vm::get calls, shared_ptr reference count is no longer a condition. * Fix sys_mmapper_free_address spuriously failing with EBUSY due to random vm::get calls. * Fix sys_vm_unmap race condition due to non-atomic vm::unmap. * Add an optional verification block ptr arg to vm::unmap, used by patches. --- Utilities/bin_patch.cpp | 6 +-- rpcs3/Emu/Cell/PPUThread.cpp | 2 +- rpcs3/Emu/Cell/lv2/sys_mmapper.cpp | 26 ++++++++++-- rpcs3/Emu/Cell/lv2/sys_vm.cpp | 2 +- rpcs3/Emu/Memory/vm.cpp | 68 +++++++++++++++++++++++++----- rpcs3/Emu/Memory/vm.h | 18 +++++++- 6 files changed, 99 insertions(+), 23 deletions(-) diff --git a/Utilities/bin_patch.cpp b/Utilities/bin_patch.cpp index ac120104b9..37d98fa05d 100644 --- a/Utilities/bin_patch.cpp +++ b/Utilities/bin_patch.cpp @@ -558,9 +558,7 @@ void unmap_vm_area(std::shared_ptr& ptr) { if (ptr && ptr->flags & (1ull << 62)) { - const u32 addr = ptr->addr; - ptr.reset(); - vm::unmap(addr, true); + vm::unmap(0, true, &ptr); } } @@ -679,7 +677,7 @@ static usz apply_modification(std::basic_string& applied, const patch_engin const u32 out_branch = vm::try_get_addr(dst + (offset & -4)).first; // Allow only if points to a PPU executable instruction - if (out_branch < 0x10000 || out_branch >= 0x4000'0000 || !vm::check_addr<4>(out_branch, vm::alloc_executable)) + if (out_branch < 0x10000 || out_branch >= 0x4000'0000 || !vm::check_addr<4>(out_branch, vm::page_executable)) { continue; } diff --git a/rpcs3/Emu/Cell/PPUThread.cpp b/rpcs3/Emu/Cell/PPUThread.cpp index 4aff0f09d7..c363258654 100644 --- a/rpcs3/Emu/Cell/PPUThread.cpp +++ b/rpcs3/Emu/Cell/PPUThread.cpp @@ -2688,7 +2688,7 @@ extern void ppu_precompile(std::vector& dir_queue, std::vectorget(); std::lock_guard pf_lock(pf_events.pf_mutex); + const auto mem = vm::get(vm::any, addr); + + if (!mem || mem->addr != addr) + { + return {CELL_EINVAL, addr}; + } + for (const auto& ev : pf_events.events) { - auto mem = vm::get(vm::any, addr); - if (mem && addr <= ev.second && ev.second <= addr + mem->size - 1) + if (addr <= ev.second && ev.second <= addr + mem->size - 1) { return CELL_EBUSY; } } // Try to unmap area - const auto area = vm::unmap(addr, true); + const auto [area, success] = vm::unmap(addr, true, &mem); if (!area) { return {CELL_EINVAL, addr}; } - if (area.use_count() != 1) + if (!success) { return CELL_EBUSY; } @@ -586,6 +592,12 @@ error_code sys_mmapper_map_shared_memory(ppu_thread& ppu, u32 addr, u32 mem_id, if (!area->falloc(addr, mem->size, &mem->shm, mem->align == 0x10000 ? SYS_MEMORY_PAGE_SIZE_64K : SYS_MEMORY_PAGE_SIZE_1M)) { mem->counter--; + + if (!area->is_valid()) + { + return {CELL_EINVAL, addr}; + } + return CELL_EBUSY; } @@ -634,6 +646,12 @@ error_code sys_mmapper_search_and_map(ppu_thread& ppu, u32 start_addr, u32 mem_i if (!addr) { mem->counter--; + + if (!area->is_valid()) + { + return {CELL_EINVAL, start_addr}; + } + return CELL_ENOMEM; } diff --git a/rpcs3/Emu/Cell/lv2/sys_vm.cpp b/rpcs3/Emu/Cell/lv2/sys_vm.cpp index e016e90d2d..5503a87e58 100644 --- a/rpcs3/Emu/Cell/lv2/sys_vm.cpp +++ b/rpcs3/Emu/Cell/lv2/sys_vm.cpp @@ -116,7 +116,7 @@ error_code sys_vm_unmap(ppu_thread& ppu, u32 addr) const auto vmo = idm::withdraw(sys_vm_t::find_id(addr), [&](sys_vm_t& vmo) { // Free block - ensure(vm::unmap(addr)); + ensure(vm::unmap(addr).second); // Return memory vmo.ct->used -= vmo.psize; diff --git a/rpcs3/Emu/Memory/vm.cpp b/rpcs3/Emu/Memory/vm.cpp index b468c71ea3..82197099a2 100644 --- a/rpcs3/Emu/Memory/vm.cpp +++ b/rpcs3/Emu/Memory/vm.cpp @@ -1170,7 +1170,8 @@ namespace vm } block_t::block_t(u32 addr, u32 size, u64 flags) - : addr(addr) + : m_id([](){ static atomic_t s_id = 1; return s_id++; }()) + , addr(addr) , size(size) , flags(process_block_flags(flags)) { @@ -1183,12 +1184,12 @@ namespace vm } } - block_t::~block_t() + bool block_t::unmap() { auto& m_map = (m.*block_map)(); - { - vm::writer_lock lock(0); + if (m_id.exchange(0)) + { // Deallocate all memory for (auto it = m_map.begin(), end = m_map.end(); it != end;) { @@ -1205,7 +1206,16 @@ namespace vm m_common->unmap_critical(vm::get_super_ptr(addr)); #endif } + + return true; } + + return false; + } + + block_t::~block_t() + { + ensure(!is_valid()); } u32 block_t::alloc(const u32 orig_size, const std::shared_ptr* src, u32 align, u64 flags) @@ -1257,6 +1267,12 @@ namespace vm vm::writer_lock lock(0); + if (!is_valid()) + { + // Expired block + return 0; + } + // Search for an appropriate place (unoptimized) for (;; addr += align) { @@ -1321,6 +1337,12 @@ namespace vm vm::writer_lock lock(0); + if (!is_valid()) + { + // Expired block + return 0; + } + if (!try_alloc(addr, flags, size, std::move(shm))) { return 0; @@ -1569,8 +1591,15 @@ namespace vm return block; } - std::shared_ptr unmap(u32 addr, bool must_be_empty) + std::pair, bool> unmap(u32 addr, bool must_be_empty, const std::shared_ptr* ptr) { + if (ptr) + { + addr = (*ptr)->addr; + } + + std::pair, bool> result{}; + vm::writer_lock lock(0); for (auto it = g_locations.begin() + memory_location_max; it != g_locations.end(); it++) @@ -1587,18 +1616,26 @@ namespace vm continue; } - if (must_be_empty && (it->use_count() != 1 || (*it)->imp_used(lock))) + if (ptr && *it != *ptr) { - return *it; + return {}; } - auto block = std::move(*it); + if (must_be_empty && (*it)->imp_used(lock)) + { + result.first = *it; + return result; + } + + result.first = std::move(*it); g_locations.erase(it); - return block; + ensure(result.first->unmap()); + result.second = true; + return result; } } - return nullptr; + return {}; } std::shared_ptr get(memory_location_t location, u32 addr) @@ -1730,7 +1767,16 @@ namespace vm void close() { - g_locations.clear(); + { + vm::writer_lock lock(0); + + for (auto& block : g_locations) + { + if (block) block->unmap(); + } + + g_locations.clear(); + } utils::memory_decommit(g_base_addr, 0x200000000); utils::memory_decommit(g_exec_addr, 0x200000000); diff --git a/rpcs3/Emu/Memory/vm.h b/rpcs3/Emu/Memory/vm.h index da4d47dd20..8f064149bf 100644 --- a/rpcs3/Emu/Memory/vm.h +++ b/rpcs3/Emu/Memory/vm.h @@ -123,8 +123,13 @@ namespace vm // Common mapped region for special cases std::shared_ptr m_common; + atomic_t m_id = 0; + bool try_alloc(u32 addr, u64 bflags, u32 size, std::shared_ptr&&) const; + // Unmap block + bool unmap(); + public: block_t(u32 addr, u32 size, u64 flags); @@ -155,6 +160,15 @@ namespace vm // Internal u32 imp_used(const vm::writer_lock&) const; + + // Returns 0 if invalid, none-zero unique id if valid + u64 is_valid() const + { + return m_id; + } + + friend std::pair, bool> unmap(u32, bool, const std::shared_ptr*); + friend void close(); }; // Create new memory block with specified parameters and return it @@ -163,8 +177,8 @@ namespace vm // Create new memory block with at arbitrary position with specified alignment std::shared_ptr find_map(u32 size, u32 align, u64 flags = 0); - // Delete existing memory block with specified start address, return it - std::shared_ptr unmap(u32 addr, bool must_be_empty = false); + // Delete existing memory block with specified start address, .first=its ptr, .second=success + std::pair, bool> unmap(u32 addr, bool must_be_empty = false, const std::shared_ptr* ptr = nullptr); // Get memory block associated with optionally specified memory location or optionally specified address std::shared_ptr get(memory_location_t location, u32 addr = 0);