[PATCH 04/14] objtool/x86: Add .tail_call_sites

Peter Zijlstra posted 14 patches 2 months ago
[PATCH 04/14] objtool/x86: Add .tail_call_sites
Posted by Peter Zijlstra 2 months ago
In order to allow rewriting all direct (tail) calls to ENDBR to go +4, also
generate a tail-call sites section.

This must be different from the .call_sites, because call depth tracking
specifically cares about the CALL instruction, but not JMP instructions.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/vmlinux.lds.S           |    9 ++++-
 tools/objtool/check.c                   |   52 +++++++++++++++++++++++++++++---
 tools/objtool/include/objtool/objtool.h |    1 
 tools/objtool/objtool.c                 |    1 
 4 files changed, 58 insertions(+), 5 deletions(-)

--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -285,6 +285,7 @@ SECTIONS
 		*(.return_sites)
 		__return_sites_end = .;
 	}
+#endif
 
 	. = ALIGN(8);
 	.call_sites : AT(ADDR(.call_sites) - LOAD_OFFSET) {
@@ -292,7 +293,13 @@ SECTIONS
 		*(.call_sites)
 		__call_sites_end = .;
 	}
-#endif
+
+	. = ALIGN(8);
+	.tail_call_sites : AT(ADDR(.tail_call_sites) - LOAD_OFFSET) {
+		__tail_call_sites = .;
+		*(.tail_call_sites)
+		__tail_call_sites_end = .;
+	}
 
 #ifdef CONFIG_X86_KERNEL_IBT
 	. = ALIGN(8);
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -893,7 +893,6 @@ static int create_cfi_sections(struct ob
 
 	sec = find_section_by_name(file->elf, ".cfi_sites");
 	if (sec) {
-		INIT_LIST_HEAD(&file->call_list);
 		WARN("file already has .cfi_sites section, skipping");
 		return 0;
 	}
@@ -1018,6 +1017,45 @@ static int create_direct_call_sections(s
 	return 0;
 }
 
+static int create_direct_tail_call_sections(struct objtool_file *file)
+{
+	struct instruction *insn;
+	struct section *sec;
+	int idx;
+
+	sec = find_section_by_name(file->elf, ".tail_call_sites");
+	if (sec) {
+		INIT_LIST_HEAD(&file->tail_call_list);
+		WARN("file already has .tail_call_sites section, skipping");
+		return 0;
+	}
+
+	if (list_empty(&file->tail_call_list))
+		return 0;
+
+	idx = 0;
+	list_for_each_entry(insn, &file->tail_call_list, call_node)
+		idx++;
+
+	sec = elf_create_section_pair(file->elf, ".tail_call_sites",
+				      sizeof(unsigned int), idx, idx);
+	if (!sec)
+		return -1;
+
+	idx = 0;
+	list_for_each_entry(insn, &file->tail_call_list, call_node) {
+
+		if (!elf_init_reloc_text_sym(file->elf, sec,
+					     idx * sizeof(unsigned int), idx,
+					     insn->sec, insn->offset))
+			return -1;
+
+		idx++;
+	}
+
+	return 0;
+}
+
 /*
  * Warnings shouldn't be reported for ignored functions.
  */
@@ -1417,9 +1455,12 @@ static void annotate_call_site(struct ob
 		return;
 	}
 
-	if (insn->type == INSN_CALL && !insn->sec->init &&
-	    !insn->_call_dest->embedded_insn)
-		list_add_tail(&insn->call_node, &file->call_list);
+	if (!insn->sec->init && !insn->_call_dest->embedded_insn) {
+		if (insn->type == INSN_CALL)
+			list_add_tail(&insn->call_node, &file->call_list);
+		else
+			list_add_tail(&insn->call_node, &file->tail_call_list);
+	}
 
 	if (!sibling && dead_end_function(file, sym))
 		insn->dead_end = true;
@@ -4802,6 +4843,9 @@ int check(struct objtool_file *file)
 			ret = create_direct_call_sections(file);
 			if (ret < 0)
 				goto out;
+			ret = create_direct_tail_call_sections(file);
+			if (ret < 0)
+				goto out;
 			warnings += ret;
 		}
 	}
--- a/tools/objtool/include/objtool/objtool.h
+++ b/tools/objtool/include/objtool/objtool.h
@@ -28,6 +28,7 @@ struct objtool_file {
 	struct list_head mcount_loc_list;
 	struct list_head endbr_list;
 	struct list_head call_list;
+	struct list_head tail_call_list;
 	bool ignore_unreachables, hints, rodata;
 
 	unsigned int nr_endbr;
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -106,6 +106,7 @@ struct objtool_file *objtool_open_read(c
 	INIT_LIST_HEAD(&file.mcount_loc_list);
 	INIT_LIST_HEAD(&file.endbr_list);
 	INIT_LIST_HEAD(&file.call_list);
+	INIT_LIST_HEAD(&file.tail_call_list);
 	file.ignore_unreachables = opts.no_unreachable;
 	file.hints = false;
Re: [PATCH 04/14] objtool/x86: Add .tail_call_sites
Posted by Josh Poimboeuf 2 months ago
On Fri, Sep 27, 2024 at 09:49:00PM +0200, Peter Zijlstra wrote:
> @@ -893,7 +893,6 @@ static int create_cfi_sections(struct ob
>  
>  	sec = find_section_by_name(file->elf, ".cfi_sites");
>  	if (sec) {
> -		INIT_LIST_HEAD(&file->call_list);

Hm, why exactly are we re-initing the list head anyway in these
boilerplate create_*_sections() functions?  I'm guessing that backfired
here.  I can't figure out a reason why we'd doing that anyway.

I'm also wondering why we made these boilerplate function names plural
"sections" when they only create a single section :-)

-- 
Josh
Re: [PATCH 04/14] objtool/x86: Add .tail_call_sites
Posted by Peter Zijlstra 1 month, 2 weeks ago
On Fri, Sep 27, 2024 at 04:42:47PM -0700, Josh Poimboeuf wrote:
> On Fri, Sep 27, 2024 at 09:49:00PM +0200, Peter Zijlstra wrote:
> > @@ -893,7 +893,6 @@ static int create_cfi_sections(struct ob
> >  
> >  	sec = find_section_by_name(file->elf, ".cfi_sites");
> >  	if (sec) {
> > -		INIT_LIST_HEAD(&file->call_list);
> 
> Hm, why exactly are we re-initing the list head anyway in these
> boilerplate create_*_sections() functions?  I'm guessing that backfired
> here.  I can't figure out a reason why we'd doing that anyway.

Yeah, I can't remember where that came from, nor why I removed this
particular one :/

> I'm also wondering why we made these boilerplate function names plural
> "sections" when they only create a single section :-)

Because elf_create_section_pair() creates two section_s_, right?
Re: [PATCH 04/14] objtool/x86: Add .tail_call_sites
Posted by Josh Poimboeuf 1 month, 2 weeks ago
On Wed, Oct 09, 2024 at 05:25:03PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 27, 2024 at 04:42:47PM -0700, Josh Poimboeuf wrote:
> > On Fri, Sep 27, 2024 at 09:49:00PM +0200, Peter Zijlstra wrote:
> > > @@ -893,7 +893,6 @@ static int create_cfi_sections(struct ob
> > >  
> > >  	sec = find_section_by_name(file->elf, ".cfi_sites");
> > >  	if (sec) {
> > > -		INIT_LIST_HEAD(&file->call_list);
> > 
> > Hm, why exactly are we re-initing the list head anyway in these
> > boilerplate create_*_sections() functions?  I'm guessing that backfired
> > here.  I can't figure out a reason why we'd doing that anyway.
> 
> Yeah, I can't remember where that came from, nor why I removed this
> particular one :/

I think you removed this one because file->call_list is also used by
create_direct_call_sections() so it's not wise to clear it if it was
already initalized and needed for another purpose.

> 
> > I'm also wondering why we made these boilerplate function names plural
> > "sections" when they only create a single section :-)
> 
> Because elf_create_section_pair() creates two section_s_, right?

Ah right.

-- 
Josh