Re: Linux Plumbers Conf 2021

[Mark Rutland <mark.rutland@arm.com>]

On Wed, May 19, 2021 at 12:19:38PM +0200, Marco Elver wrote:
> On Wed, 19 May 2021 at 11:32, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Tue, May 18, 2021 at 10:03:06PM +0200, Peter Zijlstra wrote:
> > > On Tue, May 18, 2021 at 09:57:26AM -0700, Nick Desaulniers wrote:
> > > > On Tue, May 18, 2021 at 6:23 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > > >  - validates noinstr annotation; *HOWEVER* we rely on objtool to NOP
> > > > >    all __sanitizer_cov_* calls in .noinstr/.entry text sections because
> > > > >    __no_sanitize_cov is 'broken' in all known compilers.
> > > >
> > > > ^ Do you have more information on this?
> > >
> > > Random link in the middle of a relevant thread:
> > >
> > >   https://lkml.kernel.org/r/CANpmjNPNa2f=kAF6c199oYVJ0iSyirQRGxeOBLxa9PmakSXRbA@mail.gmail.com
> > >
> > > Notable quote:
> > >
> > > "It's everywhere. We cannot mark noinstr functions to not be
> > > instrumented by KCOV/-fsanitize-coverage, because no compiler provides
> > > an attribute to do so. GCC doesn't have
> > > __attribute__((no_sanitize_coverage)) and Clang doesn't have
> > > __attribute__((no_sanitize("coverage")), and therefore we can't have
> > > __no_sanitize_coverage."
> 
> Hmm, we really need to add those. I looked at it briefly for Clang,
> and I think it turned out more complicated than I thought and then
> after seeing objtool "solved it", more pressing things came up. :-/
> 
> > Yuck; I wasn't aware of that.
> >
> > This means that arm64's entry code is unsound (along with some other
> > parts) if kcov is enabled.
> 
> We still have "KCOV_INSTRUMENT_file.o := n" (or "KCOV_INSTRUMENT := n"
> for everything in a Makefile) for now. Not great, but for entry code
> it might be good enough.

FWIW, I've prepared some patches to do just that; it requires moving
some functions around so that we don't unnecessarily prevent
instrumentation, but otherwise that does seem to be good enough for what
we have today.

I'll send that out shortly as part of some entry code rework.

> > That *really* needs an attribute, and going forward we shouldn't accept
> > new compiler instrumentation that we can't opt-out of like this.
> 
> Strangely enough, in Clang, there's -fsanitize-coverage-blocklist, but
> that's pretty useless for the kernel. Maintaining a list of functions
> doesn't scale, and we just need an attribute. I'll ping a few of the
> people who know Clang's fsanitize-coverage a bit better.

Great, thanks!

I think that kernel-side we should have a policy going forward that for
new instrumentation it must be possible to opt-out like this (and when
support is added, `noinstr` gets upsated to opt-out). Correspondingly on
the compiler-side, it'd be good if that was seen as a general
requirement for new instrumentation.

Thanks,
Mark.