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

[Kevin Brodsky via Phabricator via llvm-commits]

kevin.brodsky added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:30
+  return (getauxval(AT_HWCAP2) & HWCAP2_MTE) &&
+         (prctl(PR_GET_TAGGED_ADDR_CTRL, 0, 0, 0, 0) & PR_MTE_TCF_MASK) !=
+             PR_MTE_TCF_NONE;
----------------
pcc wrote:
> pcc wrote:
> > kevin.brodsky wrote:
> > > I wonder if this is really a good thing. If libc fails to enable tag checking before the allocator is initialised (which is quite possible, given that until recently `malloc()` was called very early in Bionic's libc_init), then Scudo will not tag anything. Wouldn't it be possible instead to explicitly ask Scudo to use tagging when it is initialised? This would also be more consistent with the `malloc_disable_memory_tagging()` interface: Scudo does not take care of enabling / disabling tag checking, so arguably it shouldn't check if it is enabled either.
> > So the motivation behind adding the check here was along the lines of "if the application doesn't enable memory tag checks then there's no point in enabling memory tagging". But I can see the value in decoupling these two things especially since the libc might not have a chance to turn on memory tagging before the first allocation. I'll change this to be purely based on the hwcap and let the application call `malloc_disable_memory_tagging()` early if it doesn't want to use memory tagging.
> > 
> > I'd prefer not to add an explicit initialization step to the allocator since the allocation functions can in principle be called at any time, so we'd need additional complexity to handle the case where the allocation functions were called before the allocator was formally initialized.
> I forgot about the other reason why I added this check, which was to avoid breaking the tests if the libc did not issue the prctl to enable memory tag checks. This seems like something that can be properly checked for in the tests themselves, which I've now done.
OK this makes sense, always enabling tagging in Scudo and then disabling it explicitly should be fairly robust.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:76
+        [ End ] "=&r"(End)
+      : [ Ptr ] "r"(Ptr), [ Size ] "r"(Size));
+}
----------------
pcc wrote:
> kevin.brodsky wrote:
> > Since this asm statement is modifying memory, is it safe to use it without a "memory" clobber? It certainly isn't safe in general. Same comment for the other asm statements that use `st*g`.
> I was somehow under the impression that `volatile` implied the `"memory"` clobber, but that doesn't appear to be backed up by the documentation so I'll add the clobbers here and elsewhere.
My understanding is that `volatile` and `"memory"` do different things: the former tells the compiler not to (re)move the `asm` statement, while the latter tells the compiler that the the `asm` statement may perform reads or writes from arbitrary memory locations (forcing the compiler to reload values from memory as needed). GCC's manual has a rather good description of this: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:161
+  uptr TaggedPtr = Ptr;
+  __asm__ __volatile__(".arch_extension mte; ldg %0, [%0]" : "+r"(TaggedPtr));
+  return TaggedPtr;
----------------
Technically `"memory"` is required even for memory reads. That said, in that case, the only thing that could affect `ldg` is a `st*g`, which is only done in another `asm volatile` statement, so `"memory"` is probably not absolutely needed here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70762