[PATCH] D18526: [tsan] Disable randomized address space on aarch64 linux.

[Renato Golin via llvm-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

In http://reviews.llvm.org/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


http://reviews.llvm.org/D18526