| Message ID | 20211005105905.1994700-24-elver@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kcsan: Support detecting a subset of missing memory barriers | expand |
On Tue, Oct 05, 2021 at 12:59:05PM +0200, Marco Elver wrote: > Teach objtool to turn instrumentation required for memory barrier > modeling into nops in noinstr text. > > The __tsan_func_entry/exit calls are still emitted by compilers even > with the __no_sanitize_thread attribute. The memory barrier > instrumentation will be inserted explicitly (without compiler help), and > thus needs to also explicitly be removed. How is arm64 and others using kernel/entry + noinstr going to fix this? ISTR they fully rely on the compilers not emitting instrumentation, since they don't have objtool to fix up stray issues like this.
On Tue, Oct 05, 2021 at 04:37PM +0200, Peter Zijlstra wrote: > On Tue, Oct 05, 2021 at 12:59:05PM +0200, Marco Elver wrote: > > Teach objtool to turn instrumentation required for memory barrier > > modeling into nops in noinstr text. > > > > The __tsan_func_entry/exit calls are still emitted by compilers even > > with the __no_sanitize_thread attribute. The memory barrier > > instrumentation will be inserted explicitly (without compiler help), and > > thus needs to also explicitly be removed. > > How is arm64 and others using kernel/entry + noinstr going to fix this? > > ISTR they fully rely on the compilers not emitting instrumentation, > since they don't have objtool to fix up stray issues like this. So this is where I'd like to hear if the approach of: | #if !defined(CONFIG_ARCH_WANTS_NO_INSTR) || defined(CONFIG_STACK_VALIDATION) | ... | #else | #define kcsan_noinstr noinstr | static __always_inline bool within_noinstr(unsigned long ip) | { | return (unsigned long)__noinstr_text_start <= ip && | ip < (unsigned long)__noinstr_text_end; | } | #endif and then (using the !STACK_VALIDATION definitions) | kcsan_noinstr void instrumentation_may_appear_in_noinstr(void) | { | if (within_noinstr(_RET_IP_)) | return; works for the non-x86 arches that select ARCH_WANTS_NO_INSTR. If it doesn't I can easily just remove kcsan_noinstr/within_noinstr, and add a "depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION" to the KCSAN_WEAK_MEMORY option. Looking at a previous discussion [1], however, I was under the impression that this would work. [1] https://lkml.kernel.org/r/CANpmjNMAZiW-Er=2QDgGP+_3hg1LOvPYcbfGSPMv=aR6MVTB-g@mail.gmail.com
On Tue, 5 Oct 2021 at 17:13, Marco Elver <elver@google.com> wrote: > On Tue, Oct 05, 2021 at 04:37PM +0200, Peter Zijlstra wrote: > > On Tue, Oct 05, 2021 at 12:59:05PM +0200, Marco Elver wrote: > > > Teach objtool to turn instrumentation required for memory barrier > > > modeling into nops in noinstr text. > > > > > > The __tsan_func_entry/exit calls are still emitted by compilers even > > > with the __no_sanitize_thread attribute. The memory barrier > > > instrumentation will be inserted explicitly (without compiler help), and > > > thus needs to also explicitly be removed. > > > > How is arm64 and others using kernel/entry + noinstr going to fix this? > > > > ISTR they fully rely on the compilers not emitting instrumentation, > > since they don't have objtool to fix up stray issues like this. > > So this is where I'd like to hear if the approach of: > > | #if !defined(CONFIG_ARCH_WANTS_NO_INSTR) || defined(CONFIG_STACK_VALIDATION) > | ... > | #else > | #define kcsan_noinstr noinstr > | static __always_inline bool within_noinstr(unsigned long ip) > | { > | return (unsigned long)__noinstr_text_start <= ip && > | ip < (unsigned long)__noinstr_text_end; > | } > | #endif > > and then (using the !STACK_VALIDATION definitions) > > | kcsan_noinstr void instrumentation_may_appear_in_noinstr(void) > | { > | if (within_noinstr(_RET_IP_)) > | return; > > works for the non-x86 arches that select ARCH_WANTS_NO_INSTR. > > If it doesn't I can easily just remove kcsan_noinstr/within_noinstr, and > add a "depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION" to the > KCSAN_WEAK_MEMORY option. > > Looking at a previous discussion [1], however, I was under the > impression that this would work. > > [1] https://lkml.kernel.org/r/CANpmjNMAZiW-Er=2QDgGP+_3hg1LOvPYcbfGSPMv=aR6MVTB-g@mail.gmail.com I'll send v2 of this series after 5.16-rc1. So far I think we haven't been able to say the above doesn't work, which means I'll assume it works on non-x86 architectures with ARCH_WANTS_NO_INSTR until we get evidence of the opposite.
On Thu, Nov 11, 2021 at 11:11:00AM +0100, Marco Elver wrote: > On Tue, 5 Oct 2021 at 17:13, Marco Elver <elver@google.com> wrote: > > So this is where I'd like to hear if the approach of: > > > > | #if !defined(CONFIG_ARCH_WANTS_NO_INSTR) || defined(CONFIG_STACK_VALIDATION) > > | ... > > | #else > > | #define kcsan_noinstr noinstr > > | static __always_inline bool within_noinstr(unsigned long ip) > > | { > > | return (unsigned long)__noinstr_text_start <= ip && > > | ip < (unsigned long)__noinstr_text_end; Provided these turn into compile time constants this stands a fair chance of working I suppose. Once this needs data loads things get a *lot* more tricky. > > | } > > | #endif > > > > and then (using the !STACK_VALIDATION definitions) > > > > | kcsan_noinstr void instrumentation_may_appear_in_noinstr(void) > > | { > > | if (within_noinstr(_RET_IP_)) > > | return; > > > > works for the non-x86 arches that select ARCH_WANTS_NO_INSTR. > > > > If it doesn't I can easily just remove kcsan_noinstr/within_noinstr, and > > add a "depends on !ARCH_WANTS_NO_INSTR || STACK_VALIDATION" to the > > KCSAN_WEAK_MEMORY option. > > > > Looking at a previous discussion [1], however, I was under the > > impression that this would work. > > > > [1] https://lkml.kernel.org/r/CANpmjNMAZiW-Er=2QDgGP+_3hg1LOvPYcbfGSPMv=aR6MVTB-g@mail.gmail.com > > I'll send v2 of this series after 5.16-rc1. So far I think we haven't > been able to say the above doesn't work, which means I'll assume it > works on non-x86 architectures with ARCH_WANTS_NO_INSTR until we get > evidence of the opposite. Fair enough.
diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 7e8cd3ba5482..7b694e639164 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -965,6 +965,31 @@ static struct symbol *find_call_destination(struct section *sec, unsigned long o return call_dest; } +static bool should_remove_if_noinstr(const char *name) +{ + /* + * Many compilers cannot disable KCOV with a function attribute so they + * need a little help, NOP out any KCOV calls from noinstr text. + */ + if (!strncmp(name, "__sanitizer_cov_", 16)) + return true; + + /* + * Compilers currently do not remove __tsan_func_entry/exit with the + * __no_sanitize_thread attribute, remove them. Memory barrier + * instrumentation is not emitted by the compiler, but inserted + * explicitly, so we need to also remove them. + */ + if (!strncmp(name, "__tsan_func_", 12) || + !strcmp(name, "__kcsan_mb") || + !strcmp(name, "__kcsan_wmb") || + !strcmp(name, "__kcsan_rmb") || + !strcmp(name, "__kcsan_release")) + return true; + + return false; +} + /* * Find the destination instructions for all calls. */ @@ -1031,13 +1056,8 @@ static int add_call_destinations(struct objtool_file *file) &file->static_call_list); } - /* - * Many compilers cannot disable KCOV with a function attribute - * so they need a little help, NOP out any KCOV calls from noinstr - * text. - */ if (insn->sec->noinstr && - !strncmp(insn->call_dest->name, "__sanitizer_cov_", 16)) { + should_remove_if_noinstr(insn->call_dest->name)) { if (reloc) { reloc->type = R_NONE; elf_write_reloc(file->elf, reloc);
Teach objtool to turn instrumentation required for memory barrier modeling into nops in noinstr text. The __tsan_func_entry/exit calls are still emitted by compilers even with the __no_sanitize_thread attribute. The memory barrier instrumentation will be inserted explicitly (without compiler help), and thus needs to also explicitly be removed. Signed-off-by: Marco Elver <elver@google.com> --- tools/objtool/check.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)