[compiler-rt] [scudo] Update header without read-modify-write operation (PR #66955)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 20 15:02:51 PDT 2023
https://github.com/ChiaHungDuan created https://github.com/llvm/llvm-project/pull/66955
We used to update the deallocated block with
atomic_compare_exchange_strong to ensure the concurrent double-free will be detected. However, this operation incurs huge performance overhead which takes over 50% execution time in deallocate(). Given that we already have the checksum to guard the most double-free cases and other block verifications in the primary allocator, use atomic-store instead.
>From 43da9185d37f19d7304dbc85a131e43550a0feff Mon Sep 17 00:00:00 2001
From: Chia-hung Duan <chiahungduan at google.com>
Date: Wed, 20 Sep 2023 21:54:07 +0000
Subject: [PATCH] [scudo] Update header without read-modify-write operation
We used to update the deallocated block with
atomic_compare_exchange_strong to ensure the concurrent double-free will
be detected. However, this operation incurs huge performance overhead
which takes over 50% execution time in deallocate(). Given that we
already have the checksum to guard the most double-free cases and other
block verifications in the primary allocator, use atomic-store instead.
---
compiler-rt/lib/scudo/standalone/chunk.h | 13 ----
compiler-rt/lib/scudo/standalone/combined.h | 64 +++++++++----------
.../lib/scudo/standalone/tests/chunk_test.cpp | 23 -------
3 files changed, 30 insertions(+), 70 deletions(-)
diff --git a/compiler-rt/lib/scudo/standalone/chunk.h b/compiler-rt/lib/scudo/standalone/chunk.h
index 32874a8df6421c2..9228df047189091 100644
--- a/compiler-rt/lib/scudo/standalone/chunk.h
+++ b/compiler-rt/lib/scudo/standalone/chunk.h
@@ -128,19 +128,6 @@ inline void loadHeader(u32 Cookie, const void *Ptr,
reportHeaderCorruption(const_cast<void *>(Ptr));
}
-inline void compareExchangeHeader(u32 Cookie, void *Ptr,
- UnpackedHeader *NewUnpackedHeader,
- UnpackedHeader *OldUnpackedHeader) {
- NewUnpackedHeader->Checksum =
- computeHeaderChecksum(Cookie, Ptr, NewUnpackedHeader);
- PackedHeader NewPackedHeader = bit_cast<PackedHeader>(*NewUnpackedHeader);
- PackedHeader OldPackedHeader = bit_cast<PackedHeader>(*OldUnpackedHeader);
- if (UNLIKELY(!atomic_compare_exchange_strong(
- getAtomicHeader(Ptr), &OldPackedHeader, NewPackedHeader,
- memory_order_relaxed)))
- reportHeaderRace(Ptr);
-}
-
inline bool isValid(u32 Cookie, const void *Ptr,
UnpackedHeader *NewUnpackedHeader) {
PackedHeader NewPackedHeader = atomic_load_relaxed(getConstAtomicHeader(Ptr));
diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 29589cdd99fa78a..5bc7dd32890be11 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -67,14 +67,13 @@ class Allocator {
if (UNLIKELY(Header.State != Chunk::State::Quarantined))
reportInvalidChunkState(AllocatorAction::Recycling, Ptr);
- Chunk::UnpackedHeader NewHeader = Header;
- NewHeader.State = Chunk::State::Available;
- Chunk::compareExchangeHeader(Allocator.Cookie, Ptr, &NewHeader, &Header);
+ Header.State = Chunk::State::Available;
+ Chunk::storeHeader(Allocator.Cookie, Ptr, &Header);
if (allocatorSupportsMemoryTagging<Config>())
Ptr = untagPointer(Ptr);
- void *BlockBegin = Allocator::getBlockBegin(Ptr, &NewHeader);
- Cache.deallocate(NewHeader.ClassId, BlockBegin);
+ void *BlockBegin = Allocator::getBlockBegin(Ptr, &Header);
+ Cache.deallocate(Header.ClassId, BlockBegin);
}
// We take a shortcut when allocating a quarantine batch by working with the
@@ -117,9 +116,8 @@ class Allocator {
DCHECK_EQ(Header.Offset, 0);
DCHECK_EQ(Header.SizeOrUnusedBytes, sizeof(QuarantineBatch));
- Chunk::UnpackedHeader NewHeader = Header;
- NewHeader.State = Chunk::State::Available;
- Chunk::compareExchangeHeader(Allocator.Cookie, Ptr, &NewHeader, &Header);
+ Header.State = Chunk::State::Available;
+ Chunk::storeHeader(Allocator.Cookie, Ptr, &Header);
Cache.deallocate(QuarantineClassId,
reinterpret_cast<void *>(reinterpret_cast<uptr>(Ptr) -
Chunk::getHeaderSize()));
@@ -610,47 +608,46 @@ class Allocator {
if (UNLIKELY(!isAligned(reinterpret_cast<uptr>(OldPtr), MinAlignment)))
reportMisalignedPointer(AllocatorAction::Reallocating, OldPtr);
- Chunk::UnpackedHeader OldHeader;
- Chunk::loadHeader(Cookie, OldPtr, &OldHeader);
+ Chunk::UnpackedHeader Header;
+ Chunk::loadHeader(Cookie, OldPtr, &Header);
- if (UNLIKELY(OldHeader.State != Chunk::State::Allocated))
+ if (UNLIKELY(Header.State != Chunk::State::Allocated))
reportInvalidChunkState(AllocatorAction::Reallocating, OldPtr);
// Pointer has to be allocated with a malloc-type function. Some
// applications think that it is OK to realloc a memalign'ed pointer, which
// will trigger this check. It really isn't.
if (Options.get(OptionBit::DeallocTypeMismatch)) {
- if (UNLIKELY(OldHeader.OriginOrWasZeroed != Chunk::Origin::Malloc))
+ if (UNLIKELY(Header.OriginOrWasZeroed != Chunk::Origin::Malloc))
reportDeallocTypeMismatch(AllocatorAction::Reallocating, OldPtr,
- OldHeader.OriginOrWasZeroed,
+ Header.OriginOrWasZeroed,
Chunk::Origin::Malloc);
}
- void *BlockBegin = getBlockBegin(OldTaggedPtr, &OldHeader);
+ void *BlockBegin = getBlockBegin(OldTaggedPtr, &Header);
uptr BlockEnd;
uptr OldSize;
- const uptr ClassId = OldHeader.ClassId;
+ const uptr ClassId = Header.ClassId;
if (LIKELY(ClassId)) {
BlockEnd = reinterpret_cast<uptr>(BlockBegin) +
SizeClassMap::getSizeByClassId(ClassId);
- OldSize = OldHeader.SizeOrUnusedBytes;
+ OldSize = Header.SizeOrUnusedBytes;
} else {
BlockEnd = SecondaryT::getBlockEnd(BlockBegin);
OldSize = BlockEnd - (reinterpret_cast<uptr>(OldTaggedPtr) +
- OldHeader.SizeOrUnusedBytes);
+ Header.SizeOrUnusedBytes);
}
// If the new chunk still fits in the previously allocated block (with a
// reasonable delta), we just keep the old block, and update the chunk
// header to reflect the size change.
if (reinterpret_cast<uptr>(OldTaggedPtr) + NewSize <= BlockEnd) {
if (NewSize > OldSize || (OldSize - NewSize) < getPageSizeCached()) {
- Chunk::UnpackedHeader NewHeader = OldHeader;
- NewHeader.SizeOrUnusedBytes =
+ Header.SizeOrUnusedBytes =
(ClassId ? NewSize
: BlockEnd -
(reinterpret_cast<uptr>(OldTaggedPtr) + NewSize)) &
Chunk::SizeOrUnusedBytesMask;
- Chunk::compareExchangeHeader(Cookie, OldPtr, &NewHeader, &OldHeader);
+ Chunk::storeHeader(Cookie, OldPtr, &Header);
if (UNLIKELY(useMemoryTagging<Config>(Options))) {
if (ClassId) {
resizeTaggedChunk(reinterpret_cast<uptr>(OldTaggedPtr) + OldSize,
@@ -672,7 +669,7 @@ class Allocator {
void *NewPtr = allocate(NewSize, Chunk::Origin::Malloc, Alignment);
if (LIKELY(NewPtr)) {
memcpy(NewPtr, OldTaggedPtr, Min(NewSize, OldSize));
- quarantineOrDeallocateChunk(Options, OldTaggedPtr, &OldHeader, OldSize);
+ quarantineOrDeallocateChunk(Options, OldTaggedPtr, &Header, OldSize);
}
return NewPtr;
}
@@ -1110,31 +1107,30 @@ class Allocator {
Chunk::UnpackedHeader *Header,
uptr Size) NO_THREAD_SAFETY_ANALYSIS {
void *Ptr = getHeaderTaggedPointer(TaggedPtr);
- Chunk::UnpackedHeader NewHeader = *Header;
// If the quarantine is disabled, the actual size of a chunk is 0 or larger
// than the maximum allowed, we return a chunk directly to the backend.
// This purposefully underflows for Size == 0.
const bool BypassQuarantine = !Quarantine.getCacheSize() ||
((Size - 1) >= QuarantineMaxChunkSize) ||
- !NewHeader.ClassId;
+ !Header->ClassId;
if (BypassQuarantine)
- NewHeader.State = Chunk::State::Available;
+ Header->State = Chunk::State::Available;
else
- NewHeader.State = Chunk::State::Quarantined;
- NewHeader.OriginOrWasZeroed = useMemoryTagging<Config>(Options) &&
- NewHeader.ClassId &&
- !TSDRegistry.getDisableMemInit();
- Chunk::compareExchangeHeader(Cookie, Ptr, &NewHeader, Header);
+ Header->State = Chunk::State::Quarantined;
+ Header->OriginOrWasZeroed = useMemoryTagging<Config>(Options) &&
+ Header->ClassId &&
+ !TSDRegistry.getDisableMemInit();
+ Chunk::storeHeader(Cookie, Ptr, Header);
if (UNLIKELY(useMemoryTagging<Config>(Options))) {
u8 PrevTag = extractTag(reinterpret_cast<uptr>(TaggedPtr));
storeDeallocationStackMaybe(Options, Ptr, PrevTag, Size);
- if (NewHeader.ClassId) {
+ if (Header->ClassId) {
if (!TSDRegistry.getDisableMemInit()) {
uptr TaggedBegin, TaggedEnd;
const uptr OddEvenMask = computeOddEvenMaskForPointerMaybe(
- Options, reinterpret_cast<uptr>(getBlockBegin(Ptr, &NewHeader)),
- NewHeader.ClassId);
+ Options, reinterpret_cast<uptr>(getBlockBegin(Ptr, Header)),
+ Header->ClassId);
// Exclude the previous tag so that immediate use after free is
// detected 100% of the time.
setRandomTag(Ptr, Size, OddEvenMask | (1UL << PrevTag), &TaggedBegin,
@@ -1145,8 +1141,8 @@ class Allocator {
if (BypassQuarantine) {
if (allocatorSupportsMemoryTagging<Config>())
Ptr = untagPointer(Ptr);
- void *BlockBegin = getBlockBegin(Ptr, &NewHeader);
- const uptr ClassId = NewHeader.ClassId;
+ void *BlockBegin = getBlockBegin(Ptr, Header);
+ const uptr ClassId = Header->ClassId;
if (LIKELY(ClassId)) {
bool UnlockRequired;
auto *TSD = TSDRegistry.getTSDAndLock(&UnlockRequired);
diff --git a/compiler-rt/lib/scudo/standalone/tests/chunk_test.cpp b/compiler-rt/lib/scudo/standalone/tests/chunk_test.cpp
index 7a29f3c11b70ffb..1b2c1eb5a7d0c5c 100644
--- a/compiler-rt/lib/scudo/standalone/tests/chunk_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/chunk_test.cpp
@@ -37,29 +37,6 @@ TEST(ScudoChunkDeathTest, ChunkBasic) {
free(Block);
}
-TEST(ScudoChunkTest, ChunkCmpXchg) {
- initChecksum();
- const scudo::uptr Size = 0x100U;
- scudo::Chunk::UnpackedHeader OldHeader = {};
- OldHeader.OriginOrWasZeroed = scudo::Chunk::Origin::Malloc;
- OldHeader.ClassId = 0x42U;
- OldHeader.SizeOrUnusedBytes = Size;
- OldHeader.State = scudo::Chunk::State::Allocated;
- void *Block = malloc(HeaderSize + Size);
- void *P = reinterpret_cast<void *>(reinterpret_cast<scudo::uptr>(Block) +
- HeaderSize);
- scudo::Chunk::storeHeader(Cookie, P, &OldHeader);
- memset(P, 'A', Size);
- scudo::Chunk::UnpackedHeader NewHeader = OldHeader;
- NewHeader.State = scudo::Chunk::State::Quarantined;
- scudo::Chunk::compareExchangeHeader(Cookie, P, &NewHeader, &OldHeader);
- NewHeader = {};
- EXPECT_TRUE(scudo::Chunk::isValid(Cookie, P, &NewHeader));
- EXPECT_EQ(NewHeader.State, scudo::Chunk::State::Quarantined);
- EXPECT_FALSE(scudo::Chunk::isValid(InvalidCookie, P, &NewHeader));
- free(Block);
-}
-
TEST(ScudoChunkDeathTest, CorruptHeader) {
initChecksum();
const scudo::uptr Size = 0x100U;
More information about the llvm-commits
mailing list