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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 3 inline comments as done.
pcc added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:509
+        uptr TaggedChunk = Chunk;
+        if (useMemoryTagging())
+          TaggedChunk = loadTag(Chunk);
----------------
rankov wrote:
> Should you use UNLIKELY here?
malloc_iterate is a rarely used debugging function, so we shouldn't worry too much about this sort of thing here I think.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:50
+  void *TaggedPtr, *Cur, *End;
+  __asm__ __volatile__(
+      R"(
----------------
rankov wrote:
> 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
> 
> 
Without `__attribute__((target("mte")))` on these functions I get errors such as
```
fatal error: error in backend: Cannot select: intrinsic %llvm.aarch64.ldg
```
if I try to use the intrinsics. And with that attribute the functions don't get inlined because LLVM IR doesn't support scoping the availability of the instructions to a block (unlike `.arch_extension` in inline asm). So I'd prefer to stick with inline asm for now.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:35
+inline void disableMemoryTagChecks() {
+  __asm__ __volatile__(".arch_extension mte; msr tco, #1");
+}
----------------
rankov wrote:
> 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.
Yes, I had intended to switch this over to using a prctl once the kernel patches to support prctl became available.

Now that I think about it, it would be a good idea to use PSTATE.TCO instead of prctl in the tests in order to avoid interfering with the "real" allocator's tag checking state during the tests. The fact that PSTATE.TCO is set to 0 during a signal handler is a good reason why the real allocator should use prctl and not TCO to disable tag checks. I think that's a good argument for moving the functionality into the caller. I'll do that then. I'll also make it clear that this is a one way operation and tag checks should not be turned back on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70762