Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200603114014.152292216@infradead.org> <20200603120037.GA2570@hirez.programming.kicks-ass.net>
 <20200603120818.GC2627@hirez.programming.kicks-ass.net> <CANpmjNOxLkqh=qpHQjUC_bZ0GCjkoJ4NxF3UuNGKhJSvcjavaA@mail.gmail.com>
 <20200603121815.GC2570@hirez.programming.kicks-ass.net> <CANpmjNPxMo0sNmkbMHmVYn=WJJwtmYR03ZtFDyPhmiMuR1ug=w@mail.gmail.com>
In-Reply-To: <CANpmjNPxMo0sNmkbMHmVYn=WJJwtmYR03ZtFDyPhmiMuR1ug=w@mail.gmail.com>
From:   Marco Elver <elver@google.com>
Date:   Wed, 3 Jun 2020 16:47:54 +0200
Message-ID: <CANpmjNPzmynV2X+e76roUmt_3oq8KDDKyLLsgn__qtAb8i0aXQ@mail.gmail.com>
Subject: Re: [PATCH 0/9] x86/entry fixes
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        kasan-dev <kasan-dev@googlegroups.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Will Deacon <will@kernel.org>,
        Dmitry Vyukov <dvyukov@google.com>,
        Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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