[scudo] Avoid splitting aligned allocations on Trusty

[ahomescu]

Don't use multiple tagged pages at the beginning of an allocation, since
it prevents using such allocations for memrefs, and mappings aren't
reused anyway since Trusty uses MapAllocatorNoCache.
Upstreamed from https://r.android.com/2537251.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Andrei Homescu (ahomescu)

Changes

Don't use multiple tagged pages at the beginning of an allocation, since
it prevents using such allocations for memrefs, and mappings aren't
reused anyway since Trusty uses MapAllocatorNoCache.
Upstreamed from https://r.android.com/2537251.

---

Full diff: https://github.com/llvm/llvm-project/pull/69281.diff

1 Files Affected:

• (modified) compiler-rt/lib/scudo/standalone/secondary.h (+8-1)

diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index c89e6a95f5a68a4..937dbde4452f2e8 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -114,7 +114,14 @@ template <typename Config> class MapAllocatorNoCache {
   }
 };
 
-static const uptr MaxUnusedCachePages = 4U;
+/*
+ * On Trusty we want to avoid splitting page-aligned allocations into
+ * multiple mappings, because it prevents using the allocation for memref.
+ * Using only 1 tagged page at the start of the allocation prevents
+ * reusing the mapping for slightly smaller allocations, but since Trusty
+ * uses MapAllocatorNoCache, it wouldn't reuse anyway.
+ */
+static const uptr MaxUnusedCachePages = SCUDO_TRUSTY ? 1U : 4U;
 
 template <typename Config>
 bool mapSecondary(const Options &Options, uptr CommitBase, uptr CommitSize,

[ChiaHungDuan]

Please hold this for a while, I'm thinking if we should avoid using MaxUnusedCachePages for cache disabled case.

[ChiaHungDuan]

Given that Trusty doesn't enable the secondary cache, mapSecondary will only be used while mapping new pages in MapAllocator<Config>::allocate. It's unclear to me what goal of this change and what benefit we'll get on Trusty. Could you share more details behind this?

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ahomescu]

It's unclear to me what goal of this change and what benefit we'll get on Trusty. Could you share more details behind this?

@ChiaHungDuan I had to do a bit of digging to find the rationale behind the original change, but I managed to. See the comment in the latest version of the code for an alternative solution and an explanation.

I'm open to making the code less Trusty-specific if that's a concern.

[ChiaHungDuan]

It seems to me that this CL and https://r.android.com/2537251 are different. I'm not sure the exact case in trusty but if you are certain this approach is what we want, we may want to update the pull request description as well.

[ahomescu]

I tested this on a Pixel 6 with the following workflow:

• Boot phone with https://r.android.com/2537251 applied
• Revert that patch, confirm that phone hangs during boot
• Apply the new patch in this PR, confirm that phone boots again

[ChiaHungDuan]

Only two minor comments. Otherwise, LGTM

[ahomescu]

It's still possible if trusty requests more than 4 pages.

If you mean that Trusty can request more than 4 pages and we should still apply the original optimization there, I don't know exactly how that would work. On Trusty, we can't split the mapping past AllocPos.

Maybe we could make this configurable at either build or run time?

[ChiaHungDuan]

It's still possible if trusty requests more than 4 pages.

If you mean that Trusty can request more than 4 pages and we should still apply the original optimization there, I don't know exactly how that would work. On Trusty, we can't split the mapping past AllocPos.

Maybe we could make this configurable at either build or run time?

Sorry, I mean it's still different from trusty's case. We can do it like this now. Once we fix the logic in the other path, let's review trusty's case at that moment