Re: [PATCH 0/9] x86/entry fixes

[Marco Elver]

On Wed, 3 Jun 2020 at 15:32, Marco Elver <elver@xxxxxxxxxx> wrote:
>
> On Wed, 3 Jun 2020 at 14:18, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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