[RFC][PATCH 0/3] objtool: KCOV vs noinstr

[RFC][PATCH 0/3] objtool: KCOV vs noinstr

[Peter Zijlstra]

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.

[RFC][PATCH 1/3] objtool: Clean up elf_write() condition

[Peter Zijlstra]

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);

[RFC][PATCH 2/3] objtool: Provide elf_write_{insn,reloc}()

[Peter Zijlstra]

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);
 

[RFC][PATCH 3/3] objtool: Fix noinstr vs KCOV

[Peter Zijlstra]

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.
 		 *

Re: [RFC][PATCH 0/3] objtool: KCOV vs noinstr

[Matt Helsley]

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

Re: [RFC][PATCH 3/3] objtool: Fix noinstr vs KCOV

[Dmitry Vyukov]

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.
>                  *
>
>

Re: [RFC][PATCH 1/3] objtool: Clean up elf_write() condition

[Matt Helsley]

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);
> 
> 

Re: [RFC][PATCH 1/3] objtool: Clean up elf_write() condition

[Peter Zijlstra]

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.

Re: [RFC][PATCH 1/3] objtool: Clean up elf_write() condition

[Peter Zijlstra]

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;

Re: [RFC][PATCH 2/3] objtool: Provide elf_write_{insn,reloc}()

[Peter Zijlstra]

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.

Re: [RFC][PATCH 2/3] objtool: Provide elf_write_{insn,reloc}()

[Matt Helsley]

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