From a5d9e62cc6e5d0b4473726cda6518e34e27a724b Mon Sep 17 00:00:00 2001 From: Ryan Houdek Date: Tue, 26 Jan 2021 02:54:14 -0800 Subject: [PATCH] More improvements to BRK handling Base size is now only one page in size. We will then increment that BRK size by 8MB alignments. 256MB for 32bit applications was causing some applications on the edge to run out of virtual memory I was hitting some 32bit applications that were being fairly mean with BRK. They were allocating all of BRK space then running out of virtual memory space with its mmap handler fallback after freeing BRK space. This means we now munmap BRK pages on release for the guest, similar to behaviour that Linux does. Additionally I had an application that was getting very upset that BRK wasn't actually at the end of program space. So allocate at the end of program space like expected. brk test now passes from gvisor --- External/FEXCore/Source/Utils/ELFLoader.cpp | 12 ++++ .../FEXCore/include/FEXCore/Utils/ELFLoader.h | 20 +++++++ Source/Tests/ELFLoader.cpp | 3 +- Source/Tests/HarnessHelpers.h | 15 +++-- Source/Tests/LinuxSyscalls/Syscalls.cpp | 59 +++++++++++++++---- Source/Tests/LinuxSyscalls/Syscalls.h | 6 +- unittests/gvisor-tests/Known_Failures | 1 - 7 files changed, 96 insertions(+), 20 deletions(-) diff --git a/External/FEXCore/Source/Utils/ELFLoader.cpp b/External/FEXCore/Source/Utils/ELFLoader.cpp index 6c1274c28..049c3c1e8 100644 --- a/External/FEXCore/Source/Utils/ELFLoader.cpp +++ b/External/FEXCore/Source/Utils/ELFLoader.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -201,6 +202,9 @@ bool ELFContainer::LoadELF_32() { DynamicProgram = Header._32.e_type != ET_EXEC; + // Default BRK size + BRKSize = 4096; + return true; } @@ -239,6 +243,9 @@ bool ELFContainer::LoadELF_64() { DynamicProgram = Header._64.e_type != ET_EXEC; + // Default BRK size + BRKSize = 0x1000'0000; + return true; } @@ -330,6 +337,11 @@ void ELFContainer::CalculateMemoryLayouts() { } } + // Calculate BRK + MaxPhysAddr = AlignUp(MaxPhysAddr, 4096); + BRKBase = MaxPhysAddr; + MaxPhysAddr += BRKSize; + PhysMemSize = MaxPhysAddr - MinPhysAddr; MinPhysicalMemoryLocation = MinPhysAddr; diff --git a/External/FEXCore/include/FEXCore/Utils/ELFLoader.h b/External/FEXCore/include/FEXCore/Utils/ELFLoader.h index 5ba92a867..dbdfd59f9 100644 --- a/External/FEXCore/include/FEXCore/Utils/ELFLoader.h +++ b/External/FEXCore/include/FEXCore/Utils/ELFLoader.h @@ -40,6 +40,15 @@ public: PhysicalMemorySize); } + struct BRKInfo { + uint64_t Base; + uint64_t Size; + }; + + BRKInfo GetBRKInfo() const { + return {BRKBase, BRKSize}; + } + // Data, Physical, Size using MemoryWriter = std::function; void WriteLoadableSections(MemoryWriter Writer, uint64_t Offset = 0); @@ -67,6 +76,14 @@ public: void GetInitLocations(uint64_t GuestELFBase, std::vector *Locations); bool HasTLS() const { return TLSHeader._64 != nullptr; } + uint64_t GetTLSBase() const { + if (GetMode() == ELFMode::MODE_64BIT) { + return TLSHeader._64->p_vaddr; + } + else { + return TLSHeader._32->p_vaddr; + } + } enum ELFMode { MODE_32BIT, @@ -122,6 +139,9 @@ private: uint64_t MinPhysicalMemoryLocation{0}; uint64_t MaxPhysicalMemoryLocation{0}; uint64_t PhysicalMemorySize{0}; + + uint64_t BRKBase{}; + uint64_t BRKSize{}; ProgramHeader InterpreterHeader{}; bool DynamicProgram{false}; std::string DynamicLinker; diff --git a/Source/Tests/ELFLoader.cpp b/Source/Tests/ELFLoader.cpp index 6fedaa39e..be54f1f16 100644 --- a/Source/Tests/ELFLoader.cpp +++ b/Source/Tests/ELFLoader.cpp @@ -207,7 +207,6 @@ int main(int argc, char **argv, char **const envp) { FEXCore::Config::Value ABILocalFlags{FEXCore::Config::CONFIG_ABI_LOCAL_FLAGS, false}; FEXCore::Config::Value AbiNoPF{FEXCore::Config::CONFIG_ABI_NO_PF, false}; - ::SilentLog = SilentLog(); if (!::SilentLog) { @@ -269,6 +268,8 @@ int main(int argc, char **argv, char **const envp) { CTX, SignalDelegation.get(), &Loader)}; + auto BRKInfo = Loader.GetBRKInfo(); + SyscallHandler->DefaultProgramBreak(BRKInfo.Base, BRKInfo.Size); FEXCore::Context::SetSignalDelegator(CTX, SignalDelegation.get()); FEXCore::Context::SetSyscallHandler(CTX, SyscallHandler.get()); diff --git a/Source/Tests/HarnessHelpers.h b/Source/Tests/HarnessHelpers.h index 07b046912..c75f53242 100644 --- a/Source/Tests/HarnessHelpers.h +++ b/Source/Tests/HarnessHelpers.h @@ -495,13 +495,13 @@ public: //AuxVariables.emplace_back(auxv_t{24, ~0ULL}); // AT_PLATFORM // On x86 only allows userspace to check for monitor and fs/gs base writing in CPL3 //AuxVariables.emplace_back(auxv_t{26, 0}); // AT_HWCAP2 - AuxVariables.emplace_back(auxv_t{32, 0ULL}); // sysinfo (vDSO) - AuxVariables.emplace_back(auxv_t{33, 0ULL}); // sysinfo (vDSO) + AuxVariables.emplace_back(auxv_t{32, 0}); // AT_SYSINFO - Entry point to syscall + AuxVariables.emplace_back(auxv_t{33, 0}); // AT_SYSINFO_EHDR - Address of the start of VDSO } else { AuxVariables.emplace_back(auxv_t{4, 0x20}); // AT_PHENT - AuxVariables.emplace_back(auxv_t{32, 0ULL}); // sysinfo (vDSO) - AuxVariables.emplace_back(auxv_t{33, 0ULL}); // sysinfo (vDSO) + AuxVariables.emplace_back(auxv_t{32, 0}); // AT_SYSINFO - Entry point to syscall + AuxVariables.emplace_back(auxv_t{33, 0}); // AT_SYSINFO_EHDR - Address of the start of VDSO } AuxVariables.emplace_back(auxv_t{3, DB.GetElfBase()}); // Program header @@ -733,9 +733,16 @@ public: bool Is64BitMode() const { return File.GetMode() == ::ELFLoader::ELFContainer::MODE_64BIT; } + ::ELFLoader::ELFContainer::BRKInfo GetBRKInfo() const { + auto Info = File.GetBRKInfo(); + Info.Base += DB.GetElfBase(); + return Info; + } + private: ::ELFLoader::ELFContainer File; ::ELFLoader::ELFSymbolDatabase DB; + std::vector Args; std::vector EnvironmentVariables; std::vector LoaderArgs; diff --git a/Source/Tests/LinuxSyscalls/Syscalls.cpp b/Source/Tests/LinuxSyscalls/Syscalls.cpp index e08b7bf1e..bd3b840cc 100644 --- a/Source/Tests/LinuxSyscalls/Syscalls.cpp +++ b/Source/Tests/LinuxSyscalls/Syscalls.cpp @@ -20,18 +20,13 @@ #include #include -#define BRK_SIZE 0x1000'0000 - namespace FEX::HLE { SyscallHandler *_SyscallHandler{}; uint64_t SyscallHandler::HandleBRK(FEXCore::Core::InternalThreadState *Thread, void *Addr) { std::lock_guard lk(MMapMutex); - uint64_t Result; - if (DataSpace == 0) { - DefaultProgramBreak(Thread); - } + uint64_t Result; if (Addr == nullptr) { // Just wants to get the location of the program break atm Result = DataSpace + DataSpaceSize; @@ -46,10 +41,45 @@ uint64_t SyscallHandler::HandleBRK(FEXCore::Core::InternalThreadState *Thread, v } else { uint64_t NewSize = NewEnd - DataSpace; + uint64_t NewSizeAligned = AlignUp(NewSize, 4096); - // make sure we don't overflow to TLS storage - if (NewSize >= BRK_SIZE) - return -ENOMEM; + if (NewSizeAligned < DataSpaceMaxSize) { + // If we are shrinking the brk then munmap the ranges + // That way we gain the memory back and also give the application zero pages if it allocates again + // DataspaceMaxSize is always page aligned + + uint64_t RemainingSize = DataSpaceMaxSize - NewSizeAligned; + // We have pages we can unmap + munmap(reinterpret_cast(DataSpace + NewSizeAligned), RemainingSize); + DataSpaceMaxSize = NewSizeAligned; + } + else if (NewSize > DataSpaceMaxSize) { + constexpr static uint64_t SizeAlignment = 8 * 1024 * 1024; + uint64_t AllocateNewSize = AlignUp(NewSize, SizeAlignment) - DataSpaceMaxSize; + if (!Is64BitMode() && + (DataSpace + DataSpaceMaxSize + AllocateNewSize > 0x1'0000'0000ULL)) { + // If we are 32bit and we tried going about the 32bit limit then out of memory + return DataSpace + DataSpaceSize; + } + + uint64_t NewBRK = (uint64_t)mmap((void*)(DataSpace + DataSpaceMaxSize), AllocateNewSize, PROT_READ | PROT_WRITE, MAP_FIXED_NOREPLACE | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + + if (NewBRK != (DataSpace + DataSpaceMaxSize)) { + // Couldn't allocate that the region we wanted + // Can happen if MAP_FIXED_NOREPLACE isn't understood by the kernel + munmap(reinterpret_cast(NewBRK), AllocateNewSize); + NewBRK = ~0ULL; + } + + if (NewBRK == ~0ULL) { + // If we couldn't allocate a new region then out of memory + return DataSpace + DataSpaceSize; + } + else { + // Increase our BRK size + DataSpaceMaxSize += AllocateNewSize; + } + } DataSpaceSize = NewSize; } @@ -58,9 +88,10 @@ uint64_t SyscallHandler::HandleBRK(FEXCore::Core::InternalThreadState *Thread, v return Result; } -void SyscallHandler::DefaultProgramBreak(FEXCore::Core::InternalThreadState *Thread) { - DataSpaceSize = 0; - DataSpace = (uint64_t)mmap(nullptr, BRK_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); +void SyscallHandler::DefaultProgramBreak(uint64_t Base, uint64_t Size) { + DataSpace = Base; + DataSpaceMaxSize = Size; + DataSpaceStartingSize = Size; } SyscallHandler::SyscallHandler(FEXCore::Context::Context *ctx, FEX::HLE::SignalDelegator *_SignalDelegation) @@ -70,6 +101,10 @@ SyscallHandler::SyscallHandler(FEXCore::Context::Context *ctx, FEX::HLE::SignalD HostKernelVersion = CalculateHostKernelVersion(); } +SyscallHandler::~SyscallHandler() { + munmap(reinterpret_cast(DataSpace + DataSpaceStartingSize), DataSpaceMaxSize - DataSpaceStartingSize); +} + uint32_t SyscallHandler::CalculateHostKernelVersion() { struct utsname buf{}; if (uname(&buf) == -1) { diff --git a/Source/Tests/LinuxSyscalls/Syscalls.h b/Source/Tests/LinuxSyscalls/Syscalls.h index 3285a91d4..fc401cea3 100644 --- a/Source/Tests/LinuxSyscalls/Syscalls.h +++ b/Source/Tests/LinuxSyscalls/Syscalls.h @@ -47,12 +47,12 @@ namespace FEX::HLE { class SyscallHandler : public FEXCore::HLE::SyscallHandler { public: SyscallHandler(FEXCore::Context::Context *ctx, FEX::HLE::SignalDelegator *_SignalDelegation); - virtual ~SyscallHandler() = default; + virtual ~SyscallHandler(); // In the case that the syscall doesn't hit the optimized path then we still need to go here uint64_t HandleSyscall(FEXCore::Core::InternalThreadState *Thread, FEXCore::HLE::SyscallArguments *Args) final override; - void DefaultProgramBreak(FEXCore::Core::InternalThreadState *Thread); + void DefaultProgramBreak(uint64_t Base, uint64_t Size); using SyscallPtrArg0 = uint64_t(*)(FEXCore::Core::InternalThreadState *Thread); using SyscallPtrArg1 = uint64_t(*)(FEXCore::Core::InternalThreadState *Thread, uint64_t); @@ -116,6 +116,8 @@ protected: // BRK management uint64_t DataSpace {}; uint64_t DataSpaceSize {}; + uint64_t DataSpaceMaxSize {}; + uint64_t DataSpaceStartingSize{}; // (Major << 24) | (Minor << 16) | Patch uint32_t HostKernelVersion{}; diff --git a/unittests/gvisor-tests/Known_Failures b/unittests/gvisor-tests/Known_Failures index cf0d2978a..2c09454bc 100644 --- a/unittests/gvisor-tests/Known_Failures +++ b/unittests/gvisor-tests/Known_Failures @@ -68,7 +68,6 @@ aio_test alarm_test arch_prctl_test bad_test -brk_test chmod_test chown_test clock_nanosleep_test