This is an archive of the discontinued LLVM Phabricator instance.

Differential D87420

scudo: Introduce a new mechanism to let Scudo access a platform-specific TLS slot
ClosedPublic

Authored by pcc on Sep 9 2020, 3:33 PM.

Details

Reviewers
cryptoad
cferris
Commits
rG84c2c4977dfe: scudo: Introduce a new mechanism to let Scudo access a platform-specific TLS…
Summary

An upcoming change to Scudo will change how we use the TLS slot
in tsd_shared.h, which will be a little easier to deal with if
we can remove the code path that calls pthread_getspecific and
pthread_setspecific. The only known user of this code path is Fuchsia.

We can't eliminate this code path by making Fuchsia use ELF TLS
because although Fuchsia supports ELF TLS, it is not supported within
libc itself. To address this, Roland McGrath on the Fuchsia team has
proposed that Scudo will optionally call a platform-provided function
to access a TLS slot reserved for Scudo. Android also has a reserved
TLS slot, but the code that accesses the TLS slot lives in Scudo.

We can eliminate some complexity and duplicated code by having Android
use the same mechanism that was proposed for Fuchsia, which is what
this change does. A separate change to Android implements it.

Diff Detail

Event Timeline

pcc created this revision.Sep 9 2020, 3:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 3:33 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
pcc requested review of this revision.Sep 9 2020, 3:33 PM
Harbormaster completed remote builds in B71154: Diff 290835.Sep 9 2020, 4:22 PM
mcgrathr added a comment.Sep 9 2020, 6:59 PM

Looks fine to me. Thanks!

compiler-rt/lib/scudo/standalone/tsd_shared.h
91–92

You could just make it static thread_local in this function and not need any other conditionalization declarations below.

196–197

Should these conditionals be #if !SCUDO_HAS_PLATFORM_TLS_SLOT now since the code above won't compile if that's not defined and this isn't declared?

eugenis added a subscriber: eugenis.Sep 10 2020, 12:14 PM

Where is the implementation of getPlatformTlsSlot?

pcc added a comment.Sep 10 2020, 12:25 PM
In D87420#2266456, @eugenis wrote:

Where is the implementation of getPlatformTlsSlot?

Here: https://android-review.googlesource.com/c/platform/bionic/+/1423770

pcc updated this revision to Diff 291064.Sep 10 2020, 12:31 PM
  • Address review comments
pcc marked an inline comment as done.Sep 10 2020, 12:32 PM
pcc added inline comments.
compiler-rt/lib/scudo/standalone/tsd_shared.h
91–92

Good idea, done.

196–197

Now moot

pcc added a child revision: D87478: scudo: Remove the THREADLOCAL macro..Sep 10 2020, 12:39 PM
Harbormaster completed remote builds in B71282: Diff 291064.Sep 10 2020, 1:37 PM
pcc updated this revision to Diff 291077.Sep 10 2020, 1:54 PM
pcc marked an inline comment as done.
  • Renamed the function per feedback on the Android review
Harbormaster completed remote builds in B71287: Diff 291077.Sep 10 2020, 2:28 PM
cferris accepted this revision.Sep 10 2020, 4:52 PM

This seems fine to me, but maybe someone from llvm should also approve.

This revision is now accepted and ready to land.Sep 10 2020, 4:52 PM
cryptoad added inline comments.Sep 10 2020, 5:30 PM
compiler-rt/lib/scudo/standalone/tsd_shared.h
12

Is this still necessary?

16

Is there some requirement as to where this should live, or build time include path requirement that should be documented?

pcc updated this revision to Diff 291109.Sep 10 2020, 6:49 PM
  • Address review comments
compiler-rt/lib/scudo/standalone/tsd_shared.h
12

No it isn't, removed.

16

Added some documentation here.

Harbormaster completed remote builds in B71307: Diff 291109.Sep 10 2020, 7:03 PM
cryptoad accepted this revision.Sep 10 2020, 7:06 PM
This revision was landed with ongoing or failed builds.Sep 10 2020, 7:16 PM
Closed by commit rG84c2c4977dfe: scudo: Introduce a new mechanism to let Scudo access a platform-specific TLS… (authored by pcc). · Explain Why
This revision was automatically updated to reflect the committed changes.
pcc added a commit: rG84c2c4977dfe: scudo: Introduce a new mechanism to let Scudo access a platform-specific TLS….
russell.gallop added a subscriber: russell.gallop.Sep 17 2020, 9:36 AM
russell.gallop added inline comments.
compiler-rt/lib/scudo/standalone/tsd_shared.h
188

Is this still needed? The key is just created and deleted with nothing set or get. Apologies if I've missed something.

cryptoad added inline comments.Sep 21 2020, 12:52 PM
compiler-rt/lib/scudo/standalone/tsd_shared.h
188

That's a good point, it's no longer used, and we don't use the key destructor for the shared model.
I'll remove it in another CL.

cryptoad mentioned this in D88046: [scudo][standalone] Remove the pthread key from the shared TSD.Sep 21 2020, 2:11 PM
russell.gallop added inline comments.Sep 22 2020, 1:40 AM
compiler-rt/lib/scudo/standalone/tsd_shared.h
188

Thanks

cryptoad mentioned this in rG7b51961cd074: [scudo][standalone] Remove the pthread key from the shared TSD.Sep 22 2020, 8:25 AM
cryptoad mentioned this in D90195: [GWP-ASan] Abstract the thread local variables access.Oct 26 2020, 2:54 PM
cryptoad mentioned this in rG90678f65ae47: [GWP-ASan] Abstract the thread local variables access.Oct 28 2020, 3:06 PM

Revision Contents

PathSize
compiler-rt/
lib/
scudo/
standalone/
45 lines
38 lines

Diff 291077

compiler-rt/lib/scudo/standalone/linux.h

Show All 12 Lines
#if SCUDO_LINUX #if SCUDO_LINUX
namespace scudo { namespace scudo {
// MapPlatformData is unused on Linux, define it as a minimally sized structure. // MapPlatformData is unused on Linux, define it as a minimally sized structure.
struct MapPlatformData {}; struct MapPlatformData {};
#if SCUDO_ANDROID
#if defined(__aarch64__)
#define __get_tls() \
({ \
void **__v; \
__asm__("mrs %0, tpidr_el0" : "=r"(__v)); \
__v; \
})
#elif defined(__arm__)
#define __get_tls() \
({ \
void **__v; \
__asm__("mrc p15, 0, %0, c13, c0, 3" : "=r"(__v)); \
__v; \
})
#elif defined(__i386__)
#define __get_tls() \
({ \
void **__v; \
__asm__("movl %%gs:0, %0" : "=r"(__v)); \
__v; \
})
#elif defined(__x86_64__)
#define __get_tls() \
({ \
void **__v; \
__asm__("mov %%fs:0, %0" : "=r"(__v)); \
__v; \
})
#else
#error "Unsupported architecture."
#endif
// The Android Bionic team has allocated a TLS slot for sanitizers starting
// with Q, given that Android currently doesn't support ELF TLS. It is used to
// store sanitizer thread specific data.
static const int TLS_SLOT_SANITIZER = 6;
ALWAYS_INLINE uptr *getAndroidTlsPtr() {
return reinterpret_cast<uptr *>(&__get_tls()[TLS_SLOT_SANITIZER]);
}
#endif // SCUDO_ANDROID
} // namespace scudo } // namespace scudo
#endif // SCUDO_LINUX #endif // SCUDO_LINUX
#endif // SCUDO_LINUX_H_ #endif // SCUDO_LINUX_H_

compiler-rt/lib/scudo/standalone/tsd_shared.h

//===-- tsd_shared.h --------------------------------------------*- C++ -*-===// //===-- tsd_shared.h --------------------------------------------*- C++ -*-===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#ifndef SCUDO_TSD_SHARED_H_ #ifndef SCUDO_TSD_SHARED_H_
#define SCUDO_TSD_SHARED_H_ #define SCUDO_TSD_SHARED_H_
#include "linux.h" // for getAndroidTlsPtr() #include "linux.h" // for getAndroidTlsPtr()
cryptoadUnsubmitted
Not Done
ReplyInline Actions

Is this still necessary?

cryptoad: Is this still necessary?
pccAuthorUnsubmitted
Done
ReplyInline Actions

No it isn't, removed.

pcc: No it isn't, removed.
#include "tsd.h" #include "tsd.h"
#if SCUDO_HAS_PLATFORM_TLS_SLOT
#include "scudo_platform_tls_slot.h"
cryptoadUnsubmitted
Not Done
ReplyInline Actions

Is there some requirement as to where this should live, or build time include path requirement that should be documented?

cryptoad: Is there some requirement as to where this should live, or build time include path requirement…
pccAuthorUnsubmitted
Done
ReplyInline Actions

Added some documentation here.

pcc: Added some documentation here.
#endif
namespace scudo { namespace scudo {
template <class Allocator, u32 TSDsArraySize, u32 DefaultTSDCount> template <class Allocator, u32 TSDsArraySize, u32 DefaultTSDCount>
struct TSDRegistrySharedT { struct TSDRegistrySharedT {
void initLinkerInitialized(Allocator *Instance) { void initLinkerInitialized(Allocator *Instance) {
Instance->initLinkerInitialized(); Instance->initLinkerInitialized();
CHECK_EQ(pthread_key_create(&PThreadKey, nullptr), 0); // For non-TLS CHECK_EQ(pthread_key_create(&PThreadKey, nullptr), 0); // For non-TLS
for (u32 I = 0; I < TSDsArraySize; I++) for (u32 I = 0; I < TSDsArraySize; I++)
▲ Show 20 Lines • Show All 52 Lines • ▼ Show 20 Linesstruct TSDRegistrySharedT {
bool setOption(Option O, sptr Value) { bool setOption(Option O, sptr Value) {
if (O == Option::MaxTSDsCount) if (O == Option::MaxTSDsCount)
return setNumberOfTSDs(static_cast<u32>(Value)); return setNumberOfTSDs(static_cast<u32>(Value));
// Not supported by the TSD Registry, but not an error either. // Not supported by the TSD Registry, but not an error either.
return true; return true;
} }
private: private:
ALWAYS_INLINE void setCurrentTSD(TSD<Allocator> *CurrentTSD) { ALWAYS_INLINE uptr *getTlsPtr() const {
#if _BIONIC #if SCUDO_HAS_PLATFORM_TLS_SLOT
*getAndroidTlsPtr() = reinterpret_cast<uptr>(CurrentTSD); return reinterpret_cast<uptr *>(getPlatformAllocatorTlsSlot());
#elif SCUDO_LINUX
ThreadTSD = CurrentTSD;
#else #else
CHECK_EQ( static thread_local uptr ThreadTSD;
pthread_setspecific(PThreadKey, reinterpret_cast<void *>(CurrentTSD)), return &ThreadTSD;
mcgrathrUnsubmitted
Done
ReplyInline Actions

You could just make it static thread_local in this function and not need any other conditionalization declarations below.

mcgrathr: You could just make it `static thread_local` in this function and not need any other…
pccAuthorUnsubmitted
Done
ReplyInline Actions

Good idea, done.

pcc: Good idea, done.
0);
#endif #endif
} }
ALWAYS_INLINE void setCurrentTSD(TSD<Allocator> *CurrentTSD) {
*getTlsPtr() = reinterpret_cast<uptr>(CurrentTSD);
}
ALWAYS_INLINE TSD<Allocator> *getCurrentTSD() { ALWAYS_INLINE TSD<Allocator> *getCurrentTSD() {
#if _BIONIC return reinterpret_cast<TSD<Allocator> *>(*getTlsPtr());
return reinterpret_cast<TSD<Allocator> *>(*getAndroidTlsPtr());
#elif SCUDO_LINUX
return ThreadTSD;
#else
return reinterpret_cast<TSD<Allocator> *>(pthread_getspecific(PThreadKey));
#endif
} }
bool setNumberOfTSDs(u32 N) { bool setNumberOfTSDs(u32 N) {
ScopedLock L(MutexTSDs); ScopedLock L(MutexTSDs);
if (N < NumberOfTSDs) if (N < NumberOfTSDs)
return false; return false;
if (N > TSDsArraySize) if (N > TSDsArraySize)
N = TSDsArraySize; N = TSDsArraySize;
▲ Show 20 Lines • Show All 70 Lines • ▼ Show 20 Lines if (N > 1U) {
return CandidateTSD; return CandidateTSD;
} }
} }
// Last resort, stick with the current one. // Last resort, stick with the current one.
CurrentTSD->lock(); CurrentTSD->lock();
return CurrentTSD; return CurrentTSD;
} }
pthread_key_t PThreadKey; pthread_key_t PThreadKey;
russell.gallopUnsubmitted
Not Done
ReplyInline Actions

Is this still needed? The key is just created and deleted with nothing set or get. Apologies if I've missed something.

russell.gallop: Is this still needed? The key is just created and deleted with nothing set or get. Apologies if…
cryptoadUnsubmitted
Not Done
ReplyInline Actions

That's a good point, it's no longer used, and we don't use the key destructor for the shared model.
I'll remove it in another CL.

cryptoad: That's a good point, it's no longer used, and we don't use the key destructor for the shared…
russell.gallopUnsubmitted
Not Done
ReplyInline Actions

Thanks

russell.gallop: Thanks
atomic_u32 CurrentIndex; atomic_u32 CurrentIndex;
u32 NumberOfTSDs; u32 NumberOfTSDs;
u32 NumberOfCoPrimes; u32 NumberOfCoPrimes;
u32 CoPrimes[TSDsArraySize]; u32 CoPrimes[TSDsArraySize];
bool Initialized; bool Initialized;
HybridMutex Mutex; HybridMutex Mutex;
HybridMutex MutexTSDs; HybridMutex MutexTSDs;
TSD<Allocator> TSDs[TSDsArraySize]; TSD<Allocator> TSDs[TSDsArraySize];
#if SCUDO_LINUX && !_BIONIC
static THREADLOCAL TSD<Allocator> *ThreadTSD;
#endif
}; };
mcgrathrUnsubmitted
Not Done
ReplyInline Actions

Should these conditionals be #if !SCUDO_HAS_PLATFORM_TLS_SLOT now since the code above won't compile if that's not defined and this isn't declared?

mcgrathr: Should these conditionals be `#if !SCUDO_HAS_PLATFORM_TLS_SLOT` now since the code above won't…
pccAuthorUnsubmitted
Done
ReplyInline Actions

Now moot

pcc: Now moot
#if SCUDO_LINUX && !_BIONIC
template <class Allocator, u32 TSDsArraySize, u32 DefaultTSDCount>
THREADLOCAL TSD<Allocator>
*TSDRegistrySharedT<Allocator, TSDsArraySize, DefaultTSDCount>::ThreadTSD;
#endif
} // namespace scudo } // namespace scudo
#endif // SCUDO_TSD_SHARED_H_ #endif // SCUDO_TSD_SHARED_H_