Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[scudo] Fix the logic of MaxAllowedFragmentedPages #107927

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

ChiaHungDuan
Copy link
Contributor

MTE doesn't support MaxReleasedCachePages which may break the assumption that only the first 4 pages will have memory tagged.

MTE doesn't support MaxReleasedCachePages which may break the assumption
that only the first 4 pages will have memory tagged.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 9, 2024

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

Author: None (ChiaHungDuan)

Changes

MTE doesn't support MaxReleasedCachePages which may break the assumption that only the first 4 pages will have memory tagged.


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

1 Files Affected:

  • (modified) compiler-rt/lib/scudo/standalone/secondary.h (+1-1)
diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index 1a232b9b9fb2d7..7d85832d7fe714 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -725,7 +725,7 @@ MapAllocator<Config>::tryAllocateFromCache(const Options &Options, uptr Size,
   uptr EntryHeaderPos;
   uptr MaxAllowedFragmentedPages = MaxUnreleasedCachePages;
 
-  if (UNLIKELY(useMemoryTagging<Config>(Options)))
+  if (LIKELY(!useMemoryTagging<Config>(Options)))
     MaxAllowedFragmentedPages += CachedBlock::MaxReleasedCachePages;
 
   Entry = Cache.retrieve(MaxAllowedFragmentedPages, Size, Alignment,

@ChiaHungDuan
Copy link
Contributor Author

I'm running some tests to confirm the issue has been fixed.

Copy link
Contributor

@cferris1000 cferris1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@ChiaHungDuan
Copy link
Contributor Author

LGTM.

I updated the CL with some comments about how it causes problem for MTE

Copy link
Contributor

@cferris1000 cferris1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supremely minor nit.

// is dependent on the platform.
// We allow a certain amount of fragmentation and part of the fragmented bytes
// will be released by `releaseAndZeroPagesToOS()`. This increases the chance
// of cache hit rate and reduce the overhead to the RSS at the same time. See
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reduces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@cferris1000 cferris1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ChiaHungDuan ChiaHungDuan merged commit 6e854a6 into llvm:main Sep 11, 2024
5 of 6 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Sep 11, 2024

LLVM Buildbot has detected a new failure on builder sanitizer-x86_64-linux-qemu running on sanitizer-buildbot3 while building compiler-rt at step 2 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/139/builds/3565

Here is the relevant piece of the build log for the reference
Step 2 (annotate) failure: 'python ../sanitizer_buildbot/sanitizers/zorg/buildbot/builders/sanitizers/buildbot_selector.py' (failure)
...
[71/77] Generating ScudoUnitTestsObjects.primary_test.cpp.aarch64.o
[72/77] Generating ScudoUnitTestsObjects.gtest-all.cc.aarch64.o
[73/77] Generating ScudoCxxUnitTest-aarch64-Test
[74/77] Generating ScudoCUnitTest-aarch64-Test
[75/77] Generating ScudoUnitTestsObjects.combined_test.cpp.aarch64.o
[76/77] Generating ScudoUnitTest-aarch64-Test
[76/77] Running Scudo Standalone tests
llvm-lit: /home/b/sanitizer-x86_64-linux-qemu/build/llvm-project/llvm/utils/lit/lit/main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 900 seconds was requested on the command line. Forcing timeout to be 900 seconds.
-- Testing: 314 tests, 88 workers --
Testing: 
FAIL: ScudoStandalone-Unit :: ./ScudoCxxUnitTest-aarch64-Test/2/3 (1 of 314)
******************** TEST 'ScudoStandalone-Unit :: ./ScudoCxxUnitTest-aarch64-Test/2/3' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoCxxUnitTest-aarch64-Test-ScudoStandalone-Unit-2791140-2-3.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=3 GTEST_SHARD_INDEX=2 /home/b/sanitizer-x86_64-linux-qemu/build/qemu_build/qemu-aarch64 -L /usr/aarch64-linux-gnu /home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoCxxUnitTest-aarch64-Test
--

Note: This is test shard 3 of 3.
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from ScudoWrappersCppTest
[ RUN      ] ScudoWrappersCppTest.AllocAfterFork
Scudo ERROR: CHECK failed @ /home/b/sanitizer-x86_64-linux-qemu/build/llvm-project/compiler-rt/lib/scudo/standalone/secondary.h:737 (CachedBlock::MaxReleasedCachePages) == (0U) ((u64)op1=4, (u64)op2=0)
qemu: uncaught target signal 6 (Aborted) - core dumped

--
exit: -6
--
shard JSON output does not exist: /home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoCxxUnitTest-aarch64-Test-ScudoStandalone-Unit-2791140-2-3.json
********************
Testing:  0
FAIL: ScudoStandalone-Unit :: ./ScudoUnitTest-aarch64-Test/104/139 (15 of 314)
******************** TEST 'ScudoStandalone-Unit :: ./ScudoUnitTest-aarch64-Test/104/139' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoUnitTest-aarch64-Test-ScudoStandalone-Unit-2791140-104-139.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=139 GTEST_SHARD_INDEX=104 /home/b/sanitizer-x86_64-linux-qemu/build/qemu_build/qemu-aarch64 -L /usr/aarch64-linux-gnu /home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoUnitTest-aarch64-Test
--

Note: This is test shard 105 of 139.
[==========] Running 2 tests from 2 test suites.
[----------] Global test environment set-up.
[----------] 1 test from ScudoCombinedTestReallocateLargeDecreasing_DefaultConfig
[ RUN      ] ScudoCombinedTestReallocateLargeDecreasing_DefaultConfig.ReallocateLargeDecreasing
Scudo ERROR: CHECK failed @ /home/b/sanitizer-x86_64-linux-qemu/build/llvm-project/compiler-rt/lib/scudo/standalone/secondary.h:737 (CachedBlock::MaxReleasedCachePages) == (0U) ((u64)op1=4, (u64)op2=0)
qemu: uncaught target signal 6 (Aborted) - core dumped

--
exit: -6
--
Step 22 (scudo debug_aarch64_qemu) failure: scudo debug_aarch64_qemu (failure)
...
[71/77] Generating ScudoUnitTestsObjects.primary_test.cpp.aarch64.o
[72/77] Generating ScudoUnitTestsObjects.gtest-all.cc.aarch64.o
[73/77] Generating ScudoCxxUnitTest-aarch64-Test
[74/77] Generating ScudoCUnitTest-aarch64-Test
[75/77] Generating ScudoUnitTestsObjects.combined_test.cpp.aarch64.o
[76/77] Generating ScudoUnitTest-aarch64-Test
[76/77] Running Scudo Standalone tests
llvm-lit: /home/b/sanitizer-x86_64-linux-qemu/build/llvm-project/llvm/utils/lit/lit/main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 900 seconds was requested on the command line. Forcing timeout to be 900 seconds.
-- Testing: 314 tests, 88 workers --
Testing: 
FAIL: ScudoStandalone-Unit :: ./ScudoCxxUnitTest-aarch64-Test/2/3 (1 of 314)
******************** TEST 'ScudoStandalone-Unit :: ./ScudoCxxUnitTest-aarch64-Test/2/3' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoCxxUnitTest-aarch64-Test-ScudoStandalone-Unit-2791140-2-3.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=3 GTEST_SHARD_INDEX=2 /home/b/sanitizer-x86_64-linux-qemu/build/qemu_build/qemu-aarch64 -L /usr/aarch64-linux-gnu /home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoCxxUnitTest-aarch64-Test
--

Note: This is test shard 3 of 3.
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from ScudoWrappersCppTest
[ RUN      ] ScudoWrappersCppTest.AllocAfterFork
Scudo ERROR: CHECK failed @ /home/b/sanitizer-x86_64-linux-qemu/build/llvm-project/compiler-rt/lib/scudo/standalone/secondary.h:737 (CachedBlock::MaxReleasedCachePages) == (0U) ((u64)op1=4, (u64)op2=0)
qemu: uncaught target signal 6 (Aborted) - core dumped

--
exit: -6
--
shard JSON output does not exist: /home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoCxxUnitTest-aarch64-Test-ScudoStandalone-Unit-2791140-2-3.json
********************
Testing:  0
FAIL: ScudoStandalone-Unit :: ./ScudoUnitTest-aarch64-Test/104/139 (15 of 314)
******************** TEST 'ScudoStandalone-Unit :: ./ScudoUnitTest-aarch64-Test/104/139' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoUnitTest-aarch64-Test-ScudoStandalone-Unit-2791140-104-139.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=139 GTEST_SHARD_INDEX=104 /home/b/sanitizer-x86_64-linux-qemu/build/qemu_build/qemu-aarch64 -L /usr/aarch64-linux-gnu /home/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_aarch64_qemu/lib/scudo/standalone/tests/./ScudoUnitTest-aarch64-Test
--

Note: This is test shard 105 of 139.
[==========] Running 2 tests from 2 test suites.
[----------] Global test environment set-up.
[----------] 1 test from ScudoCombinedTestReallocateLargeDecreasing_DefaultConfig
[ RUN      ] ScudoCombinedTestReallocateLargeDecreasing_DefaultConfig.ReallocateLargeDecreasing
Scudo ERROR: CHECK failed @ /home/b/sanitizer-x86_64-linux-qemu/build/llvm-project/compiler-rt/lib/scudo/standalone/secondary.h:737 (CachedBlock::MaxReleasedCachePages) == (0U) ((u64)op1=4, (u64)op2=0)
qemu: uncaught target signal 6 (Aborted) - core dumped

--
exit: -6
--

ChiaHungDuan added a commit that referenced this pull request Sep 11, 2024
ChiaHungDuan added a commit that referenced this pull request Sep 11, 2024
Reverts #107927

We are supposed to check the MaxAllowedFragmentedPages instead.
jrguzman-ms pushed a commit to msft-mirror-aosp/platform.external.scudo that referenced this pull request Sep 12, 2024
Reverts llvm/llvm-project#107927

We are supposed to check the MaxAllowedFragmentedPages instead.

GitOrigin-RevId: 76151c449080b7239c8b442291514a4300d51cba
Change-Id: I16cbbbb22576eb47bf2b7aa30156ac17425308da
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Sep 12, 2024
MTE doesn't support MaxReleasedCachePages which may break the assumption
that only the first 4 pages will have memory tagged.
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Sep 12, 2024
)

Reverts llvm#107927

We are supposed to check the MaxAllowedFragmentedPages instead.
bherrera pushed a commit to misttech/mist-os that referenced this pull request Sep 18, 2024
…FragmentedPages" (#108130)

Reverts llvm/llvm-project#107927

We are supposed to check the MaxAllowedFragmentedPages instead.

GitOrigin-RevId: 8c10827543c8e11689cef95c6e3d3378482887c2
Original-Revision: c682eaae1d2ca9d8e00604ee2712da6d01ff821d
Roller-URL: https://ci.chromium.org/b/8737117427431755313
CQ-Do-Not-Cancel-Tryjobs: true
Change-Id: I4bcb52dffad477f6cdc1415fa658da6646b673be
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1116349
bherrera pushed a commit to misttech/integration that referenced this pull request Sep 18, 2024
…edPages" (#108130)

Reverts llvm/llvm-project#107927

We are supposed to check the MaxAllowedFragmentedPages instead.

GitOrigin-RevId: 8c10827543c8e11689cef95c6e3d3378482887c2
Original-Revision: c682eaae1d2ca9d8e00604ee2712da6d01ff821d
Change-Id: I3edf550c4622ad9aabd159ab9e032d12f3052bf4
bherrera pushed a commit to misttech/integration that referenced this pull request Sep 18, 2024
… logic of MaxAllowedFragmentedPages" (#108130)

Reverts llvm/llvm-project#107927

We are supposed to check the MaxAllowedFragmentedPages instead.

GitOrigin-RevId: 4f5f037eb0225da54cb4de75e1382a653e9d1c54
Original-Revision: c682eaae1d2ca9d8e00604ee2712da6d01ff821d
Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1116349
Original-Revision: b1e8b615193f7d7a91e62963dc7a31128a10b103
Change-Id: Iffd2ddb6324b23914467ab1e2564b3e8ffd1effe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants