<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 11, 2015 at 12:54 PM, Adhemerval Zanella <span dir="ltr"><<a href="mailto:adhemerval.zanella@linaro.org" target="_blank">adhemerval.zanella@linaro.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
On 11-09-2015 15:20, Alexey Samsonov wrote:<br>
> I don't think this is a good change as-is. Please follow-up with the code review fixing the issues pointed below.<br>
><br>
><br>
</span><div><div class="h5">> On Fri, Sep 11, 2015 at 6:55 AM, Adhemerval Zanella via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a> <mailto:<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>>> wrote:<br>
><br>
> Author: azanella<br>
> Date: Fri Sep 11 08:55:00 2015<br>
> New Revision: 247413<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=247413&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=247413&view=rev</a><br>
> Log:<br>
> [compiler-rt] [sanitizers] Add VMA size check at runtime<br>
><br>
> This patch adds a runtime check for asan, dfsan, msan, and tsan for<br>
> architectures that support multiple VMA size (like aarch64). Currently<br>
> the check only prints a warning indicating which is the VMA built and<br>
> expected against the one detected at runtime.<br>
><br>
><br>
> Modified:<br>
> compiler-rt/trunk/lib/asan/asan_rtl.cc<br>
> compiler-rt/trunk/lib/dfsan/dfsan.cc<br>
> compiler-rt/trunk/lib/msan/msan.cc<br>
> compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h<br>
> compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.cc<br>
> compiler-rt/trunk/lib/sanitizer_common/sanitizer_win.cc<br>
> compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.cc<br>
><br>
> Modified: compiler-rt/trunk/lib/asan/asan_rtl.cc<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/asan_rtl.cc?rev=247413&r1=247412&r2=247413&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/asan_rtl.cc?rev=247413&r1=247412&r2=247413&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/asan/asan_rtl.cc (original)<br>
> +++ compiler-rt/trunk/lib/asan/asan_rtl.cc Fri Sep 11 08:55:00 2015<br>
> @@ -585,6 +585,7 @@ void NOINLINE __asan_set_death_callback(<br>
> // Initialize as requested from instrumented application code.<br>
> // We use this call as a trigger to wake up ASan from deactivated state.<br>
> void __asan_init() {<br>
> + CheckVMASize();<br>
><br>
><br>
> Hm? I feel that this is far too early - the runtime is not properly initialized at this point yet. I believe you have to find a different place<br>
> for CheckVMASize() call. It should at least be after the runtime flags initialization in AsanInitInternal() - for example where we call<br>
><br>
> AsanCheckIncompatibleRT();<br>
> AsanCheckDynamicRTPrereqs();<br>
<br>
</div></div>Right, but currently the only facility it requires is the warning using Printf, which<br>
in my understanding it not dependent of the sanitizer initialization.</blockquote><div><br></div><div>Printf behavior does depend on the runtime flags. Also, I would really like to have all the compatibility checks</div><div>etc. grouped together, and after we have the most basic parts of runtime initialized.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> And different<br>
that he AsanCheckXXX it is not really dependent of runtime information rather than<br>
the kernel VMA.<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
So do we really require to move it after sanitizer initialization to print the<br>
warning message?<br></blockquote><div><br></div><div>Yes please. At least some parts of sanitizer initialization.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
><br>
><br>
><br>
><br>
> AsanActivate();<br>
> AsanInitInternal();<br>
> }<br>
><br>
> Modified: compiler-rt/trunk/lib/dfsan/dfsan.cc<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/dfsan/dfsan.cc?rev=247413&r1=247412&r2=247413&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/dfsan/dfsan.cc?rev=247413&r1=247412&r2=247413&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/dfsan/dfsan.cc (original)<br>
> +++ compiler-rt/trunk/lib/dfsan/dfsan.cc Fri Sep 11 08:55:00 2015<br>
> @@ -399,6 +399,8 @@ static void dfsan_fini() {<br>
> }<br>
><br>
> static void dfsan_init(int argc, char **argv, char **envp) {<br>
> + CheckVMASize();<br>
> +<br>
> MmapFixedNoReserve(kShadowAddr, kUnusedAddr - kShadowAddr);<br>
><br>
><br>
><br>
> Same here. Please parse the flags first. If you want to prevent calling Mmap() without checking VMA first, move<br>
> the InitializeFlags() call up (this would be the correct change).<br>
<br>
</span>The idea is not to prevent the mmap neither to bail out the binary, but rather<br>
inform the use that different VMA is assumed in sanitizer and the one kernel<br>
is providing.<br></blockquote><div><br></div><div>Sure, then just left the VMA checks before the mmap (which could fail), but move the flags initialization earlier.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
><br>
><br>
><br>
> // Protect the region of memory we don't use, to preserve the one-to-one<br>
><br>
> Modified: compiler-rt/trunk/lib/msan/msan.cc<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/msan/msan.cc?rev=247413&r1=247412&r2=247413&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/msan/msan.cc?rev=247413&r1=247412&r2=247413&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/msan/msan.cc (original)<br>
> +++ compiler-rt/trunk/lib/msan/msan.cc Fri Sep 11 08:55:00 2015<br>
> @@ -375,6 +375,8 @@ void __msan_init() {<br>
> msan_init_is_running = 1;<br>
> SanitizerToolName = "MemorySanitizer";<br>
><br>
> + CheckVMASize();<br>
> +<br>
><br>
><br>
> Same here.<br>
><br>
><br>
> InitTlsSize();<br>
><br>
> CacheBinaryName();<br>
><br>
> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h?rev=247413&r1=247412&r2=247413&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h?rev=247413&r1=247412&r2=247413&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h (original)<br>
> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h Fri Sep 11 08:55:00 2015<br>
> @@ -97,6 +97,8 @@ void DecreaseTotalMmap(uptr size);<br>
> uptr GetRSS();<br>
> void NoHugePagesInRegion(uptr addr, uptr length);<br>
> void DontDumpShadowMemory(uptr addr, uptr length);<br>
> +// Check if the built VMA size matches the runtime one.<br>
> +void CheckVMASize();<br>
><br>
><br>
><br>
><br>
> // InternalScopedBuffer can be used instead of large stack arrays to<br>
> // keep frame size low.<br>
><br>
> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.cc<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.cc?rev=247413&r1=247412&r2=247413&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.cc?rev=247413&r1=247412&r2=247413&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.cc (original)<br>
> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.cc Fri Sep 11 08:55:00 2015<br>
> @@ -324,6 +324,22 @@ SignalContext SignalContext::Create(void<br>
> return SignalContext(context, addr, pc, sp, bp);<br>
> }<br>
><br>
> +// This function check is the built VMA matches the runtime one for<br>
> +// architectures with multiple VMA size.<br>
> +void CheckVMASize() {<br>
> +#ifdef __aarch64__<br>
> + static const unsigned kBuiltVMA = SANITIZER_AARCH64_VMA;<br>
> + unsigned maxRuntimeVMA =<br>
> + (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1);<br>
> + if (kBuiltVMA != maxRuntimeVMA) {<br>
> + Printf("WARNING: %s runtime VMA is not the one built for.\n",<br>
> + SanitizerToolName);<br>
> + Printf("\tBuilt VMA: %u bits\n", kBuiltVMA);<br>
> + Printf("\tRuntime VMA: %u bits\n", maxRuntimeVMA);<br>
> + }<br>
> +#endif<br>
> +}<br>
><br>
><br>
> Can the program proceed with invalid VMA? Or it's better to just kill it in this case?<br>
<br>
</div></div>Usually the program will fail to mmap either the shadow/origin/etc, print the maps<br>
and bail out. So I will let to sanitizer error handling to exit if it not able<br>
to run properly.<br></blockquote><div><br></div><div>OK</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
><br>
><br>
> +<br>
> } // namespace __sanitizer<br>
><br>
> #endif // SANITIZER_POSIX<br>
><br>
> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_win.cc<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_win.cc?rev=247413&r1=247412&r2=247413&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_win.cc?rev=247413&r1=247412&r2=247413&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_win.cc (original)<br>
> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_win.cc Fri Sep 11 08:55:00 2015<br>
> @@ -755,6 +755,10 @@ uptr ReadLongProcessName(/*out*/char *bu<br>
> return ReadBinaryName(buf, buf_len);<br>
> }<br>
><br>
> +void CheckVMASize() {<br>
> + // Do nothing.<br>
> +}<br>
> +<br>
> } // namespace __sanitizer<br>
><br>
> #endif // _WIN32<br>
><br>
> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.cc<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.cc?rev=247413&r1=247412&r2=247413&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.cc?rev=247413&r1=247412&r2=247413&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.cc (original)<br>
> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.cc Fri Sep 11 08:55:00 2015<br>
> @@ -312,6 +312,9 @@ void Initialize(ThreadState *thr) {<br>
> if (is_initialized)<br>
> return;<br>
> is_initialized = true;<br>
> +<br>
> + CheckVMASize();<br>
><br>
><br>
><br>
> Same here.<br>
><br>
><br>
> +<br>
> // We are not ready to handle interceptors yet.<br>
> ScopedIgnoreInterceptors ignore;<br>
> SanitizerToolName = "ThreadSanitizer";<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
</div></div>> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a> <mailto:<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>><br>
<span class="">> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
><br>
> --<br>
> Alexey Samsonov<br>
</span>> <a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a> <mailto:<a href="mailto:vonosmas@gmail.com">vonosmas@gmail.com</a>><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">Alexey Samsonov<br><a href="mailto:vonosmas@gmail.com" target="_blank">vonosmas@gmail.com</a></div></div>
</div></div>