UBSAN + OrcJIT crashes when code is placed at beginning of page

[MatzeB]

UBSAN from LLVM >= 17.x reads 8 bytes in front of a function to look for markers. This fails when a JIT compiler like ORC-Jit places code immediately at the beginning of an mmaped section (so reading 8 byte upfront ends up in unmapped memory and crashes).

See also https://reviews.llvm.org/D148665 and the similar discussion in #65253

Not sure if ORC should/wants to do something generic to avoid users running into this. For the time being I added this workaround to one of our porjects to unblock our codebase: https://github.com/pytorch/pytorch/pull/133623/files

[llvmbot]

@llvm/issue-subscribers-orcjit

Author: Matthias Braun (MatzeB)

UBSAN from LLVM >= 17.x reads 8 bytes in front of a function to look for markers. This fails when a JIT compiler like ORC-Jit places code immediately at the beginning of an `mmap`ed section (so reading 8 byte upfront ends up in unmapped memory and crashes).

See also https://reviews.llvm.org/D148665 and the similar discussion in #65253

Not sure if ORC should/wants to do something generic to avoid users running into this. For the time being I added this workaround to one of our porjects to unblock our codebase: https://github.com/pytorch/pytorch/pull/133623/files

[lhames]

This seems ok for now, but it's brittle -- we could change the segment layout to place text first in the future (and have talked about doing so).

If we're going to keep the change from https://reviews.llvm.org/D148665 then I think we should add the int __sanitize_function_metadata_size() function that I suggested in that thread so that the JIT can probe for the correct amount of space to reserve automatically.

[vitalybuka]

CC @MaskRay

[MaskRay]

tl;dr llvm-project cannot do anything. JIT workload has to be aware.

-fsanitize=function has to use the https://llvm.org/docs/LangRef.html#prefix-data like scheme to work with Intel IBT/Arm BTI and support all architectures. The alternative scheme that detects ENDBR/BTI could be very expensive and require arch-specific code for each supported architecture.

JIT workload has to accept this trade-off or turn off -fsanitize=function. ubsan can use the trap mode with no runtime. We cannot rely on a runtime library function like __sanitize_function_metadata_size.

[lhames]

tl;dr llvm-project cannot do anything. JIT workload has to be aware.

This is not correct -- if nothing else llvm-project could roll back the changes, or we could accept different performance trade-offs (as you note below).

-fsanitize=function has to use the https://llvm.org/docs/LangRef.html#prefix-data like scheme to work with Intel IBT/Arm BTI and support all architectures. The alternative scheme that detects ENDBR/BTI could be very expensive and require arch-specific code for each supported architecture.

JIT workload has to accept this trade-off or turn off -fsanitize=function. ubsan can use the trap mode with no runtime. We cannot rely on a runtime library function like __sanitize_function_metadata_size.

What would prevent us adding a __sanitize_function_metadata_size function to the sanitizer runtime? The JIT could detect its presence, invoke it to find the required padding and adjust its memory allocation accordingly.

[MaskRay]

tl;dr llvm-project cannot do anything. JIT workload has to be aware.

This is not correct -- if nothing else llvm-project could roll back the changes, or we could accept different performance trade-offs (as you note below).

Or remove -fsanitize=function from -fsanitize=undefined.

-fsanitize=function has to use the llvm.org/docs/LangRef.html#prefix-data like scheme to work with Intel IBT/Arm BTI and support all architectures. The alternative scheme that detects ENDBR/BTI could be very expensive and require arch-specific code for each supported architecture.
JIT workload has to accept this trade-off or turn off -fsanitize=function. ubsan can use the trap mode with no runtime. We cannot rely on a runtime library function like __sanitize_function_metadata_size.

What would prevent us adding a __sanitize_function_metadata_size function to the sanitizer runtime? The JIT could detect its presence, invoke it to find the required padding and adjust its memory allocation accordingly.

-fsanitize=function -fsanitize-trap=function doesn't need a runtime library. If the user uses undefined weak __sanitize_function_metadata_size (the object file feature is not supported by COFF/XCOFF), it seems not better than hard coding the value. __sanitize_function_metadata_size isn't better than a magic value as runtime library is orthogonal to compiler instrumentation and may not know the instrumentation requirement. So a future variant that requires more bytes than -fsanitize=function may not be supported by a simple __sanitize_function_metadata_size interface

[lhames]

Or remove -fsanitize=function from -fsanitize=undefined.

That seems like a reasonable solution, especially if it allows us to return to the claim that UBSan doesn't affect memory layout / ABI.

-fsanitize=function -fsanitize-trap=function doesn't need a runtime library. If the user uses undefined weak __sanitize_function_metadata_size (the object file feature is not supported by COFF/XCOFF), it seems not better than hard coding the value. __sanitize_function_metadata_size isn't better than a magic value as runtime library is orthogonal to compiler instrumentation and may not know the instrumentation requirement. So a future variant that requires more bytes than -fsanitize=function may not be supported by a simple __sanitize_function_metadata_size interface

If we do want to keep -fsanitize=function in -fsanitize=undefined I think the best solution would be a pair of symbols: one to indicate that -fsanitize=function was enabled (perhaps there's already a symbol that we can use for this purpose?), and a second with the metadata size. If the first is present then we search for the second. If the second is not found then we assume a large default conservative value (16 bytes? 32? whatever gives us reasonable headroom). I don't think we need undefined weak for this -- the dynamic symbol lookup APIs should give us the behavior that we need -- they all allow for non-existent symbols. The only problem with this scheme would be extant binaries compiled with -fsanitize=function that lack the first symbol, but that doesn't seem like a huge issue.