From e1e747984f6d6f71435da1606101940cf4b9dbac Mon Sep 17 00:00:00 2001 From: Kalesh Singh Date: Tue, 9 Apr 2024 11:48:52 -0700 Subject: [PATCH 1/3] bionic: loader: Don't extend LOAD segments for p_aligns > 64KiB Loader segment extension was introduced to fix kernel slab memory regressions in page-agnostic Android. This reression was due to required VMAs for the gap VMAs that exist when the elf-max-page-size > runtime-page-size. This issue already existed for some libraries like libart.so and libhwui.so which use 2mB alignment to make use of THPs (transparent huge pages). Later it was found that this optimization could break in-field apps due to dependencies and the address ranges in /proc/*/[s]maps. To avoid breaking the in-field apps, the kernel can work around the compatibility issues if made aware of where the padding regions exist. However, the kernel can only represent padding for p_align up to 64KiB. This is because the kernel uses 4 available bits in the vm_area_struct to represent padding extent; and so cannot enable mitigations to avoid breaking app compatibility for p_aligns > 64KiB. For ELFs with p_align > 64KiB, don't do segment extension, to avoid issues with app compatibility -- these ELFs already exist with gap mappings and are not part of the page size transition VMA slab regression. Bug: 330117029 Bug: 327600007 Bug: 330767927 Bug: 328266487 Bug: 329803029 Test: Manual - Launch Free Fire Chaos app Change-Id: Id4dcced4632dce67adab6348816f85847ce1de58 Signed-off-by: Kalesh Singh --- linker/linker_phdr.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/linker/linker_phdr.cpp b/linker/linker_phdr.cpp index ef7671cee..685ae7082 100644 --- a/linker/linker_phdr.cpp +++ b/linker/linker_phdr.cpp @@ -773,7 +773,16 @@ static inline void _extend_load_segment_vma(const ElfW(Phdr)* phdr_table, size_t const ElfW(Phdr)* next = nullptr; size_t next_idx = phdr_idx + 1; - if (phdr->p_align == kPageSize || !should_pad_segments) { + // Don't do segment extension for p_align > 64KiB, such ELFs already existed in the + // field e.g. 2MiB p_align for THPs and are relatively small in number. + // + // The kernel can only represent padding for p_align up to 64KiB. This is because + // the kernel uses 4 available bits in the vm_area_struct to represent padding + // extent; and so cannot enable mitigations to avoid breaking app compatibility for + // p_aligns > 64KiB. + // + // Don't perform segment extension on these to avoid app compatibility issues. + if (phdr->p_align <= kPageSize || phdr->p_align > 64*1024 || !should_pad_segments) { return; } From c5c1d19ebb6d675f3028906be34d447c14ffbce5 Mon Sep 17 00:00:00 2001 From: Kalesh Singh Date: Tue, 9 Apr 2024 16:27:56 -0700 Subject: [PATCH 2/3] loader: Only extend segments if kernel supports page size migration It has been found that some existing apps have implicit dependencies on the address ranges in /proc/*/maps. Since segment extension changes the range of the LOAD segment VMAs some of these apps crash, either by SIGBUS or in yet unidentified ways. Only perform the segment extension optimization if the kernel has the necessary mitigations to ensure app compatibility. Bug: 330117029 Bug: 327600007 Bug: 330767927 Bug: 328266487 Bug: 329803029 Test: Manual - Launch Free Fire Chaos app Change-Id: I5b03b22c4a468f6646750a00942cc2d57f43d0de Signed-off-by: Kalesh Singh --- linker/linker_crt_pad_segment_test.cpp | 9 +++++++++ linker/linker_phdr.cpp | 22 ++++++++++++++++++++++ linker/linker_phdr.h | 2 ++ 3 files changed, 33 insertions(+) diff --git a/linker/linker_crt_pad_segment_test.cpp b/linker/linker_crt_pad_segment_test.cpp index 5a219f8ee..c11df50fe 100644 --- a/linker/linker_crt_pad_segment_test.cpp +++ b/linker/linker_crt_pad_segment_test.cpp @@ -72,13 +72,22 @@ bool GetPadSegment(const std::string& elf_path) { }; // anonymous namespace TEST(crt_pad_segment, note_absent) { + if (!page_size_migration_supported()) { + GTEST_SKIP() << "Kernel does not support page size migration"; + } ASSERT_FALSE(GetPadSegment("no_crt_pad_segment.so")); } TEST(crt_pad_segment, note_present_and_enabled) { + if (!page_size_migration_supported()) { + GTEST_SKIP() << "Kernel does not support page size migration"; + } ASSERT_TRUE(GetPadSegment("crt_pad_segment_enabled.so")); } TEST(crt_pad_segment, note_present_and_disabled) { + if (!page_size_migration_supported()) { + GTEST_SKIP() << "Kernel does not support page size migration"; + } ASSERT_FALSE(GetPadSegment("crt_pad_segment_disabled.so")); } diff --git a/linker/linker_phdr.cpp b/linker/linker_phdr.cpp index 685ae7082..80f1ad9d4 100644 --- a/linker/linker_phdr.cpp +++ b/linker/linker_phdr.cpp @@ -46,6 +46,8 @@ #include "private/CFIShadow.h" // For kLibraryAlignment #include "private/elf_note.h" +#include + static int GetTargetElfMachine() { #if defined(__arm__) return EM_ARM; @@ -707,8 +709,28 @@ bool ElfReader::ReserveAddressSpace(address_space_params* address_space) { return true; } +/* + * Returns true if the kernel supports page size migration, else false. + */ +bool page_size_migration_supported() { + static bool pgsize_migration_enabled = []() { + std::string enabled; + if (!android::base::ReadFileToString("/sys/kernel/mm/pgsize_migration/enabled", &enabled)) { + return false; + } + return enabled.find("1") != std::string::npos; + }(); + return pgsize_migration_enabled; +} + // Find the ELF note of type NT_ANDROID_TYPE_PAD_SEGMENT and check that the desc value is 1. bool ElfReader::ReadPadSegmentNote() { + if (!page_size_migration_supported()) { + // Don't attempt to read the note, since segment extension isn't + // supported; but return true so that loading can continue normally. + return true; + } + // The ELF can have multiple PT_NOTE's, check them all for (size_t i = 0; i < phdr_num_; ++i) { const ElfW(Phdr)* phdr = &phdr_table_[i]; diff --git a/linker/linker_phdr.h b/linker/linker_phdr.h index 61242eb4c..aab9018b4 100644 --- a/linker/linker_phdr.h +++ b/linker/linker_phdr.h @@ -154,3 +154,5 @@ void phdr_table_get_dynamic_section(const ElfW(Phdr)* phdr_table, size_t phdr_co const char* phdr_table_get_interpreter_name(const ElfW(Phdr)* phdr_table, size_t phdr_count, ElfW(Addr) load_bias); + +bool page_size_migration_supported(); From 5134762efa7be5d9d4006dba0d91039237bb7b2c Mon Sep 17 00:00:00 2001 From: Kalesh Singh Date: Mon, 18 Mar 2024 17:27:59 -0700 Subject: [PATCH 3/3] bionic: loader: Drop readahead padding pages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These are padding pages are only needed to layout the ELF to be compatible with max-page-size. They are zero-filled (holes) and can be dropped from the page cache. The madvise() here is a special case that also serves to hint to the kernel what part of the segment is padding. For example the kernel then shows these padding regions as PROT_NONE VMAs (labeled [page size compat]) in /proc/*/maps. Note: This doesn't use backing vm_area_structs, so doesn't consume additional slab memory. Before: ❯ cf-adb shell cat /proc/1/maps | grep -A1 'libbase.so$' 7f8d13600000-7f8d13614000 r--p 00000000 fe:09 21909460 /system/lib64/libbase.so 7f8d13614000-7f8d13638000 r-xp 00014000 fe:09 21909460 /system/lib64/libbase.so 7f8d13638000-7f8d1363c000 r--p 00038000 fe:09 21909460 /system/lib64/libbase.so 7f8d1363c000-7f8d1363d000 rw-p 0003c000 fe:09 21909460 /system/lib64/libbase.so Segments appear extended in /proc//maps After: ❯ cf-adb shell cat /proc/1/maps | grep -A1 'libbase.so$' 7f3650043000-7f3650054000 r--p 00000000 fe:09 21906900 /system/lib64/libbase.so 7f3650054000-7f3650057000 ---p 00000000 00:00 0 [page size compat] 7f3650057000-7f3650079000 r-xp 00014000 fe:09 21906900 /system/lib64/libbase.so 7f3650079000-7f365007b000 ---p 00000000 00:00 0 [page size compat] 7f365007b000-7f365007c000 r--p 00038000 fe:09 21906900 /system/lib64/libbase.so 7f365007c000-7f365007f000 ---p 00000000 00:00 0 [page size compat] 7f365007f000-7f3650080000 rw-p 0003c000 fe:09 21906900 /system/lib64/libbase.so Segments maintain PROT_NONE gaps ("[page size compat]") for app compatiblity but these are not backed by actual slab VMA memory. Bug: 330117029 Bug: 327600007 Bug: 330767927 Bug: 328266487 Bug: 329803029 Test: Manual - Launch Free Fire Chaos app Change-Id: Ic50540e247b4294eb08f8cf70e74bd2bf6606684 Signed-off-by: Kalesh Singh --- linker/linker_phdr.cpp | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/linker/linker_phdr.cpp b/linker/linker_phdr.cpp index 80f1ad9d4..fa712a10a 100644 --- a/linker/linker_phdr.cpp +++ b/linker/linker_phdr.cpp @@ -918,10 +918,28 @@ bool ElfReader::LoadSegments() { // 2) Break the COW backing, faulting in new anon pages for a region // that will not be used. - // _seg_file_end = unextended seg_file_end - uint64_t _seg_file_end = seg_start + phdr->p_filesz; - if ((phdr->p_flags & PF_W) != 0 && page_offset(_seg_file_end) > 0) { - memset(reinterpret_cast(_seg_file_end), 0, kPageSize - page_offset(_seg_file_end)); + uint64_t unextended_seg_file_end = seg_start + phdr->p_filesz; + if ((phdr->p_flags & PF_W) != 0 && page_offset(unextended_seg_file_end) > 0) { + memset(reinterpret_cast(unextended_seg_file_end), 0, + kPageSize - page_offset(unextended_seg_file_end)); + } + + // Pages may be brought in due to readahead. + // Drop the padding (zero) pages, to avoid reclaim work later. + // + // NOTE: The madvise() here is special, as it also serves to hint to the + // kernel the portion of the LOAD segment that is padding. + // + // See: [1] https://android-review.googlesource.com/c/kernel/common/+/3032411 + // [2] https://android-review.googlesource.com/c/kernel/common/+/3048835 + uint64_t pad_start = page_end(unextended_seg_file_end); + uint64_t pad_end = page_end(seg_file_end); + CHECK(pad_start <= pad_end); + uint64_t pad_len = pad_end - pad_start; + if (page_size_migration_supported() && pad_len > 0 && + madvise(reinterpret_cast(pad_start), pad_len, MADV_DONTNEED)) { + DL_WARN("\"%s\": madvise(0x%" PRIx64 ", 0x%" PRIx64 ", MADV_DONTNEED) failed: %m", + name_.c_str(), pad_start, pad_len); } seg_file_end = page_end(seg_file_end);