| Message ID | 20211118081027.3175699-24-elver@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kcsan: Support detecting a subset of missing memory barriers | expand |
On Thu, Nov 18, 2021 at 09:10:27AM +0100, Marco Elver wrote: > @@ -1071,12 +1071,7 @@ static void annotate_call_site(struct objtool_file *file, > return; > } > > - /* > - * 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 && sym->kcov) { > + if (insn->sec->noinstr && sym->removable_instr) { > if (reloc) { > reloc->type = R_NONE; > elf_write_reloc(file->elf, reloc); I'd love to have a clearer name than 'removable_instr', though I'm having trouble coming up with something. 'profiling_func'? Profiling isn't really accurate but maybe it gets the point across. I'm definitely open to other suggestions. Also, the above code isn't very self-evident so there still needs to be a comment there, like: /* * Many compilers cannot disable KCOV or sanitizer calls with a * function attribute so they need a little help, NOP out any * such calls from noinstr text. */ > @@ -1991,6 +1986,32 @@ static int read_intra_function_calls(struct objtool_file *file) > return 0; > } > > +static bool is_removable_instr(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; A comment is good here, but the NOP-ing bit seems out of place.
On Fri, 19 Nov 2021 at 21:31, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > + if (insn->sec->noinstr && sym->removable_instr) { > > if (reloc) { > > reloc->type = R_NONE; > > elf_write_reloc(file->elf, reloc); > > I'd love to have a clearer name than 'removable_instr', though I'm > having trouble coming up with something. > > 'profiling_func'? > > Profiling isn't really accurate but maybe it gets the point across. I'm > definitely open to other suggestions. Well, this bit is not true for all "profiling functions" either. It's only true for instrumentation functions that appear in 'noinstr' and that the compiler can't remove on its own, but are valid to remove by objtool in noinstr code, hence 'removable_instr'. I'm really quite indifferent what we call it, so I'll leave you to pick whatever sounds best: -- profiling_func -- nop_profiling_func -- optional_profiling_func -- noinstr_remove -- removable_profiling_func -- noinstr_nop_func -- noinstr_nop -- nop_in_noinstr -- invalid_in_noinstr ? > Also, the above code isn't very self-evident so there still needs to be > a comment there, like: > > /* > * Many compilers cannot disable KCOV or sanitizer calls with a > * function attribute so they need a little help, NOP out any > * such calls from noinstr text. > */ > I'll add it. > > +{ > > + /* > > + * 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; > > A comment is good here, but the NOP-ing bit seems out of place. I'll fix that.
On Fri, Nov 19, 2021 at 12:31PM -0800, Josh Poimboeuf wrote: > On Thu, Nov 18, 2021 at 09:10:27AM +0100, Marco Elver wrote: [...] > > + if (insn->sec->noinstr && sym->removable_instr) { [...] > I'd love to have a clearer name than 'removable_instr', though I'm > having trouble coming up with something. [...] I now have the below as v3 of this patch. The naming isn't entirely obvious, but coming up with a short name for this is tricky, but hopefully the comments make it clear. We can of course still pick another name. Does that look reasonable? Note, I'd like this series to sit in -next for a while (probably from some time next week after sending v3 if there are no further complaints). By default everything will be picked up by the -rcu tree, and we're targeting Linux 5.18. If you feel there might be objtool conflicts coming, this patch could be taken through another tree as there are no hard dependencies, as long as this patch reaches mainline before or with the rest. Thanks, -- Marco ------ >8 ------ From: Marco Elver <elver@google.com> Date: Mon, 9 Aug 2021 12:11:14 +0200 Subject: [PATCH] objtool, kcsan: Remove memory barrier instrumentation from noinstr 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> --- v3: * s/removable_instr/profiling_func/ (suggested by Josh Poimboeuf) * Fix and add more comments. v2: * Rewrite after rebase to v5.16-rc1. --- tools/objtool/check.c | 41 ++++++++++++++++++++++++----- tools/objtool/include/objtool/elf.h | 2 +- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 61dfb66b30b6..a78186c583f4 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1072,11 +1072,11 @@ static void annotate_call_site(struct objtool_file *file, } /* - * Many compilers cannot disable KCOV with a function attribute - * so they need a little help, NOP out any KCOV calls from noinstr - * text. + * Many compilers cannot disable KCOV or sanitizer calls with a function + * attribute so they need a little help, NOP out any such calls from + * noinstr text. */ - if (insn->sec->noinstr && sym->kcov) { + if (insn->sec->noinstr && sym->profiling_func) { if (reloc) { reloc->type = R_NONE; elf_write_reloc(file->elf, reloc); @@ -1991,6 +1991,35 @@ static int read_intra_function_calls(struct objtool_file *file) return 0; } +/* + * Return true if name matches an instrumentation function, where calls to that + * function from noinstr code can safely be removed, but compilers won't do so. + */ +static bool is_profiling_func(const char *name) +{ + /* + * Many compilers cannot disable KCOV with a function attribute. + */ + 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; +} + static int classify_symbols(struct objtool_file *file) { struct section *sec; @@ -2011,8 +2040,8 @@ static int classify_symbols(struct objtool_file *file) if (!strcmp(func->name, "__fentry__")) func->fentry = true; - if (!strncmp(func->name, "__sanitizer_cov_", 16)) - func->kcov = true; + if (is_profiling_func(func->name)) + func->profiling_func = true; } } diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h index cdc739fa9a6f..d22336781401 100644 --- a/tools/objtool/include/objtool/elf.h +++ b/tools/objtool/include/objtool/elf.h @@ -58,7 +58,7 @@ struct symbol { u8 static_call_tramp : 1; u8 retpoline_thunk : 1; u8 fentry : 1; - u8 kcov : 1; + u8 profiling_func : 1; struct list_head pv_target; };
On Tue, Nov 23, 2021 at 12:29:39PM +0100, Marco Elver wrote: > On Fri, Nov 19, 2021 at 12:31PM -0800, Josh Poimboeuf wrote: > > On Thu, Nov 18, 2021 at 09:10:27AM +0100, Marco Elver wrote: > [...] > > > + if (insn->sec->noinstr && sym->removable_instr) { > [...] > > I'd love to have a clearer name than 'removable_instr', though I'm > > having trouble coming up with something. > [...] > > I now have the below as v3 of this patch. The naming isn't entirely > obvious, but coming up with a short name for this is tricky, but > hopefully the comments make it clear. We can of course still pick > another name. > > Does that look reasonable? > > Note, I'd like this series to sit in -next for a while (probably from > some time next week after sending v3 if there are no further > complaints). By default everything will be picked up by the -rcu tree, > and we're targeting Linux 5.18. > > If you feel there might be objtool conflicts coming, this patch could be > taken through another tree as there are no hard dependencies, as long as > this patch reaches mainline before or with the rest. > > Thanks, > -- Marco Looks good to me. I don't know of any upcoming conflicts, feel free to carry it with your series for now. Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 61dfb66b30b6..2b2587e5ec69 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1071,12 +1071,7 @@ static void annotate_call_site(struct objtool_file *file, return; } - /* - * 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 && sym->kcov) { + if (insn->sec->noinstr && sym->removable_instr) { if (reloc) { reloc->type = R_NONE; elf_write_reloc(file->elf, reloc); @@ -1991,6 +1986,32 @@ static int read_intra_function_calls(struct objtool_file *file) return 0; } +static bool is_removable_instr(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; +} + static int classify_symbols(struct objtool_file *file) { struct section *sec; @@ -2011,8 +2032,8 @@ static int classify_symbols(struct objtool_file *file) if (!strcmp(func->name, "__fentry__")) func->fentry = true; - if (!strncmp(func->name, "__sanitizer_cov_", 16)) - func->kcov = true; + if (is_removable_instr(func->name)) + func->removable_instr = true; } } diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h index cdc739fa9a6f..62e790a09ad2 100644 --- a/tools/objtool/include/objtool/elf.h +++ b/tools/objtool/include/objtool/elf.h @@ -58,7 +58,7 @@ struct symbol { u8 static_call_tramp : 1; u8 retpoline_thunk : 1; u8 fentry : 1; - u8 kcov : 1; + u8 removable_instr : 1; struct list_head pv_target; };
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> --- v2: * Rewrite after rebase to v5.16-rc1. --- tools/objtool/check.c | 37 ++++++++++++++++++++++------- tools/objtool/include/objtool/elf.h | 2 +- 2 files changed, 30 insertions(+), 9 deletions(-)