[PATCH] D90426: hwasan: Support for outlined checks in the Linux kernel.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h:40
 
+namespace HWASanAccessInfo {
+
----------------
hctim wrote:
> why not `enum HWASanAccessInfo { ...`?
Because we don't want these to go straight into the `llvm` namespace. `enum class` avoids this but then the enumerators would be more awkward to work with in this context (e.g. need to be `static_cast`ed into integers).


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:396
+    if (HasMatchAllTag) {
+      OutStreamer->emitInstruction(MCInstBuilder(AArch64::UBFMXri)
+                                       .addReg(AArch64::X16)
----------------
hctim wrote:
> IIUC, this is being optimised to
> 
> ```
> lsr x16, x1, #56
> cmp x16, MatchAllTag
> b.eq .ret
> ```
> 
> and this seems clearer to me. Is it possible to update the codegen here to be the same?
There is no `AArch64::LSR` or `AArch64::CMP`. When the assembler sees one of these it rewrites the instruction into the more general form (i.e. `UBFM`, `SUBS`). So we can't make it the same.


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:513
+      OutStreamer->emitInstruction(
+          MCInstBuilder(AArch64::B).addExpr(HwasanTagMismatchRef), *STI);
+    } else {
----------------
hctim wrote:
> `B` instead of `BR` implies that the kernel will never return from `__hwasan_tag_mismatch` (which is still reachable as it looks like it's still possible for the kernel to compile without short granules). Does this assumption always hold?
> B instead of BR implies that the kernel will never return from __hwasan_tag_mismatch

No it doesn't. With either `B` or `BR` x30 is not overwritten so a `RET` in `__hwasan_tag_mismatch` would go straight back to the caller of `__hwasan_check_*`. This is how tail calls work for example.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:505
+      TargetTriple.isAArch64() && TargetTriple.isOSBinFormatELF() &&
+      (ClInlineAllChecks.getNumOccurrences() ? !ClInlineAllChecks : !Recover);
+
----------------
eugenis wrote:
> I don't understand the impact of this diff.
> Now, Recover==true no longer overrides an explicit -hwasan-inline-all-checks=1.
> Why?
> Is that what prevented outlining in the kernel before?
Yes, outlining was being prevented in the kernel because it always had `Recover` enabled (see `addKernelHWAddressSanitizerPasses` in clang). What this diff lets you do is say `-hwasan-inline-all-checks=0` and as a result you are promising that your runtime supports recovery from outlined checks.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:510
+      HasMatchAllTag = true;
+      MatchAllTag = ClMatchAllTag;
+    }
----------------
eugenis wrote:
> ClMatchTag & 0xF
> either here or when constructing AccessInfo
You mean `ClMatchAllTag & 0xFF`, right? This isn't MTE :)

It is unnecessary because the field is a uint8_t but I suppose we could avoid an implicit truncation that way.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:796
           FunctionType::get(IRB.getVoidTy(), {PtrLong->getType()}, false),
-          "int3\nnopl " + itostr(0x40 + AccessInfo) + "(%rax)",
+          "int3\nnopl " + itostr(0x40 + (AccessInfo & 0xffff)) + "(%rax)",
           "{rdi}",
----------------
hctim wrote:
> can we declare this as a const `kAccessInfoRuntimeMask` (and elsewhere)?
Okay, I'll add it to HWAddressSanitizer.h.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90426