Messages in this thread | | | From | Marco Elver <> | Date | Wed, 3 Jun 2020 16:47:54 +0200 | Subject | Re: [PATCH 0/9] x86/entry fixes |
| |
On Wed, 3 Jun 2020 at 15:32, Marco Elver <elver@google.com> wrote: > > On Wed, 3 Jun 2020 at 14:18, Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Wed, Jun 03, 2020 at 02:08:57PM +0200, Marco Elver wrote: > > > > > What is the .config you used? I somehow can't reproduce. I've applied > > > the patches on top of -tip/master. > > > > So tip/master, my patches, your patches, this series. > > > > $ make CC=/opt/llvm/bin/clang O=defconfig-build/ -j80 -s bzImage > > > > is what I used, with the below config. > > > > Thanks, can reproduce now. So far I haven't found any indication that > there is a missing check in Clang's instrumentation passes somewhere. > I'm a bit suspicious because both Clang and GCC have this behaviour. > I'll continue looking.
This is fun: __always_inline functions inlined into __no_sanitize_undefined *do* get instrumented because apparently UBSan passes must run before the optimizer (before inlining), contrary to what [ATM]SAN instrumentation does. Both GCC and Clang do this.
Some options to fix:
1. Add __no_sanitize_undefined to the problematic __always_inline functions. I don't know if a macro like '#define __always_inline_noinstr __always_inline __no_sanitize_undefined' is useful, but it's not an automatic fix either. This option isn't great, because it doesn't really scale.
2. If you look at the generated code for functions with __ubsan_handle_*, all the calls are actually guarded by a branch. So if we know that there is no UBSan violation in the function, AFAIK we're fine. What are the exact requirements for 'noinstr'? Is it only "do not call anything I didn't tell you to call?" If that's the case, and there is no bug in the function ;-), then for UBSan we're fine. With that in mind, you could whitelist "__ubsan_handle"-prefixed functions in objtool. Given the __always_inline+noinstr+__ubsan_handle case is quite rare, it might be reasonable.
We could try to do better, and make __ubsan_handle_* 'noinstr' by checking if _RET_IP_ is in .noinstr.text and just return. Would that work? But that would only be useful if there is a UBSan bug. It might also slow-down regular UBSan, and if we assume that the __always_inline functions called from noinstr functions that end up with UBSan instrumentation don't have bugs (big assumption), then not much is gained either.
Thoughts?
Thanks, -- Marco
| |