From 02d2ab8f7e0751f8016d96b116a774388a2b4c48 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Fri, 16 Jul 2021 14:27:16 +0100 Subject: [PATCH] Pagemap requires registration of used space This PR exposes a pagemap interface to specify ranges that are being used. The overall invariant is that any memory in the address space manager has the pagemap committed. This means that individual operations do not need to commit entries. This is important for Windows that does not support lazy commit. It is also important if we want to PROT_NONE most of the pagemap to reduce the risk of memory safety issues getting access to the pagemap. There are minor changes to test to pull memory directly from the Pal. There are also bug fixes in the pagemap tests. --- src/backend/address_space.h | 168 +++++++++------------ src/backend/backend.h | 11 +- src/backend/pagemap.h | 55 +++---- src/mem/fixedglobalconfig.h | 2 +- src/test/func/fixed_region/fixed_region.cc | 16 +- src/test/func/pagemap/pagemap.cc | 32 ++-- src/test/func/two_alloc_types/alloc1.cc | 3 +- 7 files changed, 115 insertions(+), 172 deletions(-) diff --git a/src/backend/address_space.h b/src/backend/address_space.h index 77f45cb..d34b61e 100644 --- a/src/backend/address_space.h +++ b/src/backend/address_space.h @@ -42,8 +42,8 @@ namespace snmalloc * part of satisfying the request will be registered with the provided * arena_map for use in subsequent amplification. */ - template - CapPtr reserve(size_t size) + template + CapPtr reserve(size_t size, Pagemap& pagemap) { #ifdef SNMALLOC_TRACING std::cout << "ASM reserve request:" << size << std::endl; @@ -51,108 +51,88 @@ namespace snmalloc SNMALLOC_ASSERT(bits::is_pow2(size)); SNMALLOC_ASSERT(size >= sizeof(void*)); - if constexpr ((align == false) && !pal_supports) + /* + * For sufficiently large allocations with platforms that support + * aligned allocations and architectures that don't require + * StrictProvenance, try asking the platform first. + */ + if constexpr ( + pal_supports && !aal_supports) { - if constexpr (pal_supports) + if (size >= PAL::minimum_alloc_size) { - // TODO wasting size here. - size = bits::max(size, PAL::minimum_alloc_size); - return CapPtr( + auto base = CapPtr( PAL::template reserve_aligned(size)); - } - else - { - auto [block, size2] = PAL::reserve_at_least(size); - // TODO wasting size here. - UNUSED(size2); -#ifdef SNMALLOC_TRACING - std::cout << "Unaligned alloc here:" << block << " (" << size2 << ")" - << std::endl; -#endif - return CapPtr(block); + pagemap.register_range(address_cast(base), size); + return base; } } - else + + CapPtr res; { - /* - * For sufficiently large allocations with platforms that support - * aligned allocations and architectures that don't require - * StrictProvenance, try asking the platform first. - */ - if constexpr ( - pal_supports && - !aal_supports) + FlagLock lock(spin_lock); + res = core.template reserve(size); + if (res == nullptr) { - if (size >= PAL::minimum_alloc_size) - return CapPtr( - PAL::template reserve_aligned(size)); - } - - CapPtr res; - { - FlagLock lock(spin_lock); - res = core.template reserve(size); - if (res == nullptr) + // Allocation failed ask OS for more memory + CapPtr block = nullptr; + size_t block_size = 0; + if constexpr (pal_supports) { - // Allocation failed ask OS for more memory - CapPtr block = nullptr; - size_t block_size = 0; - if constexpr (pal_supports) - { - /* - * We will have handled the case where size >= - * minimum_alloc_size above, so we are left to handle only small - * things here. - */ - block_size = PAL::minimum_alloc_size; + /* + * We will have handled the case where size >= + * minimum_alloc_size above, so we are left to handle only small + * things here. + */ + block_size = PAL::minimum_alloc_size; - void* block_raw = - PAL::template reserve_aligned(block_size); + void* block_raw = PAL::template reserve_aligned(block_size); - // It's a bit of a lie to convert without applying bounds, but the - // platform will have bounded block for us and it's better that - // the rest of our internals expect CBChunk bounds. - block = CapPtr(block_raw); - } - else if constexpr (!pal_supports) - { - // Need at least 2 times the space to guarantee alignment. - // Hold lock here as a race could cause additional requests to - // the PAL, and this could lead to suprious OOM. This is - // particularly bad if the PAL gives all the memory on first call. - auto block_and_size = PAL::reserve_at_least(size * 2); - block = CapPtr(block_and_size.first); - block_size = block_and_size.second; - - // Ensure block is pointer aligned. - if ( - pointer_align_up(block, sizeof(void*)) != block || - bits::align_up(block_size, sizeof(void*)) > block_size) - { - auto diff = - pointer_diff(block, pointer_align_up(block, sizeof(void*))); - block_size = block_size - diff; - block_size = bits::align_down(block_size, sizeof(void*)); - } - } - if (block == nullptr) - { - return nullptr; - } - - core.template add_range(block, block_size); - - // still holding lock so guaranteed to succeed. - res = core.template reserve(size); + // It's a bit of a lie to convert without applying bounds, but the + // platform will have bounded block for us and it's better that + // the rest of our internals expect CBChunk bounds. + block = CapPtr(block_raw); } + else if constexpr (!pal_supports) + { + // Need at least 2 times the space to guarantee alignment. + // Hold lock here as a race could cause additional requests to + // the PAL, and this could lead to suprious OOM. This is + // particularly bad if the PAL gives all the memory on first call. + auto block_and_size = PAL::reserve_at_least(size * 2); + block = CapPtr(block_and_size.first); + block_size = block_and_size.second; + + // Ensure block is pointer aligned. + if ( + pointer_align_up(block, sizeof(void*)) != block || + bits::align_up(block_size, sizeof(void*)) > block_size) + { + auto diff = + pointer_diff(block, pointer_align_up(block, sizeof(void*))); + block_size = block_size - diff; + block_size = bits::align_down(block_size, sizeof(void*)); + } + } + if (block == nullptr) + { + return nullptr; + } + + pagemap.register_range(address_cast(block), block_size); + + core.template add_range(block, block_size); + + // still holding lock so guaranteed to succeed. + res = core.template reserve(size); } - - // Don't need lock while committing pages. - if constexpr (committed) - core.template commit_block(res, size); - - return res; } + + // Don't need lock while committing pages. + if constexpr (committed) + core.template commit_block(res, size); + + return res; } /** @@ -162,8 +142,8 @@ namespace snmalloc * This is useful for allowing the space required for alignment to be * used, by smaller objects. */ - template - CapPtr reserve_with_left_over(size_t size) + template + CapPtr reserve_with_left_over(size_t size, Pagemap& pagemap) { SNMALLOC_ASSERT(size >= sizeof(void*)); @@ -171,7 +151,7 @@ namespace snmalloc size_t rsize = bits::next_pow2(size); - auto res = reserve(rsize); + auto res = reserve(rsize, pagemap); if (res != nullptr) { diff --git a/src/backend/backend.h b/src/backend/backend.h index c5eb247..0d4618a 100644 --- a/src/backend/backend.h +++ b/src/backend/backend.h @@ -65,14 +65,13 @@ namespace snmalloc } template - std::enable_if_t - init(CapPtr base, size_t length) + std::enable_if_t init(void* base, size_t length) { static_assert( fixed_range_ == fixed_range, "Don't set SFINAE parameter!"); auto [heap_base, heap_length] = pagemap.init(base, length); - address_space.add_range(heap_base, heap_length); + address_space.add_range(CapPtr(heap_base), heap_length); if constexpr (!fixed_range) { @@ -106,7 +105,7 @@ namespace snmalloc auto& a = h.address_space; // TODO Improve heuristics and params auto refill_size = bits::max(size, bits::one_at_bit(21)); - auto refill = a.template reserve(refill_size); + auto refill = a.template reserve(refill_size, h.pagemap); if (refill == nullptr) return nullptr; local_state->local_address_space.template add_range( @@ -122,7 +121,7 @@ namespace snmalloc else { auto& a = h.address_space; - p = a.template reserve_with_left_over(size); + p = a.template reserve_with_left_over(size, h.pagemap); } return p; @@ -183,7 +182,7 @@ namespace snmalloc a < address_cast(pointer_offset(p, size)); a += MIN_CHUNK_SIZE) { - h.pagemap.add(a, t); + h.pagemap.set(a, t); } return {p, meta}; } diff --git a/src/backend/pagemap.h b/src/backend/pagemap.h index 06252d5..4b9d437 100644 --- a/src/backend/pagemap.h +++ b/src/backend/pagemap.h @@ -46,22 +46,23 @@ namespace snmalloc address_t base{0}; size_t size{0}; + public: /** - * Commit entry + * Ensure this range of pagemap is accessible */ - void commit_entry(void* p) + void register_range(address_t p, size_t length) { - auto entry_size = sizeof(T); - static_assert(sizeof(T) < OS_PAGE_SIZE); - // Rounding required for sub-page allocations. - auto page_start = pointer_align_down(p); - auto page_end = - pointer_align_up(pointer_offset(p, entry_size)); + // Calculate range in pagemap that is associated to this space. + auto first = &body[p >> SHIFT]; + auto last = &body[(p + length + bits::one_at_bit(SHIFT) - 1) >> SHIFT]; + + // Commit OS pages associated to the range. + auto page_start = pointer_align_down(first); + auto page_end = pointer_align_up(last); size_t using_size = pointer_diff(page_start, page_end); PAL::template notify_using(page_start, using_size); } - public: constexpr FlatPagemap() = default; /** @@ -70,14 +71,13 @@ namespace snmalloc * Returns usable range after pagemap has been allocated */ template - std::enable_if_t, size_t>> - init(CapPtr b, size_t s) + std::enable_if_t> + init(void* b, size_t s) { static_assert( has_bounds_ == has_bounds, "Don't set SFINAE template parameter!"); #ifdef SNMALLOC_TRACING - std::cout << "Pagemap.init " << b.unsafe_ptr() << " (" << s << ")" - << std::endl; + std::cout << "Pagemap.init " << b << " (" << s << ")" << std::endl; #endif SNMALLOC_ASSERT(s != 0); // TODO take account of pagemap size in the calculation of how big it @@ -92,7 +92,7 @@ namespace snmalloc // Put pagemap at start of range. // TODO CHERI capability bound here! - body = b.as_reinterpret().unsafe_ptr(); + body = reinterpret_cast(b); // Advance by size of pagemap. // TODO CHERI capability bound here! @@ -126,7 +126,8 @@ namespace snmalloc auto new_body = reinterpret_cast(new_body_untyped); // Ensure bottom page is committed - commit_entry(&new_body[0]); + // ASSUME: new memory is zeroed. + Pal::notify_using(new_body, OS_PAGE_SIZE); // Set up zero page new_body[0] = body[0]; @@ -166,7 +167,7 @@ namespace snmalloc // This means external pointer on Windows will be slow. if constexpr (potentially_out_of_range && !pal_supports) { - commit_entry(&body[p >> SHIFT]); + register_range(p, 1); } return body[p >> SHIFT]; @@ -188,27 +189,5 @@ namespace snmalloc body[p >> SHIFT] = t; } - - void add(address_t p, T t) - { -#ifdef SNMALLOC_TRACING - std::cout << "Pagemap.Add " << (void*)p << std::endl; -#endif - if constexpr (has_bounds) - { - if (p - base > size) - { - PAL::error("Internal error: Pagemap new write access out of range."); - } - p = p - base; - } - - // This could be the first time this page is used - // This will potentially be expensive on Windows, - // and we should revisit the performance here. - commit_entry(&body[p >> SHIFT]); - - body[p >> SHIFT] = t; - } }; } // namespace snmalloc diff --git a/src/mem/fixedglobalconfig.h b/src/mem/fixedglobalconfig.h index 4878453..01b268f 100644 --- a/src/mem/fixedglobalconfig.h +++ b/src/mem/fixedglobalconfig.h @@ -66,7 +66,7 @@ namespace snmalloc snmalloc::register_clean_up(); } - static void init(CapPtr base, size_t length) + static void init(void* base, size_t length) { get_backend_state().init(base, length); } diff --git a/src/test/func/fixed_region/fixed_region.cc b/src/test/func/fixed_region/fixed_region.cc index 702aaee..a7db57a 100644 --- a/src/test/func/fixed_region/fixed_region.cc +++ b/src/test/func/fixed_region/fixed_region.cc @@ -18,18 +18,14 @@ int main() { #ifndef SNMALLOC_PASS_THROUGH // Depends on snmalloc specific features - // Create a standard address space to get initial allocation - // this just bypasses having to understand the test platform. - AddressSpaceManager address_space; - // 28 is large enough to produce a nested allocator. // It is also large enough for the example to run in. // For 1MiB superslabs, SUPERSLAB_BITS + 4 is not big enough for the example. - size_t size = bits::one_at_bit(28); - auto oe_base = address_space.reserve(size); - auto oe_end = pointer_offset(oe_base, size).unsafe_ptr(); - std::cout << "Allocated region " << oe_base.unsafe_ptr() << " - " - << pointer_offset(oe_base, size).unsafe_ptr() << std::endl; + auto [oe_base, size] = Pal::reserve_at_least(bits::one_at_bit(28)); + Pal::notify_using(oe_base, size); + auto oe_end = pointer_offset(oe_base, size); + std::cout << "Allocated region " << oe_base << " - " + << pointer_offset(oe_base, size) << std::endl; CustomGlobals fixed_handle; CustomGlobals::init(oe_base, size); @@ -47,7 +43,7 @@ int main() if (r1 == nullptr) break; - if (oe_base.unsafe_ptr() > r1) + if (oe_base > r1) { std::cout << "Allocated: " << r1 << std::endl; abort(); diff --git a/src/test/func/pagemap/pagemap.cc b/src/test/func/pagemap/pagemap.cc index 7758b4f..dc5d5af 100644 --- a/src/test/func/pagemap/pagemap.cc +++ b/src/test/func/pagemap/pagemap.cc @@ -47,14 +47,6 @@ void check_get( } } -void add(bool bounded, address_t address, T new_value) -{ - if (bounded) - pagemap_test_bound.add(address, new_value); - else - pagemap_test_unbound.add(address, new_value); -} - void set(bool bounded, address_t address, T new_value) { if (bounded) @@ -71,38 +63,36 @@ void test_pagemap(bool bounded) address_t high = bits::one_at_bit(30); // Nullptr needs to work before initialisation - CHECK_GET(true, 0, T()); + CHECK_GET(bounded, 0, T()); // Initialise the pagemap if (bounded) { - auto size = bits::one_at_bit(30); - auto base = address_space.reserve(size); - std::cout << "Fixed base: " << base.unsafe_ptr() << " (" << size << ") " - << " end: " << pointer_offset(base, size).unsafe_ptr() - << std::endl; + auto [base, size] = Pal::reserve_at_least(bits::one_at_bit(30)); + Pal::notify_using(base, size); + std::cout << "Fixed base: " << base << " (" << size << ") " + << " end: " << pointer_offset(base, size) << std::endl; auto [heap_base, heap_size] = pagemap_test_bound.init(base, size); - std::cout << "Heap base: " << heap_base.unsafe_ptr() << " (" << heap_size - << ") " - << " end: " << pointer_offset(heap_base, heap_size).unsafe_ptr() - << std::endl; + std::cout << "Heap base: " << heap_base << " (" << heap_size << ") " + << " end: " << pointer_offset(heap_base, heap_size) << std::endl; low = address_cast(heap_base); high = low + heap_size; } else { pagemap_test_unbound.init(); + pagemap_test_unbound.register_range(low, high - low); } // Nullptr should still work after init. - CHECK_GET(true, 0, T()); + CHECK_GET(bounded, 0, T()); // Store a pattern into page map T value = 1; for (uintptr_t ptr = low; ptr < high; ptr += bits::one_at_bit(GRANULARITY_BITS + 3)) { - add(false, ptr, value); + set(bounded, ptr, value); value.v++; if (value.v == T().v) value = 0; @@ -116,7 +106,7 @@ void test_pagemap(bool bounded) for (uintptr_t ptr = low; ptr < high; ptr += bits::one_at_bit(GRANULARITY_BITS + 3)) { - CHECK_GET(false, ptr, value); + CHECK_GET(bounded, ptr, value); value.v++; if (value.v == T().v) value = 0; diff --git a/src/test/func/two_alloc_types/alloc1.cc b/src/test/func/two_alloc_types/alloc1.cc index 2c15db5..4e5f9d0 100644 --- a/src/test/func/two_alloc_types/alloc1.cc +++ b/src/test/func/two_alloc_types/alloc1.cc @@ -20,6 +20,5 @@ namespace snmalloc extern "C" void oe_allocator_init(void* base, void* end) { snmalloc::CustomGlobals fixed_handle; - fixed_handle.init( - CapPtr(base), address_cast(end) - address_cast(base)); + fixed_handle.init(base, address_cast(end) - address_cast(base)); }