[PATCH 0/2] Fix objtool error reporting and handling of very long symbol names

[PATCH 0/2] Fix objtool error reporting and handling of very long symbol names

[Aaron Plattner]

Two patches in this series:

First, when objtool encounters an error, it carefully propagates the return
code all the way up to main(), which proceeds to ignore it and return 0 no
matter what. This can cause problems with objtool to be missed because the
overall build succeeds. This appears to be a regression in commit
b51277eb9775c, which dropped a call to exit(ret) when a subcommand fails.

Fix that by returning the status code from main().


Second, very long symbol names with .cold variants cause objtool to fail.
This is due to using a small max length, which in turn is due to allocating
on the stack. However, there is not actually a requirement to allocate on
the stack in this (user space) code path, and in fact, the code is cleaner
with this fix: MAX_NAME_LEN is gone and the ugly manual NULL termination
is also removed.

The net result is a more capable objtool and slightly cleaner code.


Although this fix technically only applies to drivers that generate
unusually long symbol names, typically due to using C++ (and these cases
only appear to exist outside of the kernel tree so far), I think it's still
worth applying. That's because the net result is a more capable objtool:
one that lacks an arbitrary length limit for symbol names.

For example, Rust support is being added, and drivers will be the first
users of that support. And like C++, Rust also needs to mangle names [1].
So getting rid of the name length constraint is just good hygiene.

[1] https://rust-lang.github.io/rfcs/2603-rust-symbol-name-mangling-v0.html 


Aaron Plattner (2):
  objtool: return the result of objtool_run() so the build fails if
    objtool doesn't work
  objtool: use strndup() to avoid the need for a maximum symbol name
    length

 tools/objtool/elf.c     | 14 ++++++--------
 tools/objtool/objtool.c |  4 +---
 2 files changed, 7 insertions(+), 11 deletions(-)

-- 
2.42.0

[PATCH 1/2] objtool: return the result of objtool_run() so the build fails if objtool doesn't work

[Aaron Plattner]

If objtool runs into a problem that causes it to exit early, the overall
tool still returns a status code of 0, which causes the build to
continue as if nothing went wrong.

Fixes: b51277eb9775 ("objtool: Ditch subcommands")
Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/objtool/objtool.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/objtool/objtool.c b/tools/objtool/objtool.c
index c54f7235c5d94..f40febdd6e36a 100644
--- a/tools/objtool/objtool.c
+++ b/tools/objtool/objtool.c
@@ -146,7 +146,5 @@ int main(int argc, const char **argv)
 	exec_cmd_init("objtool", UNUSED, UNUSED, UNUSED);
 	pager_init(UNUSED);
 
-	objtool_run(argc, argv);
-
-	return 0;
+	return objtool_run(argc, argv);
 }
-- 
2.42.0

[PATCH 2/2] objtool: use strndup() to avoid the need for a maximum symbol name length

[Aaron Plattner]

If one of the symbols processed by read_symbols() happens to have a .cold
variant with a name longer than objtool's MAX_NAME_LEN limit, the build
fails.

Avoid this problem by just using strndup() to copy the parent function's
name, rather than strncpy()ing it onto the stack.

Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 tools/objtool/elf.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 081befa4674b8..3d27983dc908d 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -22,8 +22,6 @@
 #include <objtool/elf.h>
 #include <objtool/warn.h>
 
-#define MAX_NAME_LEN 128
-
 static inline u32 str_hash(const char *str)
 {
 	return jhash(str, strlen(str), 0);
@@ -515,7 +513,7 @@ static int read_symbols(struct elf *elf)
 	/* Create parent/child links for any cold subfunctions */
 	list_for_each_entry(sec, &elf->sections, list) {
 		sec_for_each_sym(sec, sym) {
-			char pname[MAX_NAME_LEN + 1];
+			char *pname;
 			size_t pnamelen;
 			if (sym->type != STT_FUNC)
 				continue;
@@ -531,15 +529,15 @@ static int read_symbols(struct elf *elf)
 				continue;
 
 			pnamelen = coldstr - sym->name;
-			if (pnamelen > MAX_NAME_LEN) {
-				WARN("%s(): parent function name exceeds maximum length of %d characters",
-				     sym->name, MAX_NAME_LEN);
+			pname = strndup(sym->name, pnamelen);
+			if (!pname) {
+				WARN("%s(): failed to allocate memory",
+				     sym->name);
 				return -1;
 			}
 
-			strncpy(pname, sym->name, pnamelen);
-			pname[pnamelen] = '\0';
 			pfunc = find_symbol_by_name(elf, pname);
+			free(pname);
 
 			if (!pfunc) {
 				WARN("%s(): can't find parent function",
-- 
2.42.0

Re: [PATCH 0/2] Fix objtool error reporting and handling of very long symbol names

[Josh Poimboeuf]

On Wed, Oct 04, 2023 at 05:08:17PM -0700, Aaron Plattner wrote:
> Two patches in this series:
> 
> First, when objtool encounters an error, it carefully propagates the return
> code all the way up to main(), which proceeds to ignore it and return 0 no
> matter what. This can cause problems with objtool to be missed because the
> overall build succeeds. This appears to be a regression in commit
> b51277eb9775c, which dropped a call to exit(ret) when a subcommand fails.
> 
> Fix that by returning the status code from main().

Note most objtool errors are currently ignored anyway, due to check()
unconditionally returning 0.  But as the patch mentions, it will indeed
fix early errors.

> Second, very long symbol names with .cold variants cause objtool to fail.
> This is due to using a small max length, which in turn is due to allocating
> on the stack. However, there is not actually a requirement to allocate on
> the stack in this (user space) code path, and in fact, the code is cleaner
> with this fix: MAX_NAME_LEN is gone and the ugly manual NULL termination
> is also removed.

One benefit of the stack array is that reusing it is more performant
than doing a bunch of allocations.  But this is only for cold symbols,
so the slowdown should (hopefully) be negligible.

I'll run it through some testing.  Thanks!

-- 
Josh