* [RFC][PATCH 0/3] objtool: KCOV vs noinstr
@ 2020-06-12 14:30 Peter Zijlstra
2020-06-12 14:30 ` [RFC][PATCH 1/3] objtool: Clean up elf_write() condition Peter Zijlstra
` (3 more replies)
0 siblings, 4 replies; 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
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [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
* [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
* [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 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
* 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 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
* 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
end of thread, other threads:[~2020-06-16 19:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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-15 18:34 ` Matt Helsley
2020-06-15 18:44 ` Peter Zijlstra
2020-06-16 8:32 ` Peter Zijlstra
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
2020-06-12 14:30 ` [RFC][PATCH 3/3] objtool: Fix noinstr vs KCOV Peter Zijlstra
2020-06-15 7:41 ` Dmitry Vyukov
2020-06-13 19:54 ` [RFC][PATCH 0/3] objtool: KCOV vs noinstr Matt Helsley
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.