[PATCH] [ASan] Initial support for Kernel AddressSanitizer

[Alexander Potapenko]

Addressed Alexey's comments.
Regarding the missing tests, I think it's best to add them at opt level, but I can't do that now because there's no command line switch that enables KASan (this mode only depends on the bool passed to the pass factory). I can add such a switch, but then it can be used to work around the "ASan xor KASan" restriction. WDYT?


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1359
@@ -1349,3 +1358,3 @@
 
   bool Changed = false;
 
----------------
ygribov wrote:
> samsonov wrote:
> > Wait a second.... Do you do *anything* in AddressSanitizerModule pass in `CompileKernel` mode? You don't add module constructor, and you don't instrument globals. Maybe, we should just avoid creating this pass in the first place?
> Or perhaps instrumentation of globals will follow shortly so it's ok to have this pass at hand.
> Or perhaps instrumentation of globals will follow shortly so it's ok to have this pass at hand.
Exactly, my intention was to add it soon.

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1379
@@ -1368,2 +1378,3 @@
       const std::string ExpStr = Exp ? "exp_" : "";
+      const std::string SuffixStr = CompileKernel ? "N" : "_n";
       const Type *ExpType = Exp ? Type::getInt32Ty(*C) : nullptr;
----------------
ygribov wrote:
> Perhaps use "_noabort" suffix to match gcc?
Looks like I've been comparing to the wrong gcc version, 4.9 didn't have the 'noabort' suffixes. Fixed.

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1405
@@ -1393,1 +1404,3 @@
+  const std::string MemIntrinCallbackPrefix =
+      CompileKernel ? std::string("") : ClMemoryAccessCallbackPrefix;
   AsanMemmove = checkSanitizerInterfaceFunction(M.getOrInsertFunction(
----------------
samsonov wrote:
> Hm... So you don't have __asan_memset() in the kernel. Maybe, we shouldn't instrument mem intrinsics in this case at all, not replace intrinisics with call to plain memset()?
mm/kasan/kasan.c redefines memset() et al. making them call __asan_{load,store}N (see https://github.com/ramosian-glider/kasan/blob/kasan_llvmlinux/mm/kasan/kasan.c#L263), so we just replace the intrinsics with calls to memset().

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1441
@@ +1440,3 @@
+  if (!CompileKernel) {
+    std::tie(AsanCtorFunction, AsanInitFunction) =
+        createSanitizerCtorAndInitFunctions(M, kAsanModuleCtorName,
----------------
samsonov wrote:
> You will access uninitialized memory in `AddressSanitizer::runOnFunction` (potentially in `AddressSanitizer::maybeInsertAsanInitAtFunctionEntry` as well, but the latter is relevant only for Mac).
In fact I didn't fully understand this piece before. I've looked std::tie up and it's more clear to me now.
Moved the assignment outside the condition (or shall we NULL-initialize these fields instead?)

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1530
@@ -1515,3 +1529,3 @@
 
-  bool UseCalls = false;
+  bool UseCalls = CompileKernel;
   if (ClInstrumentationWithCallsThreshold >= 0 &&
----------------
samsonov wrote:
>   bool UseCalls = CompileKernel || (ClInstrumentationWithCallsThreshold >= 0 && ...);
Done.

================
Comment at: tools/clang/include/clang/Basic/Sanitizers.def:44
@@ -43,1 +43,3 @@
 
+// Kernel AddressSanitizer (KASAN)
+SANITIZER("kernel-address", KernelAddress)
----------------
samsonov wrote:
> What is the "correct" way to capitalize it? KASAN? KASan?
It hasn't been established yet, but Dima says we prefer "KASan" (which surprised me). Fixed here and in tools/clang/lib/CodeGen/CodeGenModule.cpp

================
Comment at: tools/clang/lib/AST/Decl.cpp:3684
@@ -3683,2 +3683,3 @@
   ASTContext &Context = getASTContext();
-  if (!Context.getLangOpts().Sanitize.has(SanitizerKind::Address) ||
+  if (!(Context.getLangOpts().Sanitize.has(SanitizerKind::Address) ||
+        Context.getLangOpts().Sanitize.has(SanitizerKind::KernelAddress)) ||
----------------
samsonov wrote:
> Wow, that's hard to parse. I'd vote for adding a method to `SanitizerSet`
>   if (!Context.getLangOpts().Sanitize.hasOneOf(SanitizerKind::Address, SanitizerKind::KernelAddress)
>     ...
> 
> That would also be useful in several places below.
I've added SanitizerSet::hasAsanOrKasan() -- did you mean that?

================
Comment at: tools/clang/lib/CodeGen/BackendUtil.cpp:211
@@ +210,3 @@
+  PM.add(createAddressSanitizerFunctionPass(/*CompileKernel*/true));
+  PM.add(createAddressSanitizerModulePass(/*CompileKernel*/true));
+}
----------------
samsonov wrote:
> See note above - I'm not sure we need a module pass for that.
Can we keep it provided that we'll have module-level instrumentation soon?

================
Comment at: tools/clang/lib/CodeGen/CodeGenFunction.cpp:619
@@ -619,2 +618,3 @@
+  if (SanOpts.has(SanitizerKind::Address) || SanOpts.has(SanitizerKind::KernelAddress))
     Fn->addFnAttr(llvm::Attribute::SanitizeAddress);
   if (SanOpts.has(SanitizerKind::Thread))
----------------
Note I'm using the same attribute for both userspace and kernel instrumentation. I think we don't need to distinguish between them.

http://reviews.llvm.org/D10411

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/