[BRP] Finchify "same slot" mode

In the current implementation, the only difference between "same slot" and "previous slot" modes is end of which slot we look for ref-count. This means that when MTE is on, we still bloat up ref_count_size to the granule size and skip tagging it, even though it is no longer necessary in the "same slot" mode. In the final code, we will no longer do that, but we have to make sure that calculating the ref-count address doesn't lose the tag. a finch flag, so the coverage should be the same) Low-Coverage-Reason: OTHER (this merely converts a buildflag into Bug: 1511221, 1445816 Change-Id: I92efd626c0f57f9ae34166e229433a476cbe2d0d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5180030 Reviewed-by: Keishi Hattori <[email protected]> Commit-Queue: Bartek Nowierski <[email protected]> Cr-Commit-Position: refs/heads/main@{#1247929}
azx1573 · Jan 17, 2024 · 89ad25d · 89ad25d
1 parent 5b6b98b
commit 89ad25d
Show file tree

Hide file tree

Showing 17 changed files with 102 additions and 82 deletions.
diff --git a/base/allocator/partition_alloc_features.cc b/base/allocator/partition_alloc_features.cc
@@ -170,6 +170,7 @@ constexpr FeatureParam<BackupRefPtrMode>::Option kBackupRefPtrModeOptions[] = {
     {BackupRefPtrMode::kDisabled, "disabled"},
     {BackupRefPtrMode::kEnabled, "enabled"},
     {BackupRefPtrMode::kEnabled, "enabled-with-memory-reclaimer"},
+    {BackupRefPtrMode::kEnabledInSameSlotMode, "enabled-in-same-slot-mode"},
     {BackupRefPtrMode::kDisabled, "disabled-but-2-way-split"},
     {BackupRefPtrMode::kDisabled,
      "disabled-but-2-way-split-with-memory-reclaimer"},

diff --git a/base/allocator/partition_alloc_features.h b/base/allocator/partition_alloc_features.h
@@ -100,8 +100,12 @@ enum class BackupRefPtrMode {
 
   // BRP is enabled in the main partition, as well as certain Renderer-only
   // partitions (if enabled in Renderer at all).
-  // This entails splitting the main partition.
   kEnabled,
+
+  // As above, but "same slot" mode is used, as opposed to "previous slot".
+  // This means that ref-count is placed at the end of the same slot as the
+  // object it protects, as opposed to the end of the previous slot.
+  kEnabledInSameSlotMode,
 };
 
 enum class MemtagMode {

diff --git a/base/allocator/partition_alloc_support.cc b/base/allocator/partition_alloc_support.cc
@@ -871,6 +871,7 @@ PartitionAllocSupport::GetBrpConfiguration(const std::string& process_type) {
   CHECK(base::FeatureList::GetInstance());
 
   bool enable_brp = false;
+  bool ref_count_in_same_slot = false;
   bool process_affected_by_brp_flag = false;
 
 #if (BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) &&  \
@@ -913,6 +914,9 @@ PartitionAllocSupport::GetBrpConfiguration(const std::string& process_type) {
         // Do nothing. Equivalent to !IsEnabled(kPartitionAllocBackupRefPtr).
         break;
 
+      case base::features::BackupRefPtrMode::kEnabledInSameSlotMode:
+        ref_count_in_same_slot = true;
+        ABSL_FALLTHROUGH_INTENDED;
       case base::features::BackupRefPtrMode::kEnabled:
         enable_brp = true;
         break;
@@ -923,6 +927,7 @@ PartitionAllocSupport::GetBrpConfiguration(const std::string& process_type) {
 
   return {
       enable_brp,
+      ref_count_in_same_slot,
       process_affected_by_brp_flag,
   };
 }
@@ -1132,6 +1137,10 @@ void PartitionAllocSupport::ReconfigureAfterFeatureListInit(
            partition_alloc::TagViolationReportingMode::kDisabled));
   }
 
+  // Set ref-count mode before we create any roots that have BRP enabled.
+  partition_alloc::PartitionRoot::SetBrpRefCountInSameSlot(
+      brp_config.ref_count_in_same_slot);
+
   allocator_shim::ConfigurePartitions(
       allocator_shim::EnableBrp(brp_config.enable_brp),
       allocator_shim::EnableMemoryTagging(enable_memory_tagging),

diff --git a/base/allocator/partition_alloc_support.h b/base/allocator/partition_alloc_support.h
@@ -45,6 +45,7 @@ class BASE_EXPORT PartitionAllocSupport {
  public:
   struct BrpConfiguration {
     bool enable_brp = false;
+    bool ref_count_in_same_slot = false;
     bool process_affected_by_brp_flag = false;
   };
 

diff --git a/base/allocator/partition_allocator/build_overrides/partition_alloc.gni b/base/allocator/partition_allocator/build_overrides/partition_alloc.gni
@@ -12,7 +12,6 @@ import("//build_overrides/build.gni")
 use_partition_alloc_as_malloc_default = false
 use_allocator_shim_default = false
 enable_backup_ref_ptr_support_default = false
-put_ref_count_in_previous_slot_default = true
 enable_backup_ref_ptr_slow_checks_default = false
 enable_dangling_raw_ptr_checks_default = false
 

diff --git a/base/allocator/partition_allocator/partition_alloc.gni b/base/allocator/partition_allocator/partition_alloc.gni
@@ -172,9 +172,6 @@ declare_args() {
 use_hookable_raw_ptr = use_asan_backup_ref_ptr
 
 declare_args() {
-  # - put_ref_count_in_previous_slot: place the ref-count at the end of the
-  #   previous slot (or in metadata if a slot starts on the page boundary), as
-  #   opposed to end of the same slot.
   # - enable_backup_ref_ptr_slow_checks: enable additional safety checks that
   #   are too expensive to have on by default.
   # - enable_dangling_raw_ptr_checks: enable checking raw_ptr do not become
@@ -184,8 +181,6 @@ declare_args() {
   # - enable_backup_ref_ptr_instance_tracer: use a global table to track all
   #   live raw_ptr/raw_ref instances to help debug dangling pointers at test
   #   end.
-  put_ref_count_in_previous_slot =
-      put_ref_count_in_previous_slot_default && enable_backup_ref_ptr_support
 
   enable_backup_ref_ptr_slow_checks =
       enable_backup_ref_ptr_slow_checks_default && enable_backup_ref_ptr_support
@@ -255,7 +250,6 @@ if (!use_partition_alloc) {
   use_asan_backup_ref_ptr = false
   use_asan_unowned_ptr = false
   use_hookable_raw_ptr = false
-  put_ref_count_in_previous_slot = false
   enable_backup_ref_ptr_slow_checks = false
   enable_dangling_raw_ptr_checks = false
   enable_dangling_raw_ptr_perf_experiment = false
@@ -265,13 +259,6 @@ if (!use_partition_alloc) {
   use_starscan = false
 }
 
-# put_ref_count_in_previous_slot can only be used if
-# enable_backup_ref_ptr_support is true.
-assert(
-    enable_backup_ref_ptr_support || !put_ref_count_in_previous_slot,
-    "Can't put ref count in the previous slot if BackupRefPtr isn't enabled " +
-        "at all")
-
 # enable_backup_ref_ptr_slow_checks can only be used if
 # enable_backup_ref_ptr_support is true.
 assert(enable_backup_ref_ptr_support || !enable_backup_ref_ptr_slow_checks,

diff --git a/base/allocator/partition_allocator/src/partition_alloc/BUILD.gn b/base/allocator/partition_allocator/src/partition_alloc/BUILD.gn
@@ -192,7 +192,6 @@ buildflag_header("partition_alloc_buildflags") {
     "ENABLE_POINTER_SUBTRACTION_CHECK=$enable_pointer_subtraction_check",
     "ENABLE_POINTER_ARITHMETIC_TRAIT_CHECK=$enable_pointer_arithmetic_trait_check",
     "BACKUP_REF_PTR_POISON_OOB_PTR=$backup_ref_ptr_poison_oob_ptr",
-    "PUT_REF_COUNT_IN_PREVIOUS_SLOT=$put_ref_count_in_previous_slot",
     "USE_ASAN_BACKUP_REF_PTR=$use_asan_backup_ref_ptr",
     "USE_ASAN_UNOWNED_PTR=$use_asan_unowned_ptr",
     "USE_HOOKABLE_RAW_PTR=$use_hookable_raw_ptr",

diff --git a/base/allocator/partition_allocator/src/partition_alloc/gwp_asan_support.cc b/base/allocator/partition_allocator/src/partition_alloc/gwp_asan_support.cc
@@ -96,7 +96,8 @@ void* GwpAsanSupport::MapRegion(size_t slot_count,
 
         for (uintptr_t slot_idx = 0; slot_idx < kSlotsPerSlotSpan; ++slot_idx) {
           auto slot_start = slot_span_start + slot_idx * kSlotSize;
-          internal::PartitionRefCountPointer(slot_start, kSlotSize)
+          PartitionRoot::RefCountPointerFromSlotStartAndSize(slot_start,
+                                                             kSlotSize)
               ->InitalizeForGwpAsan();
           size_t global_slot_idx = (slot_start - super_page_span_start -
                                     kSuperPageGwpAsanSlotAreaBeginOffset) /
@@ -121,7 +122,8 @@ void* GwpAsanSupport::MapRegion(size_t slot_count,
 // static
 bool GwpAsanSupport::CanReuse(uintptr_t slot_start) {
   const size_t kSlotSize = 2 * internal::SystemPageSize();
-  return internal::PartitionRefCountPointer(slot_start, kSlotSize)
+  return PartitionRoot::RefCountPointerFromSlotStartAndSize(slot_start,
+                                                            kSlotSize)
       ->CanBeReusedByGwpAsan();
 }
 

diff --git a/base/allocator/partition_allocator/src/partition_alloc/partition_alloc_config.h b/base/allocator/partition_allocator/src/partition_alloc/partition_alloc_config.h
@@ -171,17 +171,14 @@ static_assert(sizeof(void*) == 8);
 // ref_count_size is increased to the MTE granule size and is excluded from MTE
 // tagging.
 //
-// Note, this takes effect only when IsMemoryTaggingEnabled() is true, which is
-// for very few devices (10s or 100s), so this won't bias any BRP experiment.
-#if BUILDFLAG(HAS_MEMORY_TAGGING) &&            \
-    BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT) && \
-    BUILDFLAG(PUT_REF_COUNT_IN_PREVIOUS_SLOT)
-#define PA_CONFIG_INCREASE_REF_COUNT_SIZE_FOR_MTE() 1
+// The settings has MAYBE_ in the name, because the final decision to enable is
+// based on whether both MTE and BRP are enabled, and also on BRP mode.
+#if BUILDFLAG(HAS_MEMORY_TAGGING) && BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT)
+#define PA_CONFIG_MAYBE_INCREASE_REF_COUNT_SIZE_FOR_MTE() 1
 #else
-#define PA_CONFIG_INCREASE_REF_COUNT_SIZE_FOR_MTE() 0
+#define PA_CONFIG_MAYBE_INCREASE_REF_COUNT_SIZE_FOR_MTE() 0
 #endif  // BUILDFLAG(HAS_MEMORY_TAGGING) &&
-        // BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT) &&
-        // BUILDFLAG(PUT_REF_COUNT_IN_PREVIOUS_SLOT)
+        // BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT)
 
 // Specifies whether allocation extras need to be added.
 #if BUILDFLAG(PA_DCHECK_IS_ON) || BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT)
@@ -330,8 +327,8 @@ constexpr bool kUseLazyCommit = false;
 // The bug has been fixed in macOS 12. Here we can only check the platform, and
 // the version is checked dynamically later.
 //
-// The settings has MAYBE in the name, because the final decision to enable is
-// based on the operarting version check done at run-time.
+// The settings has MAYBE_ in the name, because the final decision to enable is
+// based on the operarting system version check done at run-time.
 #if BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT) && BUILDFLAG(IS_MAC)
 #define PA_CONFIG_MAYBE_ENABLE_MAC11_MALLOC_SIZE_HACK() 1
 #else

diff --git a/base/allocator/partition_allocator/src/partition_alloc/partition_alloc_unittest.cc b/base/allocator/partition_allocator/src/partition_alloc/partition_alloc_unittest.cc
@@ -447,12 +447,14 @@ class PartitionAllocTest
     if (allocator.root()->brp_enabled()) {
       ref_count_size = kPartitionRefCountSizeAdjustment;
       ref_count_size = AlignUpRefCountSizeForMac(ref_count_size);
-#if PA_CONFIG(INCREASE_REF_COUNT_SIZE_FOR_MTE)
+#if PA_CONFIG(MAYBE_INCREASE_REF_COUNT_SIZE_FOR_MTE)
+      // Note the brp_enabled() check above.
+      // TODO(bartekn): Don't increase ref-count size in the "same slot" mode.
       if (allocator.root()->IsMemoryTaggingEnabled()) {
         ref_count_size = partition_alloc::internal::base::bits::AlignUp(
             ref_count_size, kMemTagGranuleSize);
       }
-#endif  // PA_CONFIG(INCREASE_REF_COUNT_SIZE_FOR_MTE)
+#endif  // PA_CONFIG(MAYBE_INCREASE_REF_COUNT_SIZE_FOR_MTE)
     }
     return kExtraAllocSizeWithoutRefCount + ref_count_size;
   }
@@ -2519,7 +2521,7 @@ TEST_P(PartitionAllocDeathTest, LargeAllocs) {
 // thinking the memory isn't freed and still referenced, thus making it
 // quarantine it and return early, before PA_CHECK(slot_start != freelist_head)
 // is reached.
-// TODO(bartekn): Enable in the BUILDFLAG(PUT_REF_COUNT_IN_PREVIOUS_SLOT) case.
+// TODO(bartekn): Enable in the "previous slot" mode.
 #if !BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT) || \
     (BUILDFLAG(HAS_64_BIT_POINTERS) && defined(ARCH_CPU_LITTLE_ENDIAN))
 

diff --git a/base/allocator/partition_allocator/src/partition_alloc/partition_ref_count.h b/base/allocator/partition_allocator/src/partition_alloc/partition_ref_count.h
@@ -438,7 +438,8 @@ GetPartitionRefCountIndexMultiplierShift() {
 
 PA_ALWAYS_INLINE PartitionRefCount* PartitionRefCountPointer(
     uintptr_t slot_start,
-    size_t slot_size) {
+    size_t slot_size,
+    bool ref_count_in_same_slot) {
   // In the "previous slot" mode, ref-counts that would be on a different page
   // than their corresponding slot are instead placed in the super page metadata
   // area. This is done so that they don't interfere with discarding of data
@@ -455,12 +456,9 @@ PA_ALWAYS_INLINE PartitionRefCount* PartitionRefCountPointer(
   // All of the above happen have `slot_start` at the page boundary, so we can
   // reuse the "previous slot" mode code.
   if (PA_LIKELY(slot_start & SystemPageOffsetMask())) {
-#if BUILDFLAG(PUT_REF_COUNT_IN_PREVIOUS_SLOT)
-    uintptr_t refcount_address = slot_start - sizeof(PartitionRefCount);
-#else
-    uintptr_t refcount_address =
-        slot_start + slot_size - sizeof(PartitionRefCount);
-#endif  // BUILDFLAG(PUT_REF_COUNT_IN_PREVIOUS_SLOT)
+    uintptr_t refcount_address = slot_start +
+                                 (ref_count_in_same_slot ? slot_size : 0) -
+                                 sizeof(PartitionRefCount);
 #if BUILDFLAG(PA_DCHECK_IS_ON) || BUILDFLAG(ENABLE_BACKUP_REF_PTR_SLOW_CHECKS)
     PA_CHECK(refcount_address % alignof(PartitionRefCount) == 0);
 #endif

diff --git a/base/allocator/partition_allocator/src/partition_alloc/partition_root.cc b/base/allocator/partition_allocator/src/partition_alloc/partition_root.cc
@@ -1036,12 +1036,14 @@ void PartitionRoot::Init(PartitionOptions opts) {
     if (brp_enabled()) {
       size_t ref_count_size = internal::kPartitionRefCountSizeAdjustment;
       ref_count_size = internal::AlignUpRefCountSizeForMac(ref_count_size);
-#if PA_CONFIG(INCREASE_REF_COUNT_SIZE_FOR_MTE)
+#if PA_CONFIG(MAYBE_INCREASE_REF_COUNT_SIZE_FOR_MTE)
+      // Note the brp_enabled() check above.
+      // TODO(bartekn): Don't increase ref-count size in the "same slot" mode.
       if (IsMemoryTaggingEnabled()) {
         ref_count_size = internal::base::bits::AlignUp(
             ref_count_size, internal::kMemTagGranuleSize);
       }
-#endif  // PA_CONFIG(INCREASE_REF_COUNT_SIZE_FOR_MTE)
+#endif  // PA_CONFIG(MAYBE_INCREASE_REF_COUNT_SIZE_FOR_MTE)
       settings.ref_count_size = ref_count_size;
       PA_CHECK(internal::kPartitionRefCountSizeAdjustment <= ref_count_size);
       settings.extras_size += ref_count_size;
@@ -1308,7 +1310,7 @@ bool PartitionRoot::TryReallocInPlaceForNormalBuckets(
 #if BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT) && BUILDFLAG(PA_DCHECK_IS_ON)
     internal::PartitionRefCount* old_ref_count = nullptr;
     if (brp_enabled()) {
-      old_ref_count = internal::PartitionRefCountPointer(
+      old_ref_count = RefCountPointerFromSlotStartAndSize(
           slot_start, slot_span->bucket->slot_size);
     }
 #endif  // BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT) &&
@@ -1318,8 +1320,8 @@ bool PartitionRoot::TryReallocInPlaceForNormalBuckets(
 #if BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT) && BUILDFLAG(PA_DCHECK_IS_ON)
     if (brp_enabled()) {
       internal::PartitionRefCount* new_ref_count =
-          internal::PartitionRefCountPointer(slot_start,
-                                             slot_span->bucket->slot_size);
+          RefCountPointerFromSlotStartAndSize(slot_start,
+                                              slot_span->bucket->slot_size);
       PA_DCHECK(new_ref_count == old_ref_count);
     }
 #endif  // BUILDFLAG(ENABLE_BACKUP_REF_PTR_SUPPORT) &&