[PATCH] D70762: scudo: Add initial memory tagging support.

[Branislav Rankov via Phabricator via llvm-commits]

rankov added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:509
+        uptr TaggedChunk = Chunk;
+        if (useMemoryTagging())
+          TaggedChunk = loadTag(Chunk);
----------------
Should you use UNLIKELY here?


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:50
+  void *TaggedPtr, *Cur, *End;
+  __asm__ __volatile__(
+      R"(
----------------
pcc wrote:
> hctim wrote:
> > These asm stubs seem mostly abstractable - which would allow us to extend to future platforms easier, and make the intermediate [read - non-mte instructions] code easier to maintain.
> > 
> > Looks like we could abstract away to `storeZeroTag` abd `randomTagMemory` (or similar).
> I created `setRandomTag` since I needed it for tag-on-free, although I feel that it's better to keep things at the chunk level here as splitting things into too many pieces can make it harder to understand the big picture.
Instead of using assembly here, you could use functions from arm_acle.h




================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:35
+inline void disableMemoryTagChecks() {
+  __asm__ __volatile__(".arch_extension mte; msr tco, #1");
+}
----------------
eugenis wrote:
> pcc wrote:
> > eugenis wrote:
> > > I don't think it is the job of a memory allocator to mess with PSTATE.TCO.
> > > Let the caller deal with it?
> > > 
> > I can see the argument either way.
> > 
> > On one hand, TCO is a thread-wide property which the caller could arguably be considered responsible for.
> > 
> > On the other, disabling tag checks is required in order to avoid tag check failures for future allocations which reuse previously tagged chunks, so to a certain extent it makes sense to perform an operation that is required in order to keep the allocator working, even if it involves setting what is technically a thread-wide property. The operation that we perform depends on the allocator's implementation, so arguably it belongs with the allocator. If for example we switched to mprotecting without PROT_MTE when tag checks are disabled, there would be no need to set TCO here.
> > 
> > If you still think that it belongs in the caller, maybe we could document that setting TCO is required, but this may change in the future.
> OK, SGTM, let's keep it here.
PSTATE.TCO can be easily changed by other code. So it is not a good choice for disabling tag checks in this case. 
Also, the current kernel documentation says that PSTATE.TCO is reset to 0 in signal handlers.

Another concern with disabling tagging is that when memory is deallocated, the tags will remain, so enabling tag checks again is dangerous. This should be documented. Alternative is to untag memory on deallocation after tagging is disabled, but this will add cost.

I think that it would be better to leave disabling tag checks to the caller. The tests could use PSTATE.TCO, other callers might want to use other ways like calling a prctl to disable tag checks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70762/new/

https://reviews.llvm.org/D70762