[PATCH] objtool: Support multiple rodata sections.

[PATCH] objtool: Support multiple rodata sections.

[Allan Xavier]

This commit adds support for processing switch jump tables in objects
with multiple .rodata sections, such as those created when using
-ffunction-sections and -fdata-sections.  Currently, objtool always
looks in .rodata for jump table information which results in many
"sibling call from callable instruction with modified stack frame"
warnings with objects compiled using those flags.

The fix is comprised of three parts:

1. Flagging all .rodata sections when importing elf information for
easier checking later.

2. Keeping a reference to the section each relocation is from in order
to get the list_head for the other relocations in that section.

3. Finding jump tables by following relocations to .rodata sections,
rather than always referencing a single global .rodata section.

The patch has been tested without data sections enabled and no
differences in the resulting orc unwind information were seen.

Note that as objtool adds terminators to end of each .text section the
unwind information generated between a function+data sections build and
a normal build aren't directly comparable. Manual inspection suggests
that objtool is now generating the correct information, or at least
making more of an effort to do so than it did previously.

Signed-off-by: Allan Xavier <allan.x.xavier@oracle.com>
---
 tools/objtool/check.c | 30 ++++++++++++++++++++++++------
 tools/objtool/check.h |  4 ++--
 tools/objtool/elf.c   |  1 +
 tools/objtool/elf.h   |  3 ++-
 4 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 3a31b238f8856..9f13c7a018528 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -813,7 +813,7 @@ static int add_switch_table(struct objtool_file *file, struct instruction *insn,
 	struct symbol *pfunc = insn->func->pfunc;
 	unsigned int prev_offset = 0;
 
-	list_for_each_entry_from(rela, &file->rodata->rela->rela_list, list) {
+	list_for_each_entry_from(rela, &table->rela_sec->rela_list, list) {
 		if (rela == next_table)
 			break;
 
@@ -903,6 +903,7 @@ static struct rela *find_switch_table(struct objtool_file *file,
 {
 	struct rela *text_rela, *rodata_rela;
 	struct instruction *orig_insn = insn;
+	struct section *rodata_sec;
 	unsigned long table_offset;
 
 	/*
@@ -930,10 +931,13 @@ static struct rela *find_switch_table(struct objtool_file *file,
 		/* look for a relocation which references .rodata */
 		text_rela = find_rela_by_dest_range(insn->sec, insn->offset,
 						    insn->len);
-		if (!text_rela || text_rela->sym != file->rodata->sym)
+		if (!text_rela || !text_rela->sym->sec->rodata
+			       || text_rela->sym != text_rela->sym->sec->sym)
 			continue;
 
 		table_offset = text_rela->addend;
+		rodata_sec = text_rela->sym->sec;
+
 		if (text_rela->type == R_X86_64_PC32)
 			table_offset += 4;
 
@@ -941,10 +945,10 @@ static struct rela *find_switch_table(struct objtool_file *file,
 		 * Make sure the .rodata address isn't associated with a
 		 * symbol.  gcc jump tables are anonymous data.
 		 */
-		if (find_symbol_containing(file->rodata, table_offset))
+		if (find_symbol_containing(rodata_sec, table_offset))
 			continue;
 
-		rodata_rela = find_rela_by_dest(file->rodata, table_offset);
+		rodata_rela = find_rela_by_dest(rodata_sec, table_offset);
 		if (rodata_rela) {
 			/*
 			 * Use of RIP-relative switch jumps is quite rare, and
@@ -1029,7 +1033,7 @@ static int add_switch_table_alts(struct objtool_file *file)
 	struct symbol *func;
 	int ret;
 
-	if (!file->rodata || !file->rodata->rela)
+	if (!file->rodata)
 		return 0;
 
 	for_each_sec(file, sec) {
@@ -2133,6 +2137,20 @@ static void cleanup(struct objtool_file *file)
 	elf_close(file->elf);
 }
 
+static bool mark_rodata(struct objtool_file *file)
+{
+	struct section *sec;
+	bool found = false;
+
+	for_each_sec(file, sec)
+		if (strstr(sec->name, ".rodata") == sec->name) {
+			sec->rodata = true;
+			found = true;
+		}
+
+	return found;
+}
+
 int check(const char *_objname, bool orc)
 {
 	struct objtool_file file;
@@ -2147,10 +2165,10 @@ int check(const char *_objname, bool orc)
 	INIT_LIST_HEAD(&file.insn_list);
 	hash_init(file.insn_hash);
 	file.whitelist = find_section_by_name(file.elf, ".discard.func_stack_frame_non_standard");
-	file.rodata = find_section_by_name(file.elf, ".rodata");
 	file.c_file = find_section_by_name(file.elf, ".comment");
 	file.ignore_unreachables = no_unreachable;
 	file.hints = false;
+	file.rodata = mark_rodata(&file);
 
 	arch_initial_func_cfi_state(&initial_func_cfi);
 
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index c6b68fcb926ff..a039521b67530 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -60,8 +60,8 @@ struct objtool_file {
 	struct elf *elf;
 	struct list_head insn_list;
 	DECLARE_HASHTABLE(insn_hash, 16);
-	struct section *rodata, *whitelist;
-	bool ignore_unreachables, c_file, hints;
+	struct section *whitelist;
+	bool ignore_unreachables, c_file, hints, rodata;
 };
 
 int check(const char *objname, bool orc);
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 4e60e105583ee..7241afdecda33 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -364,6 +364,7 @@ static int read_relas(struct elf *elf)
 			rela->offset = rela->rela.r_offset;
 			symndx = GELF_R_SYM(rela->rela.r_info);
 			rela->sym = find_symbol_by_index(elf, symndx);
+			rela->rela_sec = sec;
 			if (!rela->sym) {
 				WARN("can't find rela entry symbol %d for %s",
 				     symndx, sec->name);
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index de5cd2ddded98..bc97ed86b9cd8 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -48,7 +48,7 @@ struct section {
 	char *name;
 	int idx;
 	unsigned int len;
-	bool changed, text;
+	bool changed, text, rodata;
 };
 
 struct symbol {
@@ -68,6 +68,7 @@ struct rela {
 	struct list_head list;
 	struct hlist_node hash;
 	GElf_Rela rela;
+	struct section *rela_sec;
 	struct symbol *sym;
 	unsigned int type;
 	unsigned long offset;
-- 
2.18.0

Re: [PATCH] objtool: Support multiple rodata sections.

[Josh Poimboeuf]

On Wed, Jul 25, 2018 at 03:17:53PM +0100, Allan Xavier wrote:
> This commit adds support for processing switch jump tables in objects
> with multiple .rodata sections, such as those created when using
> -ffunction-sections and -fdata-sections.  Currently, objtool always
> looks in .rodata for jump table information which results in many
> "sibling call from callable instruction with modified stack frame"
> warnings with objects compiled using those flags.

Thanks for the patch!  kpatch-build also uses those flags and has a
similar issue, so it's good this will be useful for multiple users.

> @@ -930,10 +931,13 @@ static struct rela *find_switch_table(struct objtool_file *file,
>  		/* look for a relocation which references .rodata */
>  		text_rela = find_rela_by_dest_range(insn->sec, insn->offset,
>  						    insn->len);
> -		if (!text_rela || text_rela->sym != file->rodata->sym)
> +		if (!text_rela || !text_rela->sym->sec->rodata
> +			       || text_rela->sym != text_rela->sym->sec->sym)
>  			continue;

This last condition is a bit hard to follow.  I think we can just check
that the sym type is STT_SECTION.  Also the formatting deviates a bit
from the kernel style.  How about something like this?

		if (!text_rela || text_rela->sym->type != STT_SECTION ||
		    !text_rela->sym->sec->rodata)
			continue;

> @@ -2133,6 +2137,20 @@ static void cleanup(struct objtool_file *file)
>  	elf_close(file->elf);
>  }
>  
> +static bool mark_rodata(struct objtool_file *file)
> +{
> +	struct section *sec;
> +	bool found = false;
> +
> +	for_each_sec(file, sec)

Please use brackets for the outer loop.

> +		if (strstr(sec->name, ".rodata") == sec->name) {
> +			sec->rodata = true;
> +			found = true;

This check is too broad.  It will also match the following sections:

.rodata.str1.8
.rodata.str1.1
.rela.rodata
.init.rodata

Also, the loop should break once it finds an rodata section.


> +		}
> +
> +	return found;
> +}
> +
>  int check(const char *_objname, bool orc)
>  {
>  	struct objtool_file file;
> @@ -2147,10 +2165,10 @@ int check(const char *_objname, bool orc)
>  	INIT_LIST_HEAD(&file.insn_list);
>  	hash_init(file.insn_hash);
>  	file.whitelist = find_section_by_name(file.elf, ".discard.func_stack_frame_non_standard");
> -	file.rodata = find_section_by_name(file.elf, ".rodata");
>  	file.c_file = find_section_by_name(file.elf, ".comment");
>  	file.ignore_unreachables = no_unreachable;
>  	file.hints = false;
> +	file.rodata = mark_rodata(&file);

I believe it would be cleaner to call mark_rodata() from
decode_sections().  And file.rodata bool should be set from within
mark_rodata().

-- 
Josh

Re: [PATCH] objtool: Support multiple rodata sections.

[Allan Xavier]

Hi Josh,

Thanks for your comments, I've sent a v2 with fixes for what you mentioned, but
also just wanted to respond to some of your comments directly.

On 26/07/18 17:36, Josh Poimboeuf wrote:
> 
>> +		if (strstr(sec->name, ".rodata") == sec->name) {
>> +			sec->rodata = true;
>> +			found = true;
> 
> This check is too broad.  It will also match the following sections:
> 
> .rodata.str1.8
> .rodata.str1.1
> .rela.rodata
> .init.rodata

As written, it doesn't match .rela.rodata or .init.rodata, although you're right
that incorrectly matches .rodata.str1.[18] as well as
.rodata.<funcname>.str1.[18] when data sections are used.

In the v2 I've addressed this by checking for .str1. before the last character
of the section name, I fully admit it's pretty ugly though and the logic could
be relaxed for a cleaner implementation, it depends on how strict you want to be.

Alternatively, it could also be possible to key off whether the rodata section
has any relocations, which has the additional benefit of narrowing down which
rodata sections are likely to contain jump tables.

Let me know what you think of this or if you have any better suggestions.

> Also, the loop should break once it finds an rodata section.

I don't quite understand your point here. If the loop breaks once it has found
an .rodata section it would only ever mark that one section as rodata, ignoring
the rest?

Thanks,
Allan