| Message ID | 20211201152604.3984495-1-elver@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kcov: fix generic Kconfig dependencies if ARCH_WANTS_NO_INSTR | expand |
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 >
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
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 >
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.
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
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.
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>
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!
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 :-)
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
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
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(-)