* [RFC][PATCH 1/3] objtool: Clean up elf_write() condition
2020-06-12 14:30 [RFC][PATCH 0/3] objtool: KCOV vs noinstr Peter Zijlstra
@ 2020-06-12 14:30 ` Peter Zijlstra
2020-06-15 18:34 ` Matt Helsley
2020-06-12 14:30 ` [RFC][PATCH 2/3] objtool: Provide elf_write_{insn,reloc}() Peter Zijlstra
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-06-12 14:30 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: linux-kernel, x86, dvyukov, elver, andreyknvl, mark.rutland,
mhelsley, rostedt, jthierry, mbenes, peterz
With there being multiple ways to change the ELF data, let's more
concisely track modification.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
tools/objtool/check.c | 2 ++
tools/objtool/elf.c | 8 +++++++-
tools/objtool/elf.h | 3 ++-
3 files changed, 11 insertions(+), 2 deletions(-)
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2779,7 +2779,9 @@ int check(const char *_objname, bool orc
ret = create_orc_sections(&file);
if (ret < 0)
goto out;
+ }
+ if (file.elf->changed) {
ret = elf_write(file.elf);
if (ret < 0)
goto out;
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -746,6 +746,8 @@ struct section *elf_create_section(struc
elf_hash_add(elf->section_hash, &sec->hash, sec->idx);
elf_hash_add(elf->section_name_hash, &sec->name_hash, str_hash(sec->name));
+ elf->changed = true;
+
return sec;
}
@@ -862,7 +864,7 @@ int elf_rebuild_reloc_section(struct sec
return elf_rebuild_rela_section(sec, nr);
}
-int elf_write(const struct elf *elf)
+int elf_write(struct elf *elf)
{
struct section *sec;
Elf_Scn *s;
@@ -879,6 +881,8 @@ int elf_write(const struct elf *elf)
WARN_ELF("gelf_update_shdr");
return -1;
}
+
+ sec->changed = false;
}
}
@@ -891,6 +895,8 @@ int elf_write(const struct elf *elf)
return -1;
}
+ elf->changed = false;
+
return 0;
}
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -79,6 +79,7 @@ struct elf {
Elf *elf;
GElf_Ehdr ehdr;
int fd;
+ bool changed;
char *name;
struct list_head sections;
DECLARE_HASHTABLE(symbol_hash, ELF_HASH_BITS);
@@ -121,7 +122,7 @@ struct elf *elf_open_read(const char *na
struct section *elf_create_section(struct elf *elf, const char *name, size_t entsize, int nr);
struct section *elf_create_reloc_section(struct elf *elf, struct section *base, int reltype);
void elf_add_reloc(struct elf *elf, struct reloc *reloc);
-int elf_write(const struct elf *elf);
+int elf_write(struct elf *elf);
void elf_close(struct elf *elf);
struct section *find_section_by_name(const struct elf *elf, const char *name);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3] objtool: Clean up elf_write() condition
2020-06-12 14:30 ` [RFC][PATCH 1/3] objtool: Clean up elf_write() condition Peter Zijlstra
@ 2020-06-15 18:34 ` Matt Helsley
2020-06-15 18:44 ` Peter Zijlstra
0 siblings, 1 reply; 11+ messages in thread
From: Matt Helsley @ 2020-06-15 18:34 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Josh Poimboeuf, linux-kernel, x86, dvyukov, elver, andreyknvl,
mark.rutland, rostedt, jthierry, mbenes
On Fri, Jun 12, 2020 at 04:30:35PM +0200, Peter Zijlstra wrote:
> With there being multiple ways to change the ELF data, let's more
> concisely track modification.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Would it make sense to set the relocation section's "changed" flag in
addition to the elf struct's changed flag in elf_rebuild_reloc_section()?
Right now I think the code is assuming that it's a newly created section
but it would be more defensive to set it during a rebuild too I think.
Otherwise looks good to me.
> ---
> tools/objtool/check.c | 2 ++
> tools/objtool/elf.c | 8 +++++++-
> tools/objtool/elf.h | 3 ++-
> 3 files changed, 11 insertions(+), 2 deletions(-)
>
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2779,7 +2779,9 @@ int check(const char *_objname, bool orc
> ret = create_orc_sections(&file);
> if (ret < 0)
> goto out;
> + }
>
> + if (file.elf->changed) {
> ret = elf_write(file.elf);
> if (ret < 0)
> goto out;
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -746,6 +746,8 @@ struct section *elf_create_section(struc
> elf_hash_add(elf->section_hash, &sec->hash, sec->idx);
> elf_hash_add(elf->section_name_hash, &sec->name_hash, str_hash(sec->name));
>
> + elf->changed = true;
> +
> return sec;
> }
>
> @@ -862,7 +864,7 @@ int elf_rebuild_reloc_section(struct sec
> return elf_rebuild_rela_section(sec, nr);
> }
>
> -int elf_write(const struct elf *elf)
> +int elf_write(struct elf *elf)
> {
> struct section *sec;
> Elf_Scn *s;
> @@ -879,6 +881,8 @@ int elf_write(const struct elf *elf)
> WARN_ELF("gelf_update_shdr");
> return -1;
> }
> +
> + sec->changed = false;
> }
> }
>
> @@ -891,6 +895,8 @@ int elf_write(const struct elf *elf)
> return -1;
> }
>
> + elf->changed = false;
> +
> return 0;
> }
>
> --- a/tools/objtool/elf.h
> +++ b/tools/objtool/elf.h
> @@ -79,6 +79,7 @@ struct elf {
> Elf *elf;
> GElf_Ehdr ehdr;
> int fd;
> + bool changed;
> char *name;
> struct list_head sections;
> DECLARE_HASHTABLE(symbol_hash, ELF_HASH_BITS);
> @@ -121,7 +122,7 @@ struct elf *elf_open_read(const char *na
> struct section *elf_create_section(struct elf *elf, const char *name, size_t entsize, int nr);
> struct section *elf_create_reloc_section(struct elf *elf, struct section *base, int reltype);
> void elf_add_reloc(struct elf *elf, struct reloc *reloc);
> -int elf_write(const struct elf *elf);
> +int elf_write(struct elf *elf);
> void elf_close(struct elf *elf);
>
> struct section *find_section_by_name(const struct elf *elf, const char *name);
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3] objtool: Clean up elf_write() condition
2020-06-15 18:34 ` Matt Helsley
@ 2020-06-15 18:44 ` Peter Zijlstra
2020-06-16 8:32 ` Peter Zijlstra
0 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-06-15 18:44 UTC (permalink / raw)
To: Matt Helsley, Josh Poimboeuf, linux-kernel, x86, dvyukov, elver,
andreyknvl, mark.rutland, rostedt, jthierry, mbenes
On Mon, Jun 15, 2020 at 11:34:48AM -0700, Matt Helsley wrote:
> On Fri, Jun 12, 2020 at 04:30:35PM +0200, Peter Zijlstra wrote:
> > With there being multiple ways to change the ELF data, let's more
> > concisely track modification.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> Would it make sense to set the relocation section's "changed" flag in
> addition to the elf struct's changed flag in elf_rebuild_reloc_section()?
>
> Right now I think the code is assuming that it's a newly created section
> but it would be more defensive to set it during a rebuild too I think.
Indeed, currently the code assumes (and this is so) elf_rebuild_rela_sections()
is only called on an elf_create_reloc_section() result and thus setting
->changed is superfluous.
But sure, I can certainly set them there too.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 1/3] objtool: Clean up elf_write() condition
2020-06-15 18:44 ` Peter Zijlstra
@ 2020-06-16 8:32 ` Peter Zijlstra
0 siblings, 0 replies; 11+ messages in thread
From: Peter Zijlstra @ 2020-06-16 8:32 UTC (permalink / raw)
To: Matt Helsley, Josh Poimboeuf, linux-kernel, x86, dvyukov, elver,
andreyknvl, mark.rutland, rostedt, jthierry, mbenes
On Mon, Jun 15, 2020 at 08:44:11PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 15, 2020 at 11:34:48AM -0700, Matt Helsley wrote:
> > On Fri, Jun 12, 2020 at 04:30:35PM +0200, Peter Zijlstra wrote:
> > > With there being multiple ways to change the ELF data, let's more
> > > concisely track modification.
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> >
> > Would it make sense to set the relocation section's "changed" flag in
> > addition to the elf struct's changed flag in elf_rebuild_reloc_section()?
> >
> > Right now I think the code is assuming that it's a newly created section
> > but it would be more defensive to set it during a rebuild too I think.
>
> Indeed, currently the code assumes (and this is so) elf_rebuild_rela_sections()
> is only called on an elf_create_reloc_section() result and thus setting
> ->changed is superfluous.
>
> But sure, I can certainly set them there too.
Delta to the patch.
---
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2740,7 +2740,7 @@ int check(const char *_objname, bool orc
objname = _objname;
- file.elf = elf_open_read(objname, orc ? O_RDWR : O_RDONLY);
+ file.elf = elf_open_read(objname, O_RDWR);
if (!file.elf)
return 1;
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -849,11 +849,14 @@ static int elf_rebuild_rela_section(stru
return 0;
}
-int elf_rebuild_reloc_section(struct section *sec)
+int elf_rebuild_reloc_section(struct elf *elf, struct section *sec)
{
struct reloc *reloc;
int nr;
+ sec->changed = true;
+ elf->changed = true;
+
nr = 0;
list_for_each_entry(reloc, &sec->reloc_list, list)
nr++;
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -134,7 +134,7 @@ struct reloc *find_reloc_by_dest(const s
struct reloc *find_reloc_by_dest_range(const struct elf *elf, struct section *sec,
unsigned long offset, unsigned int len);
struct symbol *find_func_containing(struct section *sec, unsigned long offset);
-int elf_rebuild_reloc_section(struct section *sec);
+int elf_rebuild_reloc_section(struct elf *elf, struct section *sec);
#define for_each_sec(file, sec) \
list_for_each_entry(sec, &file->elf->sections, list)
--- a/tools/objtool/orc_gen.c
+++ b/tools/objtool/orc_gen.c
@@ -222,7 +222,7 @@ int create_orc_sections(struct objtool_f
}
}
- if (elf_rebuild_reloc_section(ip_relocsec))
+ if (elf_rebuild_reloc_section(file->elf, ip_relocsec))
return -1;
return 0;
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC][PATCH 2/3] objtool: Provide elf_write_{insn,reloc}()
2020-06-12 14:30 [RFC][PATCH 0/3] objtool: KCOV vs noinstr Peter Zijlstra
2020-06-12 14:30 ` [RFC][PATCH 1/3] objtool: Clean up elf_write() condition Peter Zijlstra
@ 2020-06-12 14:30 ` Peter Zijlstra
2020-06-16 9:12 ` Peter Zijlstra
2020-06-12 14:30 ` [RFC][PATCH 3/3] objtool: Fix noinstr vs KCOV Peter Zijlstra
2020-06-13 19:54 ` [RFC][PATCH 0/3] objtool: KCOV vs noinstr Matt Helsley
3 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-06-12 14:30 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: linux-kernel, x86, dvyukov, elver, andreyknvl, mark.rutland,
mhelsley, rostedt, jthierry, mbenes, peterz
This provides infrastructure to rewrite instructions; this is
immediately useful for helping out with KCOV-vs-noinstr, but will
also come in handy for a bunch of variable sized jump-label patches
that are still on ice.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
tools/objtool/elf.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
tools/objtool/elf.h | 7 ++++++-
2 files changed, 57 insertions(+), 2 deletions(-)
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -562,8 +562,9 @@ static int read_relocs(struct elf *elf)
return -1;
}
- reloc->sym = find_symbol_by_index(elf, symndx);
reloc->sec = sec;
+ reloc->idx = i;
+ reloc->sym = find_symbol_by_index(elf, symndx);
if (!reloc->sym) {
WARN("can't find reloc entry symbol %d for %s",
symndx, sec->name);
@@ -848,6 +849,55 @@ static int elf_rebuild_rela_section(stru
return 0;
}
+
+int elf_write_insn(struct elf *elf, struct section *sec,
+ unsigned long offset, unsigned int len,
+ const char *insn)
+{
+ Elf_Data *data = sec->data;
+
+ if (data->d_type != ELF_T_BYTE || data->d_off) {
+ WARN("write to unexpected data for section: %s", sec->name);
+ return -1;
+ }
+
+ memcpy(data->d_buf + offset, insn, len);
+ elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY);
+
+ sec->changed = true;
+ elf->changed = true;
+
+ return 0;
+}
+
+int elf_write_reloc(struct elf *elf, struct reloc *reloc)
+{
+ struct section *sec = reloc->sec;
+
+ if (sec->sh.sh_type == SHT_REL) {
+ reloc->rel.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
+ reloc->rel.r_offset = reloc->offset;
+
+ if (!gelf_update_rel(sec->data, reloc->idx, &reloc->rel)) {
+ WARN_ELF("gelf_update_rel");
+ return -1;
+ }
+ } else {
+ reloc->rela.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
+ reloc->rela.r_addend = reloc->addend;
+ reloc->rela.r_offset = reloc->offset;
+
+ if (!gelf_update_rela(sec->data, reloc->idx, &reloc->rela)) {
+ WARN_ELF("gelf_update_rela");
+ return -1;
+ }
+ }
+
+ sec->changed = true;
+ elf->changed = true;
+
+ return 0;
+}
int elf_rebuild_reloc_section(struct section *sec)
{
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -67,9 +67,10 @@ struct reloc {
};
struct section *sec;
struct symbol *sym;
- unsigned int type;
unsigned long offset;
+ unsigned int type;
int addend;
+ int idx;
bool jump_table_start;
};
@@ -122,6 +123,10 @@ struct elf *elf_open_read(const char *na
struct section *elf_create_section(struct elf *elf, const char *name, size_t entsize, int nr);
struct section *elf_create_reloc_section(struct elf *elf, struct section *base, int reltype);
void elf_add_reloc(struct elf *elf, struct reloc *reloc);
+int elf_write_insn(struct elf *elf, struct section *sec,
+ unsigned long offset, unsigned int len,
+ const char *insn);
+int elf_write_reloc(struct elf *elf, struct reloc *reloc);
int elf_write(struct elf *elf);
void elf_close(struct elf *elf);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 2/3] objtool: Provide elf_write_{insn,reloc}()
2020-06-12 14:30 ` [RFC][PATCH 2/3] objtool: Provide elf_write_{insn,reloc}() Peter Zijlstra
@ 2020-06-16 9:12 ` Peter Zijlstra
2020-06-16 19:51 ` Matt Helsley
0 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-06-16 9:12 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: linux-kernel, x86, dvyukov, elver, andreyknvl, mark.rutland,
mhelsley, rostedt, jthierry, mbenes
On Fri, Jun 12, 2020 at 04:30:36PM +0200, Peter Zijlstra wrote:
> +int elf_write_insn(struct elf *elf, struct section *sec,
> + unsigned long offset, unsigned int len,
> + const char *insn)
> +{
> + Elf_Data *data = sec->data;
> +
> + if (data->d_type != ELF_T_BYTE || data->d_off) {
> + WARN("write to unexpected data for section: %s", sec->name);
> + return -1;
> + }
> +
> + memcpy(data->d_buf + offset, insn, len);
> + elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY);
> +
> + sec->changed = true;
> + elf->changed = true;
> +
> + return 0;
> +}
> +
> +int elf_write_reloc(struct elf *elf, struct reloc *reloc)
> +{
> + struct section *sec = reloc->sec;
> +
> + if (sec->sh.sh_type == SHT_REL) {
> + reloc->rel.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
> + reloc->rel.r_offset = reloc->offset;
> +
> + if (!gelf_update_rel(sec->data, reloc->idx, &reloc->rel)) {
> + WARN_ELF("gelf_update_rel");
> + return -1;
> + }
> + } else {
> + reloc->rela.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
> + reloc->rela.r_addend = reloc->addend;
> + reloc->rela.r_offset = reloc->offset;
> +
> + if (!gelf_update_rela(sec->data, reloc->idx, &reloc->rela)) {
> + WARN_ELF("gelf_update_rela");
> + return -1;
> + }
> + }
> +
> + sec->changed = true;
> + elf->changed = true;
> +
> + return 0;
> +}
Doing the change Matt asked for #1, I realized that sec->changed is only
required if we need to rewrite the section header, neither of these two
changes requires that, they already mark the elf data dirty so
elf_update() DTRT.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 2/3] objtool: Provide elf_write_{insn,reloc}()
2020-06-16 9:12 ` Peter Zijlstra
@ 2020-06-16 19:51 ` Matt Helsley
0 siblings, 0 replies; 11+ messages in thread
From: Matt Helsley @ 2020-06-16 19:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Josh Poimboeuf, linux-kernel, x86, dvyukov, elver, andreyknvl,
mark.rutland, rostedt, jthierry, mbenes
On Tue, Jun 16, 2020 at 11:12:53AM +0200, Peter Zijlstra wrote:
> On Fri, Jun 12, 2020 at 04:30:36PM +0200, Peter Zijlstra wrote:
> > +int elf_write_insn(struct elf *elf, struct section *sec,
> > + unsigned long offset, unsigned int len,
> > + const char *insn)
> > +{
> > + Elf_Data *data = sec->data;
> > +
> > + if (data->d_type != ELF_T_BYTE || data->d_off) {
> > + WARN("write to unexpected data for section: %s", sec->name);
> > + return -1;
> > + }
> > +
> > + memcpy(data->d_buf + offset, insn, len);
> > + elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY);
> > +
> > + sec->changed = true;
> > + elf->changed = true;
> > +
> > + return 0;
> > +}
> > +
> > +int elf_write_reloc(struct elf *elf, struct reloc *reloc)
> > +{
> > + struct section *sec = reloc->sec;
> > +
> > + if (sec->sh.sh_type == SHT_REL) {
> > + reloc->rel.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
> > + reloc->rel.r_offset = reloc->offset;
> > +
> > + if (!gelf_update_rel(sec->data, reloc->idx, &reloc->rel)) {
> > + WARN_ELF("gelf_update_rel");
> > + return -1;
> > + }
> > + } else {
> > + reloc->rela.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
> > + reloc->rela.r_addend = reloc->addend;
> > + reloc->rela.r_offset = reloc->offset;
> > +
> > + if (!gelf_update_rela(sec->data, reloc->idx, &reloc->rela)) {
> > + WARN_ELF("gelf_update_rela");
> > + return -1;
> > + }
> > + }
> > +
> > + sec->changed = true;
> > + elf->changed = true;
> > +
> > + return 0;
> > +}
>
> Doing the change Matt asked for #1, I realized that sec->changed is only
> required if we need to rewrite the section header, neither of these two
> changes requires that, they already mark the elf data dirty so
> elf_update() DTRT.
This is really useful information.
As long as you're adding the elf->changed flag it might make sense to add this
as a comment in the struct section definition or even rename sec->changed
to reflect this (e.g. sec->shdr_changed).
Cheers,
-Matt Helsley
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC][PATCH 3/3] objtool: Fix noinstr vs KCOV
2020-06-12 14:30 [RFC][PATCH 0/3] objtool: KCOV vs noinstr Peter Zijlstra
2020-06-12 14:30 ` [RFC][PATCH 1/3] objtool: Clean up elf_write() condition Peter Zijlstra
2020-06-12 14:30 ` [RFC][PATCH 2/3] objtool: Provide elf_write_{insn,reloc}() Peter Zijlstra
@ 2020-06-12 14:30 ` Peter Zijlstra
2020-06-15 7:41 ` Dmitry Vyukov
2020-06-13 19:54 ` [RFC][PATCH 0/3] objtool: KCOV vs noinstr Matt Helsley
3 siblings, 1 reply; 11+ messages in thread
From: Peter Zijlstra @ 2020-06-12 14:30 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: linux-kernel, x86, dvyukov, elver, andreyknvl, mark.rutland,
mhelsley, rostedt, jthierry, mbenes, peterz
Since many compilers cannot disable KCOV with a function attribute,
help it to NOP out any __sanitizer_cov_*() calls injected in noinstr
code.
This turns:
12: e8 00 00 00 00 callq 17 <lockdep_hardirqs_on+0x17>
13: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4
into:
12: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
13: R_X86_64_NONE __sanitizer_cov_trace_pc-0x4
Just like recordmcount does.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
tools/objtool/arch.h | 2 ++
tools/objtool/arch/x86/decode.c | 18 ++++++++++++++++++
tools/objtool/arch/x86/include/arch_elf.h | 6 ++++++
tools/objtool/check.c | 19 +++++++++++++++++++
4 files changed, 45 insertions(+)
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -84,4 +84,6 @@ unsigned long arch_jump_destination(stru
unsigned long arch_dest_reloc_offset(int addend);
+const char *arch_nop_insn(int len);
+
#endif /* _ARCH_H */
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -565,3 +565,21 @@ void arch_initial_func_cfi_state(struct
state->regs[16].base = CFI_CFA;
state->regs[16].offset = -8;
}
+
+const char *arch_nop_insn(int len)
+{
+ static const char nops[5][5] = {
+ /* 1 */ { 0x90 },
+ /* 2 */ { 0x66, 0x90 },
+ /* 3 */ { 0x0f, 0x1f, 0x00 },
+ /* 4 */ { 0x0f, 0x1f, 0x40, 0x00 },
+ /* 5 */ { 0x0f, 0x1f, 0x44, 0x00, 0x00 },
+ };
+
+ if (len < 1 || len > 5) {
+ WARN("invalid NOP size: %d\n", len);
+ return NULL;
+ }
+
+ return nops[len-1];
+}
--- /dev/null
+++ b/tools/objtool/arch/x86/include/arch_elf.h
@@ -0,0 +1,6 @@
+#ifndef _OBJTOOL_ARCH_ELF
+#define _OBJTOOL_ARCH_ELF
+
+#define R_NONE R_X86_64_NONE
+
+#endif /* _OBJTOOL_ARCH_ELF */
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -12,6 +12,7 @@
#include "check.h"
#include "special.h"
#include "warn.h"
+#include "arch_elf.h"
#include <linux/hashtable.h>
#include <linux/kernel.h>
@@ -744,6 +745,24 @@ static int add_call_destinations(struct
insn->call_dest = reloc->sym;
/*
+ * 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)) {
+ if (reloc) {
+ reloc->type = R_NONE;
+ elf_write_reloc(file->elf, reloc);
+ }
+
+ elf_write_insn(file->elf, insn->sec,
+ insn->offset, insn->len,
+ arch_nop_insn(insn->len));
+ insn->type = INSN_NOP;
+ }
+
+ /*
* Whatever stack impact regular CALLs have, should be undone
* by the RETURN of the called function.
*
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 3/3] objtool: Fix noinstr vs KCOV
2020-06-12 14:30 ` [RFC][PATCH 3/3] objtool: Fix noinstr vs KCOV Peter Zijlstra
@ 2020-06-15 7:41 ` Dmitry Vyukov
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Vyukov @ 2020-06-15 7:41 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Josh Poimboeuf, LKML, the arch/x86 maintainers, Marco Elver,
Andrey Konovalov, Mark Rutland, mhelsley, Steven Rostedt,
jthierry, mbenes
On Fri, Jun 12, 2020 at 4:37 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Since many compilers cannot disable KCOV with a function attribute,
> help it to NOP out any __sanitizer_cov_*() calls injected in noinstr
> code.
>
> This turns:
>
> 12: e8 00 00 00 00 callq 17 <lockdep_hardirqs_on+0x17>
> 13: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4
>
> into:
>
> 12: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
> 13: R_X86_64_NONE __sanitizer_cov_trace_pc-0x4
>
> Just like recordmcount does.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Dmitry Vyukov <dvyukov@google.com>
Thanks!
> ---
> tools/objtool/arch.h | 2 ++
> tools/objtool/arch/x86/decode.c | 18 ++++++++++++++++++
> tools/objtool/arch/x86/include/arch_elf.h | 6 ++++++
> tools/objtool/check.c | 19 +++++++++++++++++++
> 4 files changed, 45 insertions(+)
>
> --- a/tools/objtool/arch.h
> +++ b/tools/objtool/arch.h
> @@ -84,4 +84,6 @@ unsigned long arch_jump_destination(stru
>
> unsigned long arch_dest_reloc_offset(int addend);
>
> +const char *arch_nop_insn(int len);
> +
> #endif /* _ARCH_H */
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -565,3 +565,21 @@ void arch_initial_func_cfi_state(struct
> state->regs[16].base = CFI_CFA;
> state->regs[16].offset = -8;
> }
> +
> +const char *arch_nop_insn(int len)
> +{
> + static const char nops[5][5] = {
> + /* 1 */ { 0x90 },
> + /* 2 */ { 0x66, 0x90 },
> + /* 3 */ { 0x0f, 0x1f, 0x00 },
> + /* 4 */ { 0x0f, 0x1f, 0x40, 0x00 },
> + /* 5 */ { 0x0f, 0x1f, 0x44, 0x00, 0x00 },
> + };
> +
> + if (len < 1 || len > 5) {
> + WARN("invalid NOP size: %d\n", len);
> + return NULL;
> + }
> +
> + return nops[len-1];
> +}
> --- /dev/null
> +++ b/tools/objtool/arch/x86/include/arch_elf.h
> @@ -0,0 +1,6 @@
> +#ifndef _OBJTOOL_ARCH_ELF
> +#define _OBJTOOL_ARCH_ELF
> +
> +#define R_NONE R_X86_64_NONE
> +
> +#endif /* _OBJTOOL_ARCH_ELF */
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -12,6 +12,7 @@
> #include "check.h"
> #include "special.h"
> #include "warn.h"
> +#include "arch_elf.h"
>
> #include <linux/hashtable.h>
> #include <linux/kernel.h>
> @@ -744,6 +745,24 @@ static int add_call_destinations(struct
> insn->call_dest = reloc->sym;
>
> /*
> + * 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)) {
> + if (reloc) {
> + reloc->type = R_NONE;
> + elf_write_reloc(file->elf, reloc);
> + }
> +
> + elf_write_insn(file->elf, insn->sec,
> + insn->offset, insn->len,
> + arch_nop_insn(insn->len));
> + insn->type = INSN_NOP;
> + }
> +
> + /*
> * Whatever stack impact regular CALLs have, should be undone
> * by the RETURN of the called function.
> *
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH 0/3] objtool: KCOV vs noinstr
2020-06-12 14:30 [RFC][PATCH 0/3] objtool: KCOV vs noinstr Peter Zijlstra
` (2 preceding siblings ...)
2020-06-12 14:30 ` [RFC][PATCH 3/3] objtool: Fix noinstr vs KCOV Peter Zijlstra
@ 2020-06-13 19:54 ` Matt Helsley
3 siblings, 0 replies; 11+ messages in thread
From: Matt Helsley @ 2020-06-13 19:54 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Josh Poimboeuf, linux-kernel, x86, dvyukov, elver, andreyknvl,
mark.rutland, rostedt, jthierry, mbenes
On Fri, Jun 12, 2020 at 04:30:34PM +0200, Peter Zijlstra wrote:
> Hi All,
>
> These patches go on top of objtool/core, although possibly we need them earlier.
>
> In order to solve the KCOV-vs-noinstr situation, we need objtool to rewrite
> calls to __sanitizer_cov_*() into NOPs, similar to what recordmcount does.
>
> I'm hoping the pending objtool-recordmcount patches can also reuse some of this.
This sounds great to me -- I'll have a look through your series and will try
rebasing my work on this.
Cheers,
-Matt Helsley
^ permalink raw reply [flat|nested] 11+ messages in thread