[PATCH] kcov: fix generic Kconfig dependencies if ARCH_WANTS_NO_INSTR

[PATCH] kcov: fix generic Kconfig dependencies if ARCH_WANTS_NO_INSTR

[Marco Elver]

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.

Signed-off-by: Marco Elver <elver@google.com>
---
 arch/x86/Kconfig  | 2 +-
 lib/Kconfig.debug | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 95dd1ee01546..c030b2ee93b3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -78,7 +78,7 @@ config X86
 	select ARCH_HAS_FILTER_PGPROT
 	select ARCH_HAS_FORTIFY_SOURCE
 	select ARCH_HAS_GCOV_PROFILE_ALL
-	select ARCH_HAS_KCOV			if X86_64 && STACK_VALIDATION
+	select ARCH_HAS_KCOV			if X86_64
 	select ARCH_HAS_MEM_ENCRYPT
 	select ARCH_HAS_MEMBARRIER_SYNC_CORE
 	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9ef7ce18b4f5..589c8aaa2d5b 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1977,6 +1977,8 @@ config KCOV
 	bool "Code coverage for fuzzing"
 	depends on ARCH_HAS_KCOV
 	depends on CC_HAS_SANCOV_TRACE_PC || GCC_PLUGINS
+	depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION || \
+		   GCC_VERSION >= 120000 || CLANG_VERSION >= 130000
 	select DEBUG_FS
 	select GCC_PLUGIN_SANCOV if !CC_HAS_SANCOV_TRACE_PC
 	help
-- 
2.34.0.rc2.393.gf8c9666880-goog

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

[Mark Rutland]

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?

Thanks,
Mark.

> 
> Signed-off-by: Marco Elver <elver@google.com>
> ---
>  arch/x86/Kconfig  | 2 +-
>  lib/Kconfig.debug | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 95dd1ee01546..c030b2ee93b3 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -78,7 +78,7 @@ config X86
>  	select ARCH_HAS_FILTER_PGPROT
>  	select ARCH_HAS_FORTIFY_SOURCE
>  	select ARCH_HAS_GCOV_PROFILE_ALL
> -	select ARCH_HAS_KCOV			if X86_64 && STACK_VALIDATION
> +	select ARCH_HAS_KCOV			if X86_64
>  	select ARCH_HAS_MEM_ENCRYPT
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
>  	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 9ef7ce18b4f5..589c8aaa2d5b 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1977,6 +1977,8 @@ config KCOV
>  	bool "Code coverage for fuzzing"
>  	depends on ARCH_HAS_KCOV
>  	depends on CC_HAS_SANCOV_TRACE_PC || GCC_PLUGINS
> +	depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION || \
> +		   GCC_VERSION >= 120000 || CLANG_VERSION >= 130000
>  	select DEBUG_FS
>  	select GCC_PLUGIN_SANCOV if !CC_HAS_SANCOV_TRACE_PC
>  	help
> -- 
> 2.34.0.rc2.393.gf8c9666880-goog
> 

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

[Marco Elver]

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?).

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

Thanks,
-- Marco

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

[Mark Rutland]

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.

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

[Marco Elver]

On Wed, 1 Dec 2021 at 18:46, Mark Rutland <mark.rutland@arm.com> wrote:
[...]
> > > 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.

Agree, it's not an ideal situation. :-/

syzkaller would still work, just not as efficiently. Not sure what's
worse, less efficient fuzzing, or chance of random crashes. In fact,
on syzbot we already had to disable it:
https://github.com/google/syzkaller/blob/61f862782082c777ba335aa4b4b08d4f74d7d86e/dashboard/config/linux/bits/base.yml#L110
https://lore.kernel.org/linux-arm-kernel/20210119130010.GA2338@C02TD0UTHF1T.local/T/#m78fdfcc41ae831f91c93ad5dabe63f7ccfb482f0

So if we ran into issues with KCOV on syzbot for arm64, I'm sure it's
not just us. I can't quite see what the reasons for the crashes are,
but ruling out noinstr vs. KCOV would be a first step.

So I'm inclined to suggest we take this patch now and not wait for GCC
12, given we're already crashing with KCOV and therefore have KCOV
disabled on arm64 syzbot.

I'm still fine waiting, but just wanted to point out you can fuzz
without KCOV. Preferences?

Thanks,
-- Marco

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

[Mark Rutland]

On Wed, Dec 01, 2021 at 07:16:25PM +0100, Marco Elver wrote:
> On Wed, 1 Dec 2021 at 18:46, Mark Rutland <mark.rutland@arm.com> wrote:
> [...]
> > > > 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.
> 
> Agree, it's not an ideal situation. :-/
> 
> syzkaller would still work, just not as efficiently. Not sure what's
> worse, less efficient fuzzing, or chance of random crashes. In fact,
> on syzbot we already had to disable it:
> https://github.com/google/syzkaller/blob/61f862782082c777ba335aa4b4b08d4f74d7d86e/dashboard/config/linux/bits/base.yml#L110
> https://lore.kernel.org/linux-arm-kernel/20210119130010.GA2338@C02TD0UTHF1T.local/T/#m78fdfcc41ae831f91c93ad5dabe63f7ccfb482f0
> 
> So if we ran into issues with KCOV on syzbot for arm64, I'm sure it's
> not just us. I can't quite see what the reasons for the crashes are,
> but ruling out noinstr vs. KCOV would be a first step.
> 
> So I'm inclined to suggest we take this patch now and not wait for GCC
> 12, given we're already crashing with KCOV and therefore have KCOV
> disabled on arm64 syzbot.
> 
> I'm still fine waiting, but just wanted to point out you can fuzz
> without KCOV. Preferences?

If it's not used by Syzbot, that's good enough for me -- I can apply local
hacks to run with KCOV if I want to in the mean time, and I can debug my own
mess if I have to.

So FWIW, for taking that now:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

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

[Nathan Chancellor]

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.
> 
> Signed-off-by: Marco Elver <elver@google.com>

It might have been nice to do a feature check in Kconfig like we do in
compiler-{clang,gcc}.h but I assume it's highly unlikely that the GCC
change would get backported (and it obviously won't for clang because
older versions are not supported) plus the attributes are different
between clang and GCC.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  arch/x86/Kconfig  | 2 +-
>  lib/Kconfig.debug | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 95dd1ee01546..c030b2ee93b3 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -78,7 +78,7 @@ config X86
>  	select ARCH_HAS_FILTER_PGPROT
>  	select ARCH_HAS_FORTIFY_SOURCE
>  	select ARCH_HAS_GCOV_PROFILE_ALL
> -	select ARCH_HAS_KCOV			if X86_64 && STACK_VALIDATION
> +	select ARCH_HAS_KCOV			if X86_64
>  	select ARCH_HAS_MEM_ENCRYPT
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
>  	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 9ef7ce18b4f5..589c8aaa2d5b 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1977,6 +1977,8 @@ config KCOV
>  	bool "Code coverage for fuzzing"
>  	depends on ARCH_HAS_KCOV
>  	depends on CC_HAS_SANCOV_TRACE_PC || GCC_PLUGINS
> +	depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION || \
> +		   GCC_VERSION >= 120000 || CLANG_VERSION >= 130000
>  	select DEBUG_FS
>  	select GCC_PLUGIN_SANCOV if !CC_HAS_SANCOV_TRACE_PC
>  	help
> -- 
> 2.34.0.rc2.393.gf8c9666880-goog
> 

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

[Peter Zijlstra]

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.
> 
> Signed-off-by: Marco Elver <elver@google.com>
> ---
>  arch/x86/Kconfig  | 2 +-
>  lib/Kconfig.debug | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 95dd1ee01546..c030b2ee93b3 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -78,7 +78,7 @@ config X86
>  	select ARCH_HAS_FILTER_PGPROT
>  	select ARCH_HAS_FORTIFY_SOURCE
>  	select ARCH_HAS_GCOV_PROFILE_ALL
> -	select ARCH_HAS_KCOV			if X86_64 && STACK_VALIDATION
> +	select ARCH_HAS_KCOV			if X86_64
>  	select ARCH_HAS_MEM_ENCRYPT
>  	select ARCH_HAS_MEMBARRIER_SYNC_CORE
>  	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 9ef7ce18b4f5..589c8aaa2d5b 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1977,6 +1977,8 @@ config KCOV
>  	bool "Code coverage for fuzzing"
>  	depends on ARCH_HAS_KCOV
>  	depends on CC_HAS_SANCOV_TRACE_PC || GCC_PLUGINS
> +	depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION || \
> +		   GCC_VERSION >= 120000 || CLANG_VERSION >= 130000

Can we write that as something like:

	$(cc-attribute,__no_sanitize_coverage)

instead? Other than that, yes totally.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[Marco Elver]

On Thu, 2 Dec 2021 at 18:30, Peter Zijlstra <peterz@infradead.org> wrote:
>
> 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.
> >
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> >  arch/x86/Kconfig  | 2 +-
> >  lib/Kconfig.debug | 2 ++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 95dd1ee01546..c030b2ee93b3 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -78,7 +78,7 @@ config X86
> >       select ARCH_HAS_FILTER_PGPROT
> >       select ARCH_HAS_FORTIFY_SOURCE
> >       select ARCH_HAS_GCOV_PROFILE_ALL
> > -     select ARCH_HAS_KCOV                    if X86_64 && STACK_VALIDATION
> > +     select ARCH_HAS_KCOV                    if X86_64
> >       select ARCH_HAS_MEM_ENCRYPT
> >       select ARCH_HAS_MEMBARRIER_SYNC_CORE
> >       select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 9ef7ce18b4f5..589c8aaa2d5b 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -1977,6 +1977,8 @@ config KCOV
> >       bool "Code coverage for fuzzing"
> >       depends on ARCH_HAS_KCOV
> >       depends on CC_HAS_SANCOV_TRACE_PC || GCC_PLUGINS
> > +     depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION || \
> > +                GCC_VERSION >= 120000 || CLANG_VERSION >= 130000
>
> Can we write that as something like:
>
>         $(cc-attribute,__no_sanitize_coverage)
>
> instead? Other than that, yes totally.

That'd be nice, but I think we don't have that cc-attribute helper? I
checked how e.g. CC_HAS_NO_PROFILE_FN_ATTR does it, but it won't work
like that because gcc and clang define the attribute differently and
it becomes a mess. That's also what Nathan pointed out here I think:
https://lkml.kernel.org/r/Yaet8x/1WYiADlPh@archlinux-ax161

Let's keep it simple.

> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Thanks!

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

[Peter Zijlstra]

On Thu, Dec 02, 2021 at 06:38:13PM +0100, Marco Elver wrote:
> On Thu, 2 Dec 2021 at 18:30, Peter Zijlstra <peterz@infradead.org> wrote:

> > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > > index 9ef7ce18b4f5..589c8aaa2d5b 100644
> > > --- a/lib/Kconfig.debug
> > > +++ b/lib/Kconfig.debug
> > > @@ -1977,6 +1977,8 @@ config KCOV
> > >       bool "Code coverage for fuzzing"
> > >       depends on ARCH_HAS_KCOV
> > >       depends on CC_HAS_SANCOV_TRACE_PC || GCC_PLUGINS
> > > +     depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION || \
> > > +                GCC_VERSION >= 120000 || CLANG_VERSION >= 130000
> >
> > Can we write that as something like:
> >
> >         $(cc-attribute,__no_sanitize_coverage)
> >
> > instead? Other than that, yes totally.
> 
> That'd be nice, but I think we don't have that cc-attribute helper? I

Nah indeed, I made that up on the spot.

> checked how e.g. CC_HAS_NO_PROFILE_FN_ATTR does it, but it won't work
> like that because gcc and clang define the attribute differently and
> it becomes a mess. That's also what Nathan pointed out here I think:
> https://lkml.kernel.org/r/Yaet8x/1WYiADlPh@archlinux-ax161


Urgh, that's one of them MsgIDs with a '/' in..

/me substitues with %2f and magic...

Hurmph yeah... so if we can somehow do that it would allow back porting
those fixes to older compiler versions and have things magically work.
Not sure how realistic that is, but still.. A well. I'll go do something
useful then :-)

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

[Marco Elver]

On Wed, 1 Dec 2021 at 16:26, Marco Elver <elver@google.com> wrote:
[...]
> 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.
>
> Signed-off-by: Marco Elver <elver@google.com>

I think this is good to pick up. Even though it has an x86 change in
it, I think kcov changes go through -mm. Andrew, x86 maintainers, any
preference?

With the conclusion from [1], I think we decided it's better to take
this now, given we discovered KCOV already appears broken on arm64
(likely due to noinstr) and e.g. syzbot disables it on arm64.

[1] https://lkml.kernel.org/r/Yae+6clmwHox7CHN@FVFF77S0Q05N

Thanks,
-- Marco