[PATCH v5 00/11] perf symbol: Properly seto DSO binary/symtab types

[PATCH v5 00/11] perf symbol: Properly seto DSO binary/symtab types

[Namhyung Kim]

Hello,

Currently we have 3 types for a DSO.

* binary type: determines where to read binary instruction or data of the
  DSO.  Basically used by annotate.  Some (special) DSOs like kallsyms or
  JIT may not have the data though.

* symtab type: determines where to read symbol tables.  Set when symbols
  in the DSO are loaded.

* debug info type: determines where to read debug info.  Used by srcline,
  DWARF unwinding, data type profiling and so on.  Often debug info is
  saved in a separate file.

Some DSOs may have the same type for 3 and others can have all different.
Previously it generally assumed binary type would be same as symtab type
but now I think we should not and only set the binary type as it's used.

Also it should make sure that symtab type is set properly after loading
symbols in the DSO.

Probably this patchset won't make any practical difference in runtime
behaviors but I think it's good to track them accurately.

v5 changes)

 * reset errno before dso__get_filename()
 * check ELF section type being SHT_PROGBITS

v4: https://lore.kernel.org/r/20261012234654.60666-1-namhyung@kernel.org

 * add Ian's Reviewed-by tags
 * make sure to grab _dso__data_open_lock only once
 * check open file count before closing the cache
 * clear errno so that it won't have a stale value

v3: https://lore.kernel.org/r/20261012052601.13920-1-namhyung@kernel.org

 * set KALLSYM type for split kallsyms DSOs
 * take dso lock in dso__find_binary_type()
 * handle EMFILE failures in dso__find_binary_type()
 * drop the libbfd change as it need a separate patchset
 * lookup binary type in dso__e_machine_endian()
 * get rid of machine_or_dso_e_machine() due to a lock problem
 * fixed a bug symbol__disassemble() for BPF and kcore

v2: https://lore.kernel.org/r/20261006234315.920817-1-namhyung@kernel.org

 * add dso__find_binary_type() for checking ".text" section
 * make perf annotate use the dso's binary type directly

v1: https://lore.kernel.org/r/20261005063411.114244-1-namhyung@kernel.org

Thanks,
Namhyung


Namhyung Kim (11):
  perf tools: Remove redundant dso data init
  perf tools: Check ELF section type with name
  perf tools: Try linked debug files for DSO debug info
  perf symbol: Update symtab type of vmlinux from build-id cache
  perf symbol: Set binary/symtab type for split kallsyms
  perf symbol: Get rid of machine_or_dso_e_machine()
  perf tools: Factor out dso__find_binary_type()
  perf symbol: Set binary type for JIT map DSOs
  perf symbol: Do not set binary type from symtab type
  perf annotate: Simplify symbol__disassemble() with dso binary_type
  perf test: Fix dso data tests

 tools/perf/tests/dso-data.c  |   7 ++
 tools/perf/util/disasm.c     | 124 ++++++------------------------
 tools/perf/util/dso.c        | 144 +++++++++++++++++++++++++++++++----
 tools/perf/util/dso.h        |  13 ++++
 tools/perf/util/symbol-elf.c |   3 +-
 tools/perf/util/symbol.c     |  50 ++++--------
 6 files changed, 190 insertions(+), 151 deletions(-)

-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 01/11] perf tools: Remove redundant dso data init

[Namhyung Kim]

The fd and status are initialized later with cache.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index d3017c82ffb5f4b1..df03f84c9723560e 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1722,8 +1722,6 @@ struct dso *dso__new_id(const char *name, const struct dso_id *id)
 		dso->srclines = RB_ROOT_CACHED;
 		dso->data_types = RB_ROOT;
 		dso->global_vars = RB_ROOT;
-		dso->data.fd = -1;
-		dso->data.status = DSO_DATA_STATUS_UNKNOWN;
 		dso->symtab_type = DSO_BINARY_TYPE__NOT_FOUND;
 		dso->binary_type = DSO_BINARY_TYPE__NOT_FOUND;
 		dso->dbginfo_type = DSO_BINARY_TYPE__NOT_FOUND;
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 02/11] perf tools: Check ELF section type with name

[Namhyung Kim]

In filename__has_section(), it just checks the name of the section but
it's possible that the section header is present without actual data.
In that case, it'll have SHT_NOBITS type and can confuse the result.

Update the function to check the section type as well.  Normally we
want to check if the section content is present in the file so it'd
have SHT_PROGBITS.  All the current callers of the function expect
the section with SHT_PROGBITS type (even for debug info).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol-elf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index e955c3feddcd62d0..1e2716c6af45712a 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -228,7 +228,8 @@ bool filename__has_section(const char *filename, const char *sec)
 	if (gelf_getehdr(elf, &ehdr) == NULL)
 		goto elf_out;
 
-	found = !!elf_section_by_name(elf, &ehdr, &shdr, sec, NULL);
+	found = !!elf_section_by_name(elf, &ehdr, &shdr, sec, NULL) &&
+		shdr.sh_type == SHT_PROGBITS;
 
 elf_out:
 	elf_end(elf);
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 03/11] perf tools: Try linked debug files for DSO debug info

[Namhyung Kim]

The dso__find_dbginfo_type() should try DSO_BINARY_TYPE__DEBUGLINK so it
can follow the .gnu_debuglink and find the debug information in the
separate file.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index df03f84c9723560e..913877b84b2d437e 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -2111,6 +2111,7 @@ void dso__find_dbginfo_type(struct dso *dso)
 		DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO,
 		DSO_BINARY_TYPE__BUILDID_DEBUGINFO,
 		DSO_BINARY_TYPE__SYSTEM_PATH_DSO,
+		DSO_BINARY_TYPE__DEBUGLINK,
 		DSO_BINARY_TYPE__NOT_FOUND,
 	};
 	const enum dso_binary_type *type;
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 04/11] perf symbol: Update symtab type of vmlinux from build-id cache

[Namhyung Kim]

Currently dso__load_vmlinux() sets both binary type and symtab type to
VMLINUX.  To precisely track the symtab type of the kernel dso, update
the type to build-id cache if it's loaded from there.  It won't have
any functional difference though.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 5d98888d068c741a..cf099cbd29f268ec 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2079,7 +2079,7 @@ int dso__load_vmlinux_path(struct dso *dso, struct map *map)
 	if (filename != NULL) {
 		err = dso__load_vmlinux(dso, map, filename, true);
 		if (err > 0)
-			goto out;
+			dso__set_symtab_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
 	}
 out:
 	return err;
@@ -2235,8 +2235,10 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
 		filename = __dso__build_id_filename(dso, NULL, 0, false, false);
 	if (filename != NULL) {
 		err = dso__load_vmlinux(dso, map, filename, true);
-		if (err > 0)
+		if (err > 0) {
+			dso__set_symtab_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
 			return err;
+		}
 	}
 
 	if (!symbol_conf.ignore_vmlinux && vmlinux_path != NULL) {
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 05/11] perf symbol: Set binary/symtab type for split kallsyms

[Namhyung Kim]

When it creates new DSOs for split kallsyms, set the binary and symtab
type to KALLSYMS properly.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index cf099cbd29f268ec..56b8d75966133f87 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1099,6 +1099,13 @@ static int maps__split_kallsyms(struct maps *kmaps, struct dso *dso, u64 delta,
 				return -1;
 
 			dso__set_kernel(ndso, dso__kernel(dso));
+			if (dso__kernel(dso) == DSO_SPACE__KERNEL_GUEST) {
+				dso__set_binary_type(ndso, DSO_BINARY_TYPE__GUEST_KALLSYMS);
+				dso__set_symtab_type(ndso, DSO_BINARY_TYPE__GUEST_KALLSYMS);
+			} else {
+				dso__set_binary_type(ndso, DSO_BINARY_TYPE__KALLSYMS);
+				dso__set_symtab_type(ndso, DSO_BINARY_TYPE__KALLSYMS);
+			}
 			dso__set_loaded(ndso);
 
 			curr_map = map__new2(pos->start, ndso);
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 06/11] perf symbol: Get rid of machine_or_dso_e_machine()

[Namhyung Kim]

The function tries to get e_machine from a DSO (preferred) or env from
the machine.  But currently it's only used for kernel DSO's during the
load which refers to /proc/kallsyms most likely and it doesn't have an
ELF header.

Also machine->env would have proper information about the kernel so no
need to check DSO in this case.  Actually check DSO's e_machine requires
a correct binary type to be set but it'd cause a recursive locking
problem.  I think it's ok to use machine->env info for the kernel.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 56b8d75966133f87..a7afecb5816c25f8 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -967,23 +967,6 @@ static int maps__split_kallsyms_for_kcore(struct maps *kmaps, struct dso *dso)
 	return count;
 }
 
-static uint16_t machine_or_dso_e_machine(struct machine *machine, struct dso *dso)
-{
-	uint16_t e_machine = EM_NONE;
-	/* DSO should be most accurate */
-	if (dso)
-		e_machine = dso__e_machine(dso, machine, /*e_flags=*/NULL);
-
-	if (e_machine != EM_NONE)
-		return e_machine;
-
-	/* Check the global environment next. */
-	if (machine && machine->env && machine->env->e_machine != EM_NONE)
-		return machine->env->e_machine;
-
-	return perf_env__e_machine(machine ? machine->env : NULL, /*e_flags=*/NULL);
-}
-
 /*
  * Split the symbols into maps, making sure there are no overlaps, i.e. the
  * kernel range is broken in several maps, named [kernel].N, as we don't have
@@ -1005,7 +988,7 @@ static int maps__split_kallsyms(struct maps *kmaps, struct dso *dso, u64 delta,
 		return -1;
 
 	machine = maps__machine(kmaps);
-	e_machine = machine_or_dso_e_machine(machine, dso);
+	e_machine = perf_env__e_machine(machine->env, /*e_flags=*/NULL);
 
 	while (next) {
 		char *module;
@@ -1567,7 +1550,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 		free(new_node);
 	}
 
-	if (machine_or_dso_e_machine(machine, dso) == EM_X86_64) {
+	if (perf_env__e_machine(machine->env, /*e_flags=*/NULL) == EM_X86_64) {
 		u64 addr;
 
 		/*
@@ -1855,7 +1838,7 @@ int dso__load(struct dso *dso, struct map *map)
 			ret = dso__load_guest_kernel_sym(dso, map);
 
 		machine = maps__machine(map__kmaps(map));
-		if (machine && machine_or_dso_e_machine(machine, dso) == EM_X86_64)
+		if (perf_env__e_machine(machine->env, /*e_flags=*/NULL) == EM_X86_64)
 			machine__map_x86_64_entry_trampolines(machine, dso);
 		goto out;
 	}
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 07/11] perf tools: Factor out dso__find_binary_type()

[Namhyung Kim]

Separate out the logic to detect binary type of a DSO.  It checks if
the candidate file contains ".text" section.  There could be data-only
DSOs but as our use cases focus on the code (perf annotate or intel
PT), we can ignore them for now.

But it also can fail due to open file limit.  Handle the case by
retrying with _dso__data_open_lock.

Introduce bintype_searched flag and it accessors to prevent repeated
(failed) search.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c | 138 ++++++++++++++++++++++++++++++++++++++----
 tools/perf/util/dso.h |  13 ++++
 2 files changed, 139 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 913877b84b2d437e..47241077ad260744 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -833,34 +833,148 @@ void dso__data_close(struct dso *dso)
 	mutex_unlock(dso__data_open_lock());
 }
 
-static void try_to_open_dso(struct dso *dso, struct machine *machine)
-	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
+static int dso__find_binary_type_locked(struct dso *dso)
+	EXCLUSIVE_LOCKS_REQUIRED(dso__lock(dso))
 {
 	enum dso_binary_type binary_type_data[] = {
 		DSO_BINARY_TYPE__BUILD_ID_CACHE,
 		DSO_BINARY_TYPE__SYSTEM_PATH_DSO,
+		dso__symtab_type(dso),
 		DSO_BINARY_TYPE__NOT_FOUND,
 	};
 	int i = 0;
-	struct dso_data *dso_data = dso__data(dso);
+	char *path;
+	bool found = false;
+	bool decomp = false;
+	int saved_errno;
 
-	if (dso_data->fd >= 0)
-		return;
+	if (dso__bintype_searched(dso))
+		return dso__binary_type(dso);
+	dso__set_bintype_searched(dso);
 
-	if (dso__binary_type(dso) != DSO_BINARY_TYPE__NOT_FOUND) {
-		dso_data->fd = open_dso(dso, machine);
-		goto out;
+	switch (dso__binary_type(dso)) {
+	case DSO_BINARY_TYPE__KALLSYMS:
+	case DSO_BINARY_TYPE__GUEST_KALLSYMS:
+	case DSO_BINARY_TYPE__VMLINUX:
+	case DSO_BINARY_TYPE__GUEST_VMLINUX:
+	case DSO_BINARY_TYPE__KCORE:
+	case DSO_BINARY_TYPE__GUEST_KCORE:
+		/* Nothing to do with kernel images */
+		WARN_ON(!dso__kernel(dso));
+		return dso__binary_type(dso);
+	case DSO_BINARY_TYPE__BPF_PROG_INFO:
+	case DSO_BINARY_TYPE__BPF_IMAGE:
+	case DSO_BINARY_TYPE__OOL:
+	case DSO_BINARY_TYPE__JAVA_JIT:
+		/* Same for the special DSOs */
+		return dso__binary_type(dso);
+	case DSO_BINARY_TYPE__GUEST_KMODULE:
+	case DSO_BINARY_TYPE__GUEST_KMODULE_COMP:
+	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
+	case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP:
+	case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
+	case DSO_BINARY_TYPE__BUILD_ID_CACHE:
+		/* We don't expect these are set; fall through */
+	case DSO_BINARY_TYPE__NOT_FOUND:
+		/* Let's find it out (for user DSOs or kernel modules) */
+		break;
+	case DSO_BINARY_TYPE__FEDORA_DEBUGINFO:
+	case DSO_BINARY_TYPE__UBUNTU_DEBUGINFO:
+	case DSO_BINARY_TYPE__MIXEDUP_UBUNTU_DEBUGINFO:
+	case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:
+	case DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO:
+	case DSO_BINARY_TYPE__BUILDID_DEBUGINFO:
+	case DSO_BINARY_TYPE__GNU_DEBUGDATA:
+	case DSO_BINARY_TYPE__DEBUGLINK:
+	default:
+		/* Unexpected binary types (usually debug only) */
+		break;
 	}
 
 	do {
 		dso__set_binary_type(dso, binary_type_data[i++]);
 
-		dso_data->fd = open_dso(dso, machine);
-		if (dso_data->fd >= 0)
-			goto out;
+		errno = 0;
+		path = dso__get_filename(dso, "", &decomp, dso__binary_type(dso));
+		if (path == NULL) {
+			if (errno == EMFILE)
+				goto open_err;
+			continue;
+		}
+
+		errno = 0;
+		found = filename__has_section(path, ".text");
+		if (!found)
+			saved_errno = errno;
+		dso__put_filename(dso, path, decomp);
+
+		if (found)
+			return dso__binary_type(dso);
+		else if (saved_errno == EMFILE)
+			goto open_err;
 
 	} while (dso__binary_type(dso) != DSO_BINARY_TYPE__NOT_FOUND);
-out:
+
+	return dso__binary_type(dso);
+
+open_err:
+	/* Too many open files.  Allow retry after closing one. */
+	RC_CHK_ACCESS(dso)->bintype_searched = 0;
+	dso__set_binary_type(dso, DSO_BINARY_TYPE__NOT_FOUND);
+	return -1;
+}
+
+#define FIND_MAX_RETRY  3
+static int __dso__find_binary_type(struct dso *dso, bool needs_open_lock)
+{
+	int ret;
+	int retry = 0;
+
+again:
+	mutex_lock(dso__lock(dso));
+	ret = dso__find_binary_type_locked(dso);
+	mutex_unlock(dso__lock(dso));
+
+	if (ret == -1 && retry++ < FIND_MAX_RETRY) {
+		if (needs_open_lock && retry == 1)
+			mutex_lock(dso__data_open_lock());
+
+		if (dso__data_open_cnt) {
+			close_first_dso();
+			goto again;
+		}
+	}
+
+	if (retry > 0 && needs_open_lock)
+		mutex_unlock(dso__data_open_lock());
+
+	if (ret == -1) {
+		mutex_lock(dso__lock(dso));
+		ret = dso__binary_type(dso);
+		dso__set_bintype_searched(dso);
+		mutex_unlock(dso__lock(dso));
+	}
+
+	return ret;
+}
+#undef FIND_MAX_RETRY
+
+enum dso_binary_type dso__find_binary_type(struct dso *dso)
+{
+	return __dso__find_binary_type(dso, /*needs_open_lock=*/true);
+}
+
+static void try_to_open_dso(struct dso *dso, struct machine *machine)
+	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
+{
+	struct dso_data *dso_data = dso__data(dso);
+
+	if (dso_data->fd >= 0)
+		return;
+
+	__dso__find_binary_type(dso, /*needs_open_lock=*/false);
+	dso_data->fd = open_dso(dso, machine);
+
 	if (dso_data->fd >= 0)
 		dso_data->status = DSO_DATA_STATUS_OK;
 	else
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 3f08d45e7f53a508..3329f417810c25a4 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -349,6 +349,7 @@ DECLARE_RC_STRUCT(dso) {
 	u8		 short_name_allocated:1;
 	u8		 long_name_allocated:1;
 	u8		 is_64_bit:1;
+	u8		 bintype_searched:1;
 	u8		 debuginfo_searched:1;
 	bool		 sorted_by_name;
 	bool		 loaded;
@@ -957,6 +958,8 @@ void dso__reset_find_symbol_cache(struct dso *dso);
 size_t dso__fprintf_symbols_by_name(struct dso *dso, FILE *fp);
 size_t dso__fprintf(struct dso *dso, FILE *fp);
 
+enum dso_binary_type dso__find_binary_type(struct dso *dso);
+
 static inline enum dso_binary_type dso__binary_type(const struct dso *dso)
 {
 	return RC_CHK_ACCESS(dso)->binary_type;
@@ -967,6 +970,16 @@ static inline void dso__set_binary_type(struct dso *dso, enum dso_binary_type bt
 	RC_CHK_ACCESS(dso)->binary_type = bt;
 }
 
+static inline bool dso__bintype_searched(const struct dso *dso)
+{
+	return RC_CHK_ACCESS(dso)->bintype_searched;
+}
+
+static inline void dso__set_bintype_searched(struct dso *dso)
+{
+	RC_CHK_ACCESS(dso)->bintype_searched = 1;
+}
+
 static inline bool dso__is_vmlinux(const struct dso *dso)
 {
 	enum dso_binary_type bt = dso__binary_type(dso);
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 08/11] perf symbol: Set binary type for JIT map DSOs

[Namhyung Kim]

The perf-<pid>.map files contain symbol name of JIT functions.
As they don't have the binary data (instructions) no need to try
for dso_data.  Let's set the binary type regardless of the result
of symbol loading so that it can be ignored.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index a7afecb5816c25f8..4a8c298039447cec 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1847,6 +1847,7 @@ int dso__load(struct dso *dso, struct map *map)
 
 	if (perfmap) {
 		ret = dso__load_perf_map(map_path, dso);
+		dso__set_binary_type(dso, DSO_BINARY_TYPE__JAVA_JIT);
 		dso__set_symtab_type(dso, ret > 0
 				? DSO_BINARY_TYPE__JAVA_JIT
 				: DSO_BINARY_TYPE__NOT_FOUND);
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 09/11] perf symbol: Do not set binary type from symtab type

[Namhyung Kim]

The DSO binary type is to access the binary (e.g. instruction) data
while symtab type is for the symbol table.  They may or may not be in
the same file.  We should track them separately and correctly.

As special binaries already set their type before loading symbols, leave
the binary type info and only update it when actually opening the DSO.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c    |  3 ++-
 tools/perf/util/symbol.c | 13 -------------
 2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 47241077ad260744..1ca4cc5ec7db027a 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1491,9 +1491,10 @@ uint16_t dso__e_machine_endian(struct dso *dso, struct machine *machine, uint32_
 			       bool *is_big_endian)
 {
 	uint16_t e_machine = EM_NONE;
+	enum dso_binary_type dbt = dso__find_binary_type(dso);
 	int fd;
 
-	switch (dso__binary_type(dso)) {
+	switch (dbt) {
 	case DSO_BINARY_TYPE__KALLSYMS:
 	case DSO_BINARY_TYPE__GUEST_KALLSYMS:
 	case DSO_BINARY_TYPE__VMLINUX:
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 4a8c298039447cec..116870595f75c7dd 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1943,19 +1943,6 @@ int dso__load(struct dso *dso, struct map *map)
 
 		if (next_slot) {
 			ss_pos++;
-
-			/*
-			 * The binary type is used to find the file containing
-			 * the executed instructions, so prefer the types that
-			 * refer to the actual object over debug-only files such
-			 * as DSO_BINARY_TYPE__DEBUGLINK.
-			 */
-			if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND ||
-			    symtab_type == DSO_BINARY_TYPE__BUILD_ID_CACHE ||
-			    (symtab_type == DSO_BINARY_TYPE__SYSTEM_PATH_DSO &&
-			     dso__binary_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE))
-				dso__set_binary_type(dso, symtab_type);
-
 			if (syms_ss && runtime_ss)
 				break;
 		} else {
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 10/11] perf annotate: Simplify symbol__disassemble() with dso binary_type

[Namhyung Kim]

Now it should set the binary type of a DSO properly.  No need to figure
out a filename suitable for disassembly.

Just call dso__find_binary_type() and dso__get_filename() for that type.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/disasm.c | 124 ++++++++-------------------------------
 1 file changed, 24 insertions(+), 100 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 5478c134e7e3b8df..7c289b67b5bde899 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1176,84 +1176,6 @@ int symbol__strerror_disassemble(struct map_symbol *ms, int errnum, char *buf, s
 	return 0;
 }
 
-static int dso__disassemble_filename(struct dso *dso, char *filename, size_t filename_size)
-{
-	char linkname[PATH_MAX];
-	char *build_id_filename;
-	char *build_id_path = NULL;
-	char *pos;
-	int len;
-
-	if (dso__symtab_type(dso) == DSO_BINARY_TYPE__KALLSYMS &&
-	    !dso__is_kcore(dso))
-		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
-
-	build_id_filename = dso__build_id_filename(dso, NULL, 0, false);
-	if (build_id_filename) {
-		/*
-		 * This is a path in perf's own build id cache, not a path on
-		 * the profiled system, so the symfs layout does not apply.
-		 */
-		path__join(filename, filename_size, symbol_conf.symfs,
-			   build_id_filename);
-		free(build_id_filename);
-	} else {
-		if (dso__has_build_id(dso))
-			return ENOMEM;
-		goto fallback;
-	}
-
-	build_id_path = strdup(filename);
-	if (!build_id_path)
-		return ENOMEM;
-
-	/*
-	 * old style build-id cache has name of XX/XXXXXXX.. while
-	 * new style has XX/XXXXXXX../{elf,kallsyms,vdso}.
-	 * extract the build-id part of dirname in the new style only.
-	 */
-	pos = strrchr(build_id_path, '/');
-	if (pos && strlen(pos) < SBUILD_ID_SIZE - 2)
-		dirname(build_id_path);
-
-	if (dso__is_kcore(dso))
-		goto fallback;
-
-	len = readlink(build_id_path, linkname, sizeof(linkname) - 1);
-	if (len < 0)
-		goto fallback;
-
-	linkname[len] = '\0';
-	if (strstr(linkname, DSO__NAME_KALLSYMS) ||
-		access(filename, R_OK)) {
-fallback:
-		/*
-		 * If we don't have build-ids or the build-id file isn't in the
-		 * cache, or is just a kallsyms file, well, lets hope that this
-		 * DSO is the same as when 'perf record' ran.
-		 */
-		if (dso__kernel(dso) && dso__long_name(dso)[0] == '/')
-			snprintf(filename, filename_size, "%s", dso__long_name(dso));
-		else
-			__symbol__join_symfs(filename, filename_size, dso__long_name(dso));
-
-		mutex_lock(dso__lock(dso));
-		if (access(filename, R_OK) && errno == ENOENT && dso__nsinfo(dso)) {
-			char *new_name = dso__filename_with_chroot(dso, filename);
-			if (new_name) {
-				strlcpy(filename, new_name, filename_size);
-				free(new_name);
-			}
-		}
-		mutex_unlock(dso__lock(dso));
-	} else if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
-		dso__set_binary_type(dso, DSO_BINARY_TYPE__BUILD_ID_CACHE);
-	}
-
-	free(build_id_path);
-	return 0;
-}
-
 static int symbol__disassemble_raw(char *filename, struct symbol *sym,
 					struct annotate_args *args)
 {
@@ -1566,14 +1488,29 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
 	struct annotation_options *options = args->options;
 	struct map *map = args->ms->map;
 	struct dso *dso = map__dso(map);
-	char symfs_filename[PATH_MAX];
+	char *symfs_filename;
 	bool delete_extract = false;
 	struct kcore_extract kce;
+	enum dso_binary_type dbt;
 	bool decomp = false;
-	int err = dso__disassemble_filename(dso, symfs_filename, sizeof(symfs_filename));
+	int err;
 
-	if (err)
-		return err;
+	dbt = dso__find_binary_type(dso);
+
+	if (dbt == DSO_BINARY_TYPE__KALLSYMS)
+		return SYMBOL_ANNOTATE_ERRNO__NO_VMLINUX;
+
+	mutex_lock(dso__lock(dso));
+	symfs_filename = dso__get_filename(dso, "", &decomp, dbt);
+	mutex_unlock(dso__lock(dso));
+
+	if (symfs_filename == NULL) {
+		if (dbt == DSO_BINARY_TYPE__BPF_PROG_INFO ||
+		    dbt == DSO_BINARY_TYPE__BPF_IMAGE)
+			symfs_filename = (char *)dso__long_name(dso);
+		else
+			return SYMBOL_ANNOTATE_ERRNO__COULDNT_DETERMINE_FILE_TYPE;
+	}
 
 	pr_debug("%s: filename=%s, sym=%s, start=%#" PRIx64 ", end=%#" PRIx64 "\n", __func__,
 		 symfs_filename, sym->name, map__unmap_ip(map, sym->start),
@@ -1581,9 +1518,7 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
 
 	pr_debug("annotating [%p] %30s : [%p] %30s\n", dso, dso__long_name(dso), sym, sym->name);
 
-	if (dso__binary_type(dso) == DSO_BINARY_TYPE__NOT_FOUND) {
-		return SYMBOL_ANNOTATE_ERRNO__COULDNT_DETERMINE_FILE_TYPE;
-	} else if (dso__is_kcore(dso)) {
+	if (dso__is_kcore(dso)) {
 		kce.addr = map__rip_2objdump(map, sym->start);
 		kce.kcore_filename = symfs_filename;
 		kce.len = sym->end - sym->start;
@@ -1591,18 +1526,8 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
 
 		if (!kcore_extract__create(&kce)) {
 			delete_extract = true;
-			strlcpy(symfs_filename, kce.extract_filename, sizeof(symfs_filename));
-		}
-	} else if (dso__needs_decompress(dso)) {
-		char tmp[KMOD_DECOMP_LEN];
-
-		if (dso__decompress_kmodule_path(dso, symfs_filename, tmp, sizeof(tmp)) < 0)
-			return -1;
-
-		/* empty pathname means file wasn't actually compressed */
-		if (tmp[0] != '\0') {
-			decomp = true;
-			strcpy(symfs_filename, tmp);
+			dso__put_filename(dso, symfs_filename, decomp);
+			symfs_filename = kce.extract_filename;
 		}
 	}
 
@@ -1660,11 +1585,10 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
 			pr_debug("Disassembled with %s\n", perf_disassembler__strs[dis]);
 	}
 out_remove_tmp:
-	if (decomp)
-		unlink(symfs_filename);
-
 	if (delete_extract)
 		kcore_extract__delete(&kce);
+	else if (dbt != DSO_BINARY_TYPE__BPF_PROG_INFO && dbt != DSO_BINARY_TYPE__BPF_IMAGE)
+		dso__put_filename(dso, symfs_filename, decomp);
 
 	return err;
 }
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v5 11/11] perf test: Fix dso data tests

[Namhyung Kim]

Now dso__binary_type() requires that the target file is a valid ELF file
containing ".text" section.  But the test only checks cached read/write
operation on a file.  Let's set the binary type to bypass the ELF check
to read/write the file properly.

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/dso-data.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index 46bc3f59726004fa..18b7052a2dc575c2 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -138,6 +138,10 @@ static int test__dso_data(struct test_suite *test __maybe_unused, int subtest __
 	dsos__init(&machine.dsos);
 
 	dso = dso__new(file);
+	TEST_ASSERT_VAL("Failed to get dso", dso);
+	/* override the binary type for dso data access */
+	dso__set_binary_type(dso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO);
+	dso__set_bintype_searched(dso);
 	TEST_ASSERT_VAL("Failed to add dso", !dsos__add(&machine.dsos, dso));
 	TEST_ASSERT_VAL("Failed to access to dso",
 			dso__data_fd(dso, &machine) >= 0);
@@ -226,6 +230,9 @@ static int dsos__create(int cnt, int size, struct dsos *dsos)
 		TEST_ASSERT_VAL("failed to get dso file", file);
 		dso = dso__new(file);
 		TEST_ASSERT_VAL("failed to get dso", dso);
+		/* override the binary type for dso data access */
+		dso__set_binary_type(dso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO);
+		dso__set_bintype_searched(dso);
 		TEST_ASSERT_VAL("failed to add dso", !dsos__add(dsos, dso));
 		dso__put(dso);
 	}
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH v5 01/11] perf tools: Remove redundant dso data init

[sashiko-bot]

> The fd and status are initialized later with cache.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=1

Re: [PATCH v5 10/11] perf annotate: Simplify symbol__disassemble() with dso binary_type

[sashiko-bot]

> Now it should set the binary type of a DSO properly.  No need to figure
> out a filename suitable for disassembly.
> 
> Just call dso__find_binary_type() and dso__get_filename() for that type.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=10

Re: [PATCH v5 11/11] perf test: Fix dso data tests

[sashiko-bot]

> Now dso__binary_type() requires that the target file is a valid ELF file
> containing ".text" section.  But the test only checks cached read/write
> operation on a file.  Let's set the binary type to bypass the ELF check
> to read/write the file properly.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=11

Re: [PATCH v5 05/11] perf symbol: Set binary/symtab type for split kallsyms

[sashiko-bot]

> When it creates new DSOs for split kallsyms, set the binary and symtab
> type to KALLSYMS properly.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=5

Re: [PATCH v5 07/11] perf tools: Factor out dso__find_binary_type()

[sashiko-bot]

> Separate out the logic to detect binary type of a DSO.  It checks if
> the candidate file contains ".text" section.  There could be data-only
> DSOs but as our use cases focus on the code (perf annotate or intel
> PT), we can ignore them for now.
> 
> But it also can fail due to open file limit.  Handle the case by
> retrying with _dso__data_open_lock.
> 
> Introduce bintype_searched flag and it accessors to prevent repeated
> (failed) search.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=7

Re: [PATCH v5 02/11] perf tools: Check ELF section type with name

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf symbol-elf: uninitialized memory read on gelf_getshdr failure
--

commit f2750bf0f2a1c49778b5a9d0173f02f48eb9e193
Author: Namhyung Kim <namhyung@kernel.org>

perf tools: Check ELF section type with name

This commit updates filename__has_section() to verify that a section
has the SHT_PROGBITS type instead of just checking its name. This prevents
empty sections with SHT_NOBITS from being incorrectly identified as present.

> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -228,7 +228,8 @@ bool filename__has_section(const char *filename, const char *sec)
>  	if (gelf_getehdr(elf, &ehdr) == NULL)
>  		goto elf_out;
>  
> -	found = !!elf_section_by_name(elf, &ehdr, &shdr, sec, NULL);
> +	found = !!elf_section_by_name(elf, &ehdr, &shdr, sec, NULL) &&
> +		shdr.sh_type == SHT_PROGBITS;

[Severity: Medium]
Can this lead to an uninitialized memory read of shdr.sh_type if the ELF
file is malformed?

Looking at elf_section_by_name(), it does not check the return value of
gelf_getshdr():

tools/perf/util/symbol-elf.c:elf_section_by_name() {
    ...
    gelf_getshdr(sec, shp);
    str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
    ...
}

If gelf_getshdr() fails, it returns NULL and leaves the shp struct unmodified.
Because shp points to the stack-allocated shdr in filename__has_section(),
a failure in gelf_getshdr() leaves shdr potentially uninitialized.

Since this patch adds a direct read of shdr.sh_type after elf_section_by_name()
returns, does this new check risk reading uninitialized stack memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=2

Re: [PATCH v5 04/11] perf symbol: Update symtab type of vmlinux from build-id cache

[sashiko-bot]

> Currently dso__load_vmlinux() sets both binary type and symtab type to
> VMLINUX.  To precisely track the symtab type of the kernel dso, update
> the type to build-id cache if it's loaded from there.  It won't have
> any functional difference though.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=4

Re: [PATCH v5 08/11] perf symbol: Set binary type for JIT map DSOs

[sashiko-bot]

> The perf-<pid>.map files contain symbol name of JIT functions.
> As they don't have the binary data (instructions) no need to try
> for dso_data.  Let's set the binary type regardless of the result
> of symbol loading so that it can be ignored.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=8

Re: [PATCH v5 03/11] perf tools: Try linked debug files for DSO debug info

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf tools: DEBUGLINK debuginfo resolution fails for containerized binaries
- [Medium] perf: Uninitialized stack read when parsing malformed .gnu_debuglink
--

commit 58954c9ee50dda18b6aba714c1a631c8845bc3e0
Author: Namhyung Kim <namhyung@kernel.org>

perf tools: Try linked debug files for DSO debug info

The dso__find_dbginfo_type() should try DSO_BINARY_TYPE__DEBUGLINK so it
can follow the .gnu_debuglink and find the debug information in the
separate file.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index df03f84c97235..913877b84b2d4 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -2111,6 +2111,7 @@ void dso__find_dbginfo_type(struct dso *dso)
>  		DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO,
>  		DSO_BINARY_TYPE__BUILDID_DEBUGINFO,
>  		DSO_BINARY_TYPE__SYSTEM_PATH_DSO,
> +		DSO_BINARY_TYPE__DEBUGLINK,

[Severity: Medium]
Will this break debuginfo resolution for containerized binaries?

When DSO_BINARY_TYPE__DEBUGLINK is attempted, dso__get_filename() calls
dso__read_binary_type_filename(). That function performs an early check
on the host namespace:

tools/perf/util/dso.c:dso__read_binary_type_filename() {
	...
		if (!is_regular_file(filename)) {
			ret = -1;
			break;
		}
	...
}

If the binary is inside a mount namespace, is_regular_file() fails and
returns -1. This causes dso__get_filename() to abort early:

tools/perf/util/dso.c:dso__get_filename() {
	...
	if (dso__read_binary_type_filename(dso, type, root_dir, name, PATH_MAX))
		goto out;
	...
}

Will this bypass the subsequent dso__filename_with_chroot() fallback logic,
causing the tool to miss debuginfo for containerized applications?


[Severity: Medium]
Could this lead to an uninitialized stack read with malformed ELF files?

When resolving DSO_BINARY_TYPE__DEBUGLINK, dso__read_binary_type_filename()
allocates an uninitialized stack buffer:

tools/perf/util/dso.c:dso__read_binary_type_filename() {
	...
		char symfile[PATH_MAX];
		...
		ret = filename__read_debuglink(filename, symfile, PATH_MAX);
	...
}

If libbfd handles the read, it copies the section directly:

tools/perf/util/libbfd.c:libbfd_filename__read_debuglink() {
	...
	if (!bfd_get_section_contents(abfd, section, debuglink, 0,
				      section->size))
		goto out_close;
	...
}

Since BFD does not append a null terminator, if the .gnu_debuglink section
is malformed and lacks a null byte, could the subsequent snprintf() in
dso__read_binary_type_filename() read uninitialized stack memory?

>  		DSO_BINARY_TYPE__NOT_FOUND,
>  	};
>  	const enum dso_binary_type *type;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=3

Re: [PATCH v5 09/11] perf symbol: Do not set binary type from symtab type

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf: guest kmodules misidentified as host system path DSOs
--

commit 509bcd616550a20de7fa8d41090d38531f89cf60
Author: Namhyung Kim <namhyung@kernel.org>

perf symbol: Do not set binary type from symtab type

The DSO binary type is used to access the binary data, while the symtab
type is for the symbol table. This commit avoids setting the binary type
from the symtab type when loading symbols, and instead updates the binary
type only when opening the DSO.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 47241077ad260..1ca4cc5ec7db0 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1491,9 +1491,10 @@ uint16_t dso__e_machine_endian(struct dso *dso, struct machine *machine, uint32_
>  			       bool *is_big_endian)
>  {
>  	uint16_t e_machine = EM_NONE;
> +	enum dso_binary_type dbt = dso__find_binary_type(dso);
>  	int fd;
>  
> -	switch (dso__binary_type(dso)) {
> +	switch (dbt) {

[Severity: High]
Will this change cause guest kmodules to be misidentified as host system
path DSOs?

By executing dso__find_binary_type() first, the code evaluates the priority
list in dso__find_binary_type_locked(), which checks
DSO_BINARY_TYPE__SYSTEM_PATH_DSO before falling back to dso__symtab_type(dso).

If the host filesystem happens to contain a module at the exact same absolute
path as the guest kernel module (e.g., /lib/modules/x.y.z/...), the
SYSTEM_PATH_DSO file check will succeed. This causes dso__find_binary_type()
to return SYSTEM_PATH_DSO instead of DSO_BINARY_TYPE__GUEST_KMODULE.

Because dso__e_machine_endian() receives SYSTEM_PATH_DSO, it will fall
through this fast-path switch, open the host file, and return the host's ELF
machine architecture. Could this break cross-platform perf analysis (such as
an x86_64 host analyzing an arm64 guest) by parsing the host's headers
instead of the guest's?

>  	case DSO_BINARY_TYPE__KALLSYMS:
>  	case DSO_BINARY_TYPE__GUEST_KALLSYMS:
>  	case DSO_BINARY_TYPE__VMLINUX:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=9

Re: [PATCH v5 06/11] perf symbol: Get rid of machine_or_dso_e_machine()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf symbol: NULL pointer dereference in dso__load() on error path
--

commit a7899d8622f5885c1647a9ddbd7a9ae67999aeeb
Author: Namhyung Kim <namhyung@kernel.org>

perf symbol: Get rid of machine_or_dso_e_machine()

This commit removes the machine_or_dso_e_machine() fallback logic and relies
directly on perf_env__e_machine() for kernel maps. It aims to prevent
recursive locking issues and assumes kernel DSO architectures are correctly
described by the machine environment.

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 56b8d75966133..a7afecb5816c2 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
[ ... ]
> @@ -1855,7 +1838,7 @@ int dso__load(struct dso *dso, struct map *map)
>  			ret = dso__load_guest_kernel_sym(dso, map);
>  
>  		machine = maps__machine(map__kmaps(map));
> -		if (machine && machine_or_dso_e_machine(machine, dso) == EM_X86_64)
> +		if (perf_env__e_machine(machine->env, /*e_flags=*/NULL) == EM_X86_64)

[Severity: Medium]
Does this code cause a NULL pointer dereference if machine is NULL?

By removing the machine pointer guard, if map__kmaps() returns NULL (for
example, if a kernel DSO is incorrectly associated with a non-kernel map from a
malformed perf.data file), maps__machine() will also return NULL. Passing
machine->env directly to perf_env__e_machine() in this state will then trigger
a segfault.

Could the machine check be restored before attempting to read its environment?

>  			machine__map_x86_64_entry_trampolines(machine, dso);
>  		goto out;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=6

Re: [PATCH v5 02/11] perf tools: Check ELF section type with name

[Namhyung Kim]

On Tue, Oct 13, 2026 at 05:31:14PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] perf symbol-elf: uninitialized memory read on gelf_getshdr failure
> --
> 
> commit f2750bf0f2a1c49778b5a9d0173f02f48eb9e193
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf tools: Check ELF section type with name
> 
> This commit updates filename__has_section() to verify that a section
> has the SHT_PROGBITS type instead of just checking its name. This prevents
> empty sections with SHT_NOBITS from being incorrectly identified as present.
> 
> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > --- a/tools/perf/util/symbol-elf.c
> > +++ b/tools/perf/util/symbol-elf.c
> > @@ -228,7 +228,8 @@ bool filename__has_section(const char *filename, const char *sec)
> >  	if (gelf_getehdr(elf, &ehdr) == NULL)
> >  		goto elf_out;
> >  
> > -	found = !!elf_section_by_name(elf, &ehdr, &shdr, sec, NULL);
> > +	found = !!elf_section_by_name(elf, &ehdr, &shdr, sec, NULL) &&
> > +		shdr.sh_type == SHT_PROGBITS;
> 
> [Severity: Medium]
> Can this lead to an uninitialized memory read of shdr.sh_type if the ELF
> file is malformed?
> 
> Looking at elf_section_by_name(), it does not check the return value of
> gelf_getshdr():
> 
> tools/perf/util/symbol-elf.c:elf_section_by_name() {
>     ...
>     gelf_getshdr(sec, shp);
>     str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
>     ...
> }
> 
> If gelf_getshdr() fails, it returns NULL and leaves the shp struct unmodified.
> Because shp points to the stack-allocated shdr in filename__has_section(),
> a failure in gelf_getshdr() leaves shdr potentially uninitialized.
> 
> Since this patch adds a direct read of shdr.sh_type after elf_section_by_name()
> returns, does this new check risk reading uninitialized stack memory?

Well.. it's already checked shp->sh_name is valid so I think it's ok.

But probably safer to check the return value.  It's also a separate
change though.

Thanks,
Namhyung

Re: [PATCH v5 09/11] perf symbol: Do not set binary type from symtab type

[Namhyung Kim]

On Tue, Oct 13, 2026 at 05:35:27PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] perf: guest kmodules misidentified as host system path DSOs
> --
> 
> commit 509bcd616550a20de7fa8d41090d38531f89cf60
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf symbol: Do not set binary type from symtab type
> 
> The DSO binary type is used to access the binary data, while the symtab
> type is for the symbol table. This commit avoids setting the binary type
> from the symtab type when loading symbols, and instead updates the binary
> type only when opening the DSO.
> 
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 47241077ad260..1ca4cc5ec7db0 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -1491,9 +1491,10 @@ uint16_t dso__e_machine_endian(struct dso *dso, struct machine *machine, uint32_
> >  			       bool *is_big_endian)
> >  {
> >  	uint16_t e_machine = EM_NONE;
> > +	enum dso_binary_type dbt = dso__find_binary_type(dso);
> >  	int fd;
> >  
> > -	switch (dso__binary_type(dso)) {
> > +	switch (dbt) {
> 
> [Severity: High]
> Will this change cause guest kmodules to be misidentified as host system
> path DSOs?
> 
> By executing dso__find_binary_type() first, the code evaluates the priority
> list in dso__find_binary_type_locked(), which checks
> DSO_BINARY_TYPE__SYSTEM_PATH_DSO before falling back to dso__symtab_type(dso).
> 
> If the host filesystem happens to contain a module at the exact same absolute
> path as the guest kernel module (e.g., /lib/modules/x.y.z/...), the
> SYSTEM_PATH_DSO file check will succeed. This causes dso__find_binary_type()
> to return SYSTEM_PATH_DSO instead of DSO_BINARY_TYPE__GUEST_KMODULE.
> 
> Because dso__e_machine_endian() receives SYSTEM_PATH_DSO, it will fall
> through this fast-path switch, open the host file, and return the host's ELF
> machine architecture. Could this break cross-platform perf analysis (such as
> an x86_64 host analyzing an arm64 guest) by parsing the host's headers
> instead of the guest's?

To get a filename, it doesn't really matter if it's SYSTEM_PATH_DSO or
KMODULE (for host and guest).  Also I believe there's no good way to
retrive guest modules from the host other than symfs.

So I think this is fine for now.  Maybe we can work on improving guest
supports later.

Thanks,
Namhyung

Re: [PATCH v5 06/11] perf symbol: Get rid of machine_or_dso_e_machine()

[Namhyung Kim]

On Tue, Oct 13, 2026 at 05:37:49PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] perf symbol: NULL pointer dereference in dso__load() on error path
> --
> 
> commit a7899d8622f5885c1647a9ddbd7a9ae67999aeeb
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf symbol: Get rid of machine_or_dso_e_machine()
> 
> This commit removes the machine_or_dso_e_machine() fallback logic and relies
> directly on perf_env__e_machine() for kernel maps. It aims to prevent
> recursive locking issues and assumes kernel DSO architectures are correctly
> described by the machine environment.
> 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index 56b8d75966133..a7afecb5816c2 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> [ ... ]
> > @@ -1855,7 +1838,7 @@ int dso__load(struct dso *dso, struct map *map)
> >  			ret = dso__load_guest_kernel_sym(dso, map);
> >  
> >  		machine = maps__machine(map__kmaps(map));
> > -		if (machine && machine_or_dso_e_machine(machine, dso) == EM_X86_64)
> > +		if (perf_env__e_machine(machine->env, /*e_flags=*/NULL) == EM_X86_64)
> 
> [Severity: Medium]
> Does this code cause a NULL pointer dereference if machine is NULL?
> 
> By removing the machine pointer guard, if map__kmaps() returns NULL (for
> example, if a kernel DSO is incorrectly associated with a non-kernel map from a
> malformed perf.data file), maps__machine() will also return NULL. Passing
> machine->env directly to perf_env__e_machine() in this state will then trigger
> a segfault.
> 
> Could the machine check be restored before attempting to read its environment?

I believe machine cannot be NULL when it's from a kmap.  In fact, I
think we'd better guarantee it's non-NULL for all cases.  IIUC only case
it can be NULL is a test case (and it's not a kmap).  I think it's better
to update the test not to pass NULL instead.

Thanks,
Namhyung

> 
> >  			machine__map_x86_64_entry_trampolines(machine, dso);
> >  		goto out;
> >  	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=6

Re: [PATCH v5 03/11] perf tools: Try linked debug files for DSO debug info

[Namhyung Kim]

On Tue, Oct 13, 2026 at 05:34:34PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] perf tools: DEBUGLINK debuginfo resolution fails for containerized binaries
> - [Medium] perf: Uninitialized stack read when parsing malformed .gnu_debuglink
> --
> 
> commit 58954c9ee50dda18b6aba714c1a631c8845bc3e0
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf tools: Try linked debug files for DSO debug info
> 
> The dso__find_dbginfo_type() should try DSO_BINARY_TYPE__DEBUGLINK so it
> can follow the .gnu_debuglink and find the debug information in the
> separate file.
> 
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index df03f84c97235..913877b84b2d4 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -2111,6 +2111,7 @@ void dso__find_dbginfo_type(struct dso *dso)
> >  		DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO,
> >  		DSO_BINARY_TYPE__BUILDID_DEBUGINFO,
> >  		DSO_BINARY_TYPE__SYSTEM_PATH_DSO,
> > +		DSO_BINARY_TYPE__DEBUGLINK,
> 
> [Severity: Medium]
> Will this break debuginfo resolution for containerized binaries?
> 
> When DSO_BINARY_TYPE__DEBUGLINK is attempted, dso__get_filename() calls
> dso__read_binary_type_filename(). That function performs an early check
> on the host namespace:
> 
> tools/perf/util/dso.c:dso__read_binary_type_filename() {
> 	...
> 		if (!is_regular_file(filename)) {
> 			ret = -1;
> 			break;
> 		}
> 	...
> }
> 
> If the binary is inside a mount namespace, is_regular_file() fails and
> returns -1. This causes dso__get_filename() to abort early:
> 
> tools/perf/util/dso.c:dso__get_filename() {
> 	...
> 	if (dso__read_binary_type_filename(dso, type, root_dir, name, PATH_MAX))
> 		goto out;
> 	...
> }
> 
> Will this bypass the subsequent dso__filename_with_chroot() fallback logic,
> causing the tool to miss debuginfo for containerized applications?

This is a known problem and I think it's in a broken state.  Let me
work on a fix after landing this series.

> 
> 
> [Severity: Medium]
> Could this lead to an uninitialized stack read with malformed ELF files?
> 
> When resolving DSO_BINARY_TYPE__DEBUGLINK, dso__read_binary_type_filename()
> allocates an uninitialized stack buffer:
> 
> tools/perf/util/dso.c:dso__read_binary_type_filename() {
> 	...
> 		char symfile[PATH_MAX];
> 		...
> 		ret = filename__read_debuglink(filename, symfile, PATH_MAX);
> 	...
> }
> 
> If libbfd handles the read, it copies the section directly:
> 
> tools/perf/util/libbfd.c:libbfd_filename__read_debuglink() {
> 	...
> 	if (!bfd_get_section_contents(abfd, section, debuglink, 0,
> 				      section->size))
> 		goto out_close;
> 	...
> }
> 
> Since BFD does not append a null terminator, if the .gnu_debuglink section
> is malformed and lacks a null byte, could the subsequent snprintf() in
> dso__read_binary_type_filename() read uninitialized stack memory?

This is a pre-existing issue.  Let's fix it separately.

Thanks,
Namhyung

> 
> >  		DSO_BINARY_TYPE__NOT_FOUND,
> >  	};
> >  	const enum dso_binary_type *type;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261013172155.160869-1-namhyung@kernel.org?part=3