[tsan] Disable randomized address space on aarch64 linux.

[yabinc]

Summary

After patch https://lkml.org/lkml/2015/12/21/340 is introduced in
linux kernel, the random gap between stack and heap is increased
from 128M to 36G on 39-bit aarch64. And it is almost impossible
to cover this big range. So we need to disable randomized virtual
space on aarch64 linux.

[zatrazz]

Any particular reason you are making this patch Android specific? Based on your
previous patch [1] and on kernel source the kernel mapping change is for linux
on aarch64 in general.

[1] http://reviews.llvm.org/D18003

[yabinc]

That patch is reverted because of breaking aarch64 tests. I tried to fix that in http://reviews.llvm.org/D18378, but I think reviewers prefer to limit the change on Android platform. And whoever wants to extend the platforms disabling randomized virtual space can test on the extended platforms.

[zatrazz]

Right, I missed this thread. I think the problem is you are using 'Report', instead of 'VReport(1, ...)' as the the other check for re-exec (stack usage). This messes with test-suite because the warning will be interpreted as the expected output. By using VReport(1, "...") the failed tests presented at http://reviews.llvm.org/D18378 don't show anymore. Also, use only one VReport call (as for the stack size warning above).

[yabinc]

Replace Report() with VReport(), and enable the patch on aarch64.

[yabinc]

ping?

[dvyukov]

Renato, are you OK with this patch?

[rengolin]

I'm confused. This patch seems eerily similar to the one that broke the buildbots... Maybe you forgot to add ifdef Android when you added vReport?

My worry is simple: this patch broke not *all* AArch64 buildbots, but "some". Meaning, the idea may work on some but not on other configurations. This is very worrying.

Furthermore, it's not because we're not testing on Android (via buildbots) today, that we won't do that tomorrow. So, if this fix is *guaranteed* to work on *every* possible Android configuration on the planet, present and future, than I'm ok with the change.

But I'm very sceptical. The previous change was *guaranteed* to work with any Linux kernel, so we accepted for all AArch64, but that was a false assumption. So forgive me if I don't trust this new assumption as well.

Now Android will be building with LLVM by default, so it's not an experimental tool any more. We used to accept "experimental" patches before because not many people were fiddling with sanitizers on Android, but this will not be the case *very* soon. In fact, we're already planning our Android buildbots as we speak, so the more false assumptions we put in now (because it happens to work on a particular developer's device), the harder it will be to get those bots green.

Concretely, what I'm asking is proof that this is true for all Android configurations. Virtual address randomization is a crucial security feature, and I'd be surprised if Google never uses it as we move into 64-bit land, especially as Android takes on more roles in automotive, home automation and small laptops.

cheers,
--renato

[zatrazz]

In D18526#390617, @rengolin wrote:

I'm confused. This patch seems eerily similar to the one that broke the buildbots... Maybe you forgot to add ifdef Android when you added vReport?

My worry is simple: this patch broke not *all* AArch64 buildbots, but "some". Meaning, the idea may work on some but not on other configurations. This is very worrying.

It was breaking the buildbot because the warning message was being handled as
the expected TSAN output instead of just an warning. The logic is similar as when
running with unlimited stack (ulimit -s unlimited): tsan will warning and reexec, but
the output won't be printed as

Furthermore, it's not because we're not testing on Android (via buildbots) today, that we won't do that tomorrow. So, if this fix is *guaranteed* to work on *every* possible Android configuration on the planet, present and future, than I'm ok with the change.

But I'm very sceptical. The previous change was *guaranteed* to work with any Linux kernel, so we accepted for all AArch64, but that was a false assumption. So forgive me if I don't trust this new assumption as well.

Now Android will be building with LLVM by default, so it's not an experimental tool any more. We used to accept "experimental" patches before because not many people were fiddling with sanitizers on Android, but this will not be the case *very* soon. In fact, we're already planning our Android buildbots as we speak, so the more false assumptions we put in now (because it happens to work on a particular developer's device), the harder it will be to get those bots green.

Concretely, what I'm asking is proof that this is true for all Android configurations. Virtual address randomization is a crucial security feature, and I'd be surprised if Google never uses it as we move into 64-bit land, especially as Android takes on more roles in automotive, home automation and small laptops.

I can't say for every kernel out there, but for the ones I have access (which is unfortunately
only old ones without the new patch that disrupt it), the testsuite works. I would expect that
yabinc tests on Android on 39-VMA with and without the kernel patch that triggered this
modification.

[yabinc]

At Diff1 of this patch, I made it Android specific. And zatrazz recommended
to use VReport() instead of Report() to avoid breaking tests. It doesn't break tests any more when I run make check-tsan.

This patch broke not *all* AArch64 buildbots, but "some".

Can you specify what build it may break? As far as I know, export TSAN_OPTIONS="verbosity=1" makes tsan print the verbose warning. But make check-tsan doesn't fail, I think it may be because make check-tsan clears environment variable TSAN_OPTIONS. Maybe you want me to fix the tests, comment on http://reviews.llvm.org/D18378?

if this fix is *guaranteed* to work on *every* possible Android configuration on the planet, present and future, than I'm ok with the change.

I think this is an unresonable promise for me. I have tested on the aarch64 android devices I have: N5X, N9, and N6P, which are all 39-VMAs,
without and with the kernel patch. Unless you have specific considerations or actionable suggests, I can't do anything further.

The previous change was *guaranteed* to work with any Linux kernel, so we accepted for all AArch64, but that was a false assumption. So forgive me if I don't trust this new assumption as well.

I don't think that was a false assumption. The previous change broken the test because of the tests. There is no sense so far that the change doesn't work with any Linux kernel.

Concretely, what I'm asking is proof that this is true for all Android configurations. Virtual address randomization is a crucial security feature, and I'd be surprised if Google never uses it as we move into 64-bit land, especially as Android takes on more roles in automotive, home automation and small laptops.

I think the current plan on android is only using tsan for debugging by platform developers and app developers, so security issue isn't involved so far (or never will be). If the tsan maintainers determine that they don't want to disable virtual address randomization, android developers have to disable virtual address space manually before tsan has a workable solution. The only way to make something true for all Android configurations is to write a Compatibility Test Suite for it, but I can't do it before I make tsan usable by platform developers (because how can you test that something is true before you can't run the test).

[rengolin]

In D18526#391720, @yabinc wrote:

if this fix is *guaranteed* to work on *every* possible Android configuration on the planet, present and future, than I'm ok with the change.

I think this is an unresonable promise for me. I have tested on the aarch64 android devices I have: N5X, N9, and N6P, which are all 39-VMAs,
without and with the kernel patch. Unless you have specific considerations or actionable suggests, I can't do anything further.

That's what I wanted to know, what and how did you test. Adhemerval has tested on two VMA configurations for different kernels without the patch, and you have tested for the same VMA, but on kernels with and without the patch. That is enough coverage for me.

The previous change was *guaranteed* to work with any Linux kernel, so we accepted for all AArch64, but that was a false assumption. So forgive me if I don't trust this new assumption as well.

I don't think that was a false assumption. The previous change broken the test because of the tests. There is no sense so far that the change doesn't work with any Linux kernel.

Good point.

Concretely, what I'm asking is proof that this is true for all Android configurations. Virtual address randomization is a crucial security feature, and I'd be surprised if Google never uses it as we move into 64-bit land, especially as Android takes on more roles in automotive, home automation and small laptops.

I think the current plan on android is only using tsan for debugging by platform developers and app developers, so security issue isn't involved so far (or never will be). If the tsan maintainers determine that they don't want to disable virtual address randomization, android developers have to disable virtual address space manually before tsan has a workable solution. The only way to make something true for all Android configurations is to write a Compatibility Test Suite for it, but I can't do it before I make tsan usable by platform developers (because how can you test that something is true before you can't run the test).

So, when I create a buildbot for Android, covering the sanitizers, will I have to disable ASRL? If so, and (assuming) I can only add *one* buildbot, won't I be testing an environment that will never be seen in production? I don't think that's good enough.

That is precisely my point about Android/LLVM not being a toy project any more. We have to start thinking seriously about it. "Security isn't involved so far" is not an option any more.

Having said that, as far as this patch goes, I'm ok with it going as it is. I have no more objections since not only you two have shown that you covered the necessary tests, but also I don't have a buildbot to show breakages on. In that sense, LGTM.

But I want us to start thinking about the bigger picture here very soon. It'd also be good if Google could setup up a buildbot on Nexus devices. I really hate the idea that you guys are pushing patches upstream without any demonstration of it working in public.

cheers,
--renato

[yabinc]

when I create a buildbot for Android, covering the sanitizers, will I have to disable ASRL?

I don't know other sanitizers. But as in this patch, tsan will do it for you.

That is precisely my point about Android/LLVM not being a toy project any more.

I never intend to say Android/LLVM doesn't involve security issue. But for tsan, as it consumes much more memory and runtime slowdown currently, I think no one intend to ship it on product.

But I want us to start thinking about the bigger picture here very soon. It'd also be good if Google could setup up a buildbot on Nexus devices.

We don't have Android/LLVM buildbot now. But I think it is nice to have. srhines@ and danalbert@ should be the right people to talk to.

[dvyukov]

We don't have Android/LLVM buildbot now. But I think it is nice to have. srhines@ and danalbert@ should be the right people to talk to.

Please file a bug for this.

[yabinc]

I have filed an internal bug. Not sure when it will be available.

[rengolin]

Thanks Yabin, Dmitry!

lib/tsan/rtl/tsan_platform_linux.cc

 #include <fcntl.h>
 #include <pthread.h>
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <stdarg.h>
 #include <sys/mman.h>
+#include <sys/personality.h>
 #include <sys/syscall.h>
 #include <sys/socket.h>
 #include <sys/time.h>
 #include <sys/types.h>
 #include <sys/resource.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <errno.h>
if (kCppMode) {
 
     if (!AddressSpaceIsUnlimited()) {
       Report("WARNING: Program is run with limited virtual address space,"
              " which wouldn't work with ThreadSanitizer.\n");
       Report("Re-execing with unlimited virtual address space.\n");
       SetAddressSpaceUnlimited();
       reexec = true;
     }
+#if defined(__aarch64__)
+    // After patch "arm64: mm: support ARCH_MMAP_RND_BITS." is introduced in
+    // linux kernel, the random gap between stack and mapped area is increased
+    // from 128M to 36G on 39-bit aarch64. As it is almost impossible to cover
+    // this big range, we should disable randomized virtual space on aarch64.
+    int old_personality = personality(0xffffffff);
+    if (old_personality != -1 && (old_personality & ADDR_NO_RANDOMIZE) == 0) {
+      VReport(1, "WARNING: Program is run with randomized virtual address "
+              "space, which wouldn't work with ThreadSanitizer.\n"
+              "Re-execing with fixed virtual address space.\n");
+      CHECK_NE(personality(old_personality | ADDR_NO_RANDOMIZE), -1);
+      reexec = true;
+    }
+#endif
     if (reexec)
       ReExec();
   }
 
 #ifndef SANITIZER_GO
   CheckAndProtect();
   InitTlsSize();
   InitDataSeg();