Re: [PATCH] kcov: fix generic Kconfig dependencies if ARCH_WANTS_NO_INSTR

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

On Wed, Dec 01, 2021 at 05:10:39PM +0100, Marco Elver wrote:
> On Wed, 1 Dec 2021 at 16:57, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Hi Marco,
> >
> > On Wed, Dec 01, 2021 at 04:26:04PM +0100, Marco Elver wrote:
> > > Until recent versions of GCC and Clang, it was not possible to disable
> > > KCOV instrumentation via a function attribute. The relevant function
> > > attribute was introduced in 540540d06e9d9 ("kcov: add
> > > __no_sanitize_coverage to fix noinstr for all architectures").
> > >
> > > x86 was the first architecture to want a working noinstr, and at the
> > > time no compiler support for the attribute existed yet. Therefore,
> > > 0f1441b44e823 ("objtool: Fix noinstr vs KCOV") introduced the ability to
> > > NOP __sanitizer_cov_*() calls in .noinstr.text.
> > >
> > > However, this doesn't work for other architectures like arm64 and s390
> > > that want a working noinstr per ARCH_WANTS_NO_INSTR.
> > >
> > > At the time of 0f1441b44e823, we didn't yet have ARCH_WANTS_NO_INSTR,
> > > but now we can move the Kconfig dependency checks to the generic KCOV
> > > option. KCOV will be available if:
> > >
> > >       - architecture does not care about noinstr, OR
> > >       - we have objtool support (like on x86), OR
> > >       - GCC is 12.0 or newer, OR
> > >       - Clang is 13.0 or newer.
> >
> > I agree this is the right thing to do, but since GCC 12.0 isn't out yet (and
> > only x86 has objtool atm) this will prevent using KCOV with a released GCC on
> > arm64 and s390, which would be unfortunate for Syzkaller.
> >
> > AFAICT the relevant GCC commit is:
> >
> >    https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=cec4d4a6782c9bd8d071839c50a239c49caca689
> >
> > Currently we mostly get away with disabling KCOV for while compilation units,
> > so maybe it's worth waiting for the GCC 12.0 release, and restricting things
> > once that's out?
> 
> An alternative would be to express 'select ARCH_WANTS_NO_INSTR' more
> precisely, say with an override or something. Because as-is,
> ARCH_WANTS_NO_INSTR then doesn't quite reflect reality on arm64
> (yet?).

It's more of a pragmatic thing -- ARCH_WANTS_NO_INSTR does reflect reality, and
we do *want* to enforce that strictly, it's just that we're just struck between
a rock and a hard place where until GCC 12 is released we either:

a) Strictly enforce noinstr, and be sure there aren't any bugs from unexpected
   instrumentation, but we can't test GCC-built kernels under Syzkaller due to
   the lack of KCOV.

b) Don't strictly enforce noinstr, and have the same latent bugs as today (of
   unknown severity), but we can test GCC-built kernels under Syzkaller.

... and since this (currently only affects KCOV, which people only practically
enable for Syzkaller, I think it's ok to wait until GCC 12 is out, so that we
can have the benefit of Sykaller in the mean time, and subsequrntly got for
option (a) and say those people need to use GCC 12+ (and clang 13+).

> But it does look simpler to wait, so I'm fine with that. I leave it to you.

FWIW, for my purposes I'm happy to take this immediately and to have to apply a
local patch to my fuzzing branches until GCC 12 is out, but I assume we'd want
the upstream testing to work in the mean time without requiring additional
patches.

Thanks,
Mark.