[ARM][AArch64] Workaround ARM/AArch64 percularity in clearing icache.

[maxim-kuvyrkov]

Summary

Certain ARM implementations treat icache clear instruction as a memory read,
and CPU segfaults on trying to clear cache on !PROT_READ page.
We workaround this in Memory::protectMappedMemory by adding
PROT_READ to affected pages, clearing the cache, and then setting
desired protection.

This fixes "AllocationTests/MappedMemoryTest.***/3" unit-tests on
affected hardware.

[maxim-kuvyrkov]

The problem manifests itself in failing "AllocationTests/MappedMemoryTest.***/3" unit-tests when running on ARMv8.2 hardware with linux kernel version ~4.10.

[jgreenhalgh]

I'm confused by the title and rationale for this change. I don't think this has anything to do with Armv8.2-A and is instead a feature of Armv8-A.

Looking at C.5.12 in a recent release (ARM DDI 0487B.a) Arm Architecture Reference Manual I see "If EL0 access is enabled, when executing at EL0, this instruction requires read access permission to the VA, otherwise it is IMPLEMENTATION DEFINED whether it causes a Permission Fault." that text (or a close equivalent) looks to have been in the manual since the first release.

So the change makes sense, but the references to Armv8.2-A look wrong to me.

lib/Support/Unix/Memory.inc

172 ↗

(On Diff #124158)

Probably this should be !(Protect & PROT_READ)

[maxim-kuvyrkov]

I've successfully tested an earlier version of this patch, but not the uploaded version yet. Please review, but don't submit until testing is finished.

lib/Support/Unix/Memory.inc

172 ↗

(On Diff #124158)

Indeed! Thanks.

[maxim-kuvyrkov]

In D40423#934358, @jgreenhalgh wrote:

...

So the change makes sense, but the references to Armv8.2-A look wrong to me.

I've checked the docs once again, and, indeed, they don't mention Armv8.2, so I'll remove the reference. However, judging from cursory read of https://patchwork.kernel.org/patch/9275721/ , it appears only Armv8.2 kernels would set EXECUTE_ONLY bits on the pages, kernels for Armv8.[01] would set READ permissions on EXEC pages as well.

[maxim-kuvyrkov]

Updated patch with James' comments addressed (testing is still in progress).

[peter.smith]

The code changes make sense to me. There is a small typo in the title, it should be "peculiarity", not worth updating the review but will be worth changing in any commit message. It will be worth waiting to see if there are comments early next week from the US due to thanksgiving.

[jgreenhalgh]

In D40423#934427, @maxim-kuvyrkov wrote:

In D40423#934358, @jgreenhalgh wrote:

...

So the change makes sense, but the references to Armv8.2-A look wrong to me.

I've checked the docs once again, and, indeed, they don't mention Armv8.2, so I'll remove the reference. However, judging from cursory read of https://patchwork.kernel.org/patch/9275721/ , it appears only Armv8.2 kernels would set EXECUTE_ONLY bits on the pages, kernels for Armv8.[01] would set READ permissions on EXEC pages as well.

My reading (confirmed by our kernel team here) is that you'll be EXECUTE_ONLY at all architecture levels. The difference is that prior to Armv8.2 you don't have a Privileged Access Never (PAN) bit, so EL1 (kernel) can read EL0 (user)'s execute only data. With PAN+UAO in Armv8.2-A, even the kernel can't read the user execute only data.

So this code has likely always been buggy for AArch64, we just didn't see it on Linux systems until Linux 4.9 (4.10?) started allowing use of execute-only user permissions.

[maxim-kuvyrkov]

@jgreenhalgh , thanks for the info.

I've successfully tested the latest version of the patch with no failures, and with 8 progressions on AArch64 machine with 4.10 kernel.

[zatrazz]

lib/Support/Unix/Memory.inc

134 ↗

(On Diff #124169)

I think instead of issue a mmap plus two mprotects for MF_EXEC, it would be better if we could just mmap with read permission, call InvalidateInstructionCache and then mmap to the expected protection. Something as:

int adjustAllocateMappedProtectionFlags (unsigned Flags)
{
#if defined(__NetBSD__) && defined(PROT_MPROTECT)
  Protect |= PROT_MPROTECT(PROT_READ | PROT_WRITE | PROT_EXEC);
#endif
#if defined(__arm__) || defined(__aarch64__)
  Protect |= PROT_READ;
#endif
  return Protect;
}

[...]

MemoryBlock
Memory::allocateMappedMemory(size_t NumBytes,
                             const MemoryBlock *const NearBlock,
                             unsigned PFlags,
                             std::error_code &EC) {
  [...]
  int Protect = getPosixProtectionFlags(PFlags);

  Protect = adjustAllocateMappedProtectionFlags (Protect);
  [...]

And then issue the InvalidateInstructionCache without requiring to mprotect to PROT_READ first:

std::error_code
Memory::protectMappedMemory(const MemoryBlock &M, unsigned Flags) {
  [...]
  bool InvalidateCache = (Flags & MF_EXEC);

#if defined(__arm__) || defined(__aarch64__)
  // Certain ARM implementations treat icache clear instruction as a memory read,
  // and CPU segfaults on trying to clear cache on !PROT_READ page.  Therefore we need
  // to temporarily add PROT_READ for the sake of flushing the instruction caches.
  if (InvalidateCache && !(Protect & PROT_READ)) {
    Memory::InvalidateInstructionCache(M.Address, M.Size);
    InvalidateCache = false;
  }
#endif

[krytarowski]

I think Memory should be removed as it's impossible to make it portable as is to every combination of OS and hardware. It's too low-level interface.

[lhames]

This workaround seems ok in the short term. Longer term I think Kamil is right: Memory should be nuked in favor of something higher level.

[maxim-kuvyrkov]

lib/Support/Unix/Memory.inc

134 ↗

(On Diff #124169)

I experimented with that, and the code starts to look much uglier. This file is target-independent, and it seems better to localize all workaround code in a single function, rather than spreading it across several functions.

[maxim-kuvyrkov]

Would someone with commit access please merge this?

[peter.smith]

I've committed it for you r319166

llvm/trunk/lib/Support/Unix/Memory.inc

if (Addr == MAP_FAILED) {
     EC = std::error_code(errno, std::generic_category());
     return MemoryBlock();
   }
 
   MemoryBlock Result;
   Result.Address = Addr;
   Result.Size = NumPages*PageSize;
 
-  if (PFlags & MF_EXEC)
-    Memory::InvalidateInstructionCache(Result.Address, Result.Size);
+  // Rely on protectMappedMemory to invalidate instruction cache.
+  if (PFlags & MF_EXEC) {
+    EC = Memory::protectMappedMemory (Result, PFlags);
+    if (EC != std::error_code())
+      return MemoryBlock();
+  }
 
   return Result;
 }
 
 std::error_code
 Memory::releaseMappedMemory(MemoryBlock &M) {
   if (M.Address == nullptr || M.Size == 0)
     return std::error_code();
Memory::protectMappedMemory(const MemoryBlock &M, unsigned Flags) {
   static const size_t PageSize = Process::getPageSize();
   if (M.Address == nullptr || M.Size == 0)
     return std::error_code();
 
   if (!Flags)
     return std::error_code(EINVAL, std::generic_category());
 
   int Protect = getPosixProtectionFlags(Flags);
-
   uintptr_t Start = alignAddr((uint8_t *)M.Address - PageSize + 1, PageSize);
   uintptr_t End = alignAddr((uint8_t *)M.Address + M.Size, PageSize);
+
+  bool InvalidateCache = (Flags & MF_EXEC);
+
+#if defined(__arm__) || defined(__aarch64__)
+  // Certain ARM implementations treat icache clear instruction as a memory read,
+  // and CPU segfaults on trying to clear cache on !PROT_READ page.  Therefore we need
+  // to temporarily add PROT_READ for the sake of flushing the instruction caches.
+  if (InvalidateCache && !(Protect & PROT_READ)) {
+    int Result = ::mprotect((void *)Start, End - Start, Protect | PROT_READ);
+    if (Result != 0)
+      return std::error_code(errno, std::generic_category());
+
+    Memory::InvalidateInstructionCache(M.Address, M.Size);
+    InvalidateCache = false;
+  }
+#endif
+
   int Result = ::mprotect((void *)Start, End - Start, Protect);
 
   if (Result != 0)
     return std::error_code(errno, std::generic_category());
 
-  if (Flags & MF_EXEC)
+  if (InvalidateCache)
     Memory::InvalidateInstructionCache(M.Address, M.Size);
 
   return std::error_code();
 }
 
 /// InvalidateInstructionCache - Before the JIT can run a block of code
 /// that has been emitted it must invalidate the instruction cache on some
 /// platforms.