[PATCH v2 00/10] perf symbol: Properly set DSO binary/symtab types

[PATCH v2 00/10] perf symbol: Properly set 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.

v2 changes)

 * 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 (10):
  perf tools: Remove redundant dso data init
  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: Set dso symtab type for libbfd
  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 |   6 ++
 tools/perf/util/disasm.c    | 116 +++++-------------------------------
 tools/perf/util/dso.c       | 103 ++++++++++++++++++++++++++++----
 tools/perf/util/dso.h       |  13 ++++
 tools/perf/util/symbol.c    |  23 +++----
 5 files changed, 133 insertions(+), 128 deletions(-)

-- 
2.56.0.rc1.315.gc6ed9934b7-goog

[PATCH v2 01/10] perf tools: Remove redundant dso data init

[Namhyung Kim]

The fd and status are initialized later with cache.

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.rc1.315.gc6ed9934b7-goog

[PATCH v2 02/10] 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.

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.rc1.315.gc6ed9934b7-goog

[PATCH v2 03/10] 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.

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.rc1.315.gc6ed9934b7-goog

[PATCH v2 04/10] 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.

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

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index cf099cbd29f268ec..fc6e4dff282bc2dc 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1099,6 +1099,8 @@ static int maps__split_kallsyms(struct maps *kmaps, struct dso *dso, u64 delta,
 				return -1;
 
 			dso__set_kernel(ndso, dso__kernel(dso));
+			dso__set_binary_type(ndso, dso__binary_type(dso));
+			dso__set_symtab_type(ndso, dso__symtab_type(dso));
 			dso__set_loaded(ndso);
 
 			curr_map = map__new2(pos->start, ndso);
-- 
2.56.0.rc1.315.gc6ed9934b7-goog

[PATCH v2 05/10] perf symbol: Set dso symtab type for libbfd

[Namhyung Kim]

It missed to set the dso symtab type when the symbols are loaded by
libbfd.  When it happens both syms_ss and runtime_ss may not be set
and won't call dso__load_sym().

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 fc6e4dff282bc2dc..02b096ccbc60ac2a 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1933,6 +1933,7 @@ int dso__load(struct dso *dso, struct map *map)
 			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 
 		if (bfdrc == 0) {
+			dso__set_symtab_type(dso, symtab_type);
 			ret = 0;
 			break;
 		}
-- 
2.56.0.rc1.315.gc6ed9934b7-goog

[PATCH v2 06/10] 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.  It should be called under the
_dso__data_open_lock.

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

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dso.c | 100 +++++++++++++++++++++++++++++++++++++-----
 tools/perf/util/dso.h |  13 ++++++
 2 files changed, 102 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 913877b84b2d437e..f5054c18a2e931e7 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -833,7 +833,7 @@ 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)
+static enum dso_binary_type __dso__find_binary_type(struct dso *dso)
 	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
 {
 	enum dso_binary_type binary_type_data[] = {
@@ -842,25 +842,103 @@ static void try_to_open_dso(struct dso *dso, struct machine *machine)
 		DSO_BINARY_TYPE__NOT_FOUND,
 	};
 	int i = 0;
-	struct dso_data *dso_data = dso__data(dso);
+	char *path;
+	bool found = false;
+	bool decomp = false;
 
-	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;
+		path = dso__get_filename(dso, "", &decomp, dso__binary_type(dso));
+		if (path == NULL)
+			continue;
+
+		found = filename__has_section(path, ".text");
+		dso__put_filename(dso, path, decomp);
+		if (found)
+			return dso__binary_type(dso);
 
 	} while (dso__binary_type(dso) != DSO_BINARY_TYPE__NOT_FOUND);
-out:
+
+	if (dso__symtab_type(dso) != DSO_BINARY_TYPE__BUILD_ID_CACHE &&
+	    dso__symtab_type(dso) != DSO_BINARY_TYPE__SYSTEM_PATH_DSO) {
+		path = dso__get_filename(dso, "", &decomp, dso__symtab_type(dso));
+		if (path == NULL)
+			return dso__binary_type(dso);
+
+		found = filename__has_section(path, ".text");
+		dso__put_filename(dso, path, decomp);
+		if (found)
+			dso__set_binary_type(dso, dso__symtab_type(dso));
+	}
+	return dso__binary_type(dso);
+}
+
+enum dso_binary_type dso__find_binary_type(struct dso *dso)
+{
+	enum dso_binary_type ret;
+
+	mutex_lock(dso__data_open_lock());
+	ret = __dso__find_binary_type(dso);
+	mutex_unlock(dso__data_open_lock());
+
+	return ret;
+}
+
+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);
+	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.rc1.315.gc6ed9934b7-goog

[PATCH v2 07/10] 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.

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 02b096ccbc60ac2a..389dbd7e5192f4f2 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1859,6 +1859,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.rc1.315.gc6ed9934b7-goog

[PATCH v2 08/10] 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.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 389dbd7e5192f4f2..1175cb60f8206757 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1956,19 +1956,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.rc1.315.gc6ed9934b7-goog

[PATCH v2 09/10] 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.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/disasm.c | 116 ++++++---------------------------------
 1 file changed, 16 insertions(+), 100 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 5478c134e7e3b8df..07a345beb6250a0f 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,24 +1488,30 @@ 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;
+
+	symfs_filename = dso__get_filename(dso, "", &decomp, dbt);
+	if (symfs_filename == NULL && dbt != DSO_BINARY_TYPE__BPF_PROG_INFO &&
+	    dbt != DSO_BINARY_TYPE__BPF_IMAGE)
+		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),
-		 map__unmap_ip(map, sym->end));
+		 symfs_filename ?: dso__long_name(dso), sym->name,
+		 map__unmap_ip(map, sym->start), map__unmap_ip(map, sym->end));
 
 	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;
@@ -1593,17 +1521,6 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
 			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);
-		}
 	}
 
 	/*
@@ -1660,8 +1577,7 @@ 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);
+	dso__put_filename(dso, symfs_filename, decomp);
 
 	if (delete_extract)
 		kcore_extract__delete(&kce);
-- 
2.56.0.rc1.315.gc6ed9934b7-goog

[PATCH v2 10/10] 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.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/dso-data.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index 46bc3f59726004fa..5a027ffab541b4a2 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -138,6 +138,9 @@ static int test__dso_data(struct test_suite *test __maybe_unused, int subtest __
 	dsos__init(&machine.dsos);
 
 	dso = dso__new(file);
+	/* 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);
@@ -225,6 +228,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);
+		/* 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 get dso", dso);
 		TEST_ASSERT_VAL("failed to add dso", !dsos__add(dsos, dso));
 		dso__put(dso);
-- 
2.56.0.rc1.315.gc6ed9934b7-goog

Re: [PATCH v2 01/10] perf tools: Remove redundant dso data init

[sashiko-bot]

> The fd and status are initialized later with cache.
> 
> 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/20261006234315.920817-1-namhyung@kernel.org?part=1

Re: [PATCH v2 07/10] 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.
> 
> 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/20261006234315.920817-1-namhyung@kernel.org?part=7

Re: [PATCH v2 02/10] 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: DSO_BINARY_TYPE__DEBUGLINK bypasses mount namespace fallback
- [Low] perf: Incorrect path formatting in DEBUGLINK resolution for local files
--

commit 8a09d3bb34892b4472de63b4949a0752ebfa1103
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 addition bypass the mount namespace fallback for containerized
workloads?

When dso__find_dbginfo_type() iterates through the types and calls
dso__get_filename() for DSO_BINARY_TYPE__DEBUGLINK, it performs direct file
I/O on the host path:

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

If the binary is containerized and not present on the host at that exact
path, this check fails and returns -1. This early error return causes
dso__get_filename() to exit via "goto out" before it can attempt the mount
namespace fallback logic:

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

	if (!is_regular_file(name)) {
        ...
		new_name = dso__filename_with_chroot(dso, name);
    ...
}

Can the early file existence check in dso__read_binary_type_filename() be
adjusted so it does not permanently bypass dso__filename_with_chroot()?

[Severity: Low]
Does this code incorrectly format fallback paths for local files?

In dso__read_binary_type_filename() for DSO_BINARY_TYPE__DEBUGLINK, the
directory component is extracted by searching backward for a slash:

tools/perf/util/dso.c:dso__read_binary_type_filename() {
    ...
		last_slash = filename + len;
		while (last_slash != filename && *last_slash != '/')
			last_slash--;

		strncpy(dso_dir, filename, last_slash - filename);
		dso_dir[last_slash-filename] = '\0';
    ...
}

If a DSO is specified without a path separator (e.g., a local file like
"vmlinux" in the current directory), last_slash remains equal to filename,
leaving dso_dir as an empty string.

Later in the function, debuglink_paths[i] evaluates the formats:

tools/perf/util/dso.c:dso__read_binary_type_filename() {
    ...
		for (i = 0; i < ARRAY_SIZE(debuglink_paths); i++) {
			snprintf(filename, size,
					debuglink_paths[i], dso_dir, symfile);
    ...
}

When evaluating formats like "%s/.debug/%s" against an empty dso_dir, doesn't
this incorrectly generate an absolute path starting from the root directory
(e.g., "/.debug/symfile") instead of a relative path from the current
directory?

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

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

Re: [PATCH v2 03/10] 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.
> 
> 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/20261006234315.920817-1-namhyung@kernel.org?part=3

Re: [PATCH v2 08/10] perf symbol: Do not set binary type from symtab type

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] perf: data-only DSOs fail to open due to strict .text section requirement
- [High] perf: compat offline DWARF unwinding broken due to missing binary_type
- [Medium] perf: redundant double-decompression of compressed kernel modules
--

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

perf symbol: Do not set binary type from symtab type

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.

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 389dbd7e5192f..1175cb60f8206 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1956,19 +1956,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);
> -

[Severity: High]
Does removing this fallback prevent opening data-only DSOs?

Without this early assignment, DSOs remain as DSO_BINARY_TYPE__NOT_FOUND.
Any profiling workload attempting to access symbols in a DSO without
executable code might hit this path:

try_to_open_dso()
  __dso__find_binary_type()
    filename__has_section(path, ".text")

Because the ".text" check fails for data-only DSOs, the binary type remains
DSO_BINARY_TYPE__NOT_FOUND. This bypasses the correct opening logic in
open_dso() and permanently marks the DSO with DSO_DATA_STATUS_ERROR. Can we
still open data-only DSOs (like eBPF maps or data-only kernel modules)
after this change?

[Severity: Medium]
Without the early binary type assignment, does this introduce redundant
decompression for compressed kernel modules?

When try_to_open_dso() is called for a compressed kernel module, it first
calls __dso__find_binary_type(), which uses dso__get_filename() to fully
decompress the module to a temporary file just to check for a ".text"
section, and then unlinks it.

Immediately following this, try_to_open_dso() calls open_dso() which again
calls dso__get_filename(), triggering a second complete decompression of the
exact same module. Can this cause severe performance degradation when
processing profiles containing compressed kernel modules?

[Severity: High]
Could this removal also break offline DWARF stack unwinding for compat tasks?

Because user-space maps now have a binary type of DSO_BINARY_TYPE__NOT_FOUND
during early processing, when thread__e_machine_endian() calls
dso__e_machine_endian() to determine the architecture, it hits the fallback:

tools/perf/util/dso.c:dso__e_machine_endian() {
    ...
    switch (dso__binary_type(dso)) {
    ...
    case DSO_BINARY_TYPE__NOT_FOUND:
    default:
        if (e_flags)
            *e_flags = 0;
        return EM_NONE;
    }
}

It immediately fails and returns EM_NONE without attempting to open the file.
Consequently, thread__e_machine_endian() fails to extract the architecture
from any map and falls back to perf_env__e_machine(), which incorrectly
applies the host architecture to the compat thread.

>  			if (syms_ss && runtime_ss)
>  				break;
>  		} else {

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

Re: [PATCH v2 05/10] perf symbol: Set dso symtab type for libbfd

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf: dso__load() returns 0 on libbfd success causing map__load() to discard first sample
- [Low] perf: unintended fallthrough to dso__load_sym clobbers libbfd symtab type
--

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

perf symbol: Set dso symtab type for libbfd

It missed to set the dso symtab type when the symbols are loaded by
libbfd.  When it happens both syms_ss and runtime_ss may not be set
and won't call dso__load_sym().

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index fc6e4dff282bc..02b096ccbc60a 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1933,6 +1933,7 @@ int dso__load(struct dso *dso, struct map *map)
>  			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
>  
>  		if (bfdrc == 0) {
> +			dso__set_symtab_type(dso, symtab_type);
>  			ret = 0;

[Severity: High]
Does returning 0 here cause map__load() to discard the first sample?

Unlike dso__load_sym(), which returns the number of loaded symbols (> 0),
leaving ret = 0 causes dso__load() to return 0. The caller map__load()
strictly checks "if (nr == 0)" and treats it as a failure:

tools/perf/util/map.c:map__load() {
    ...
	} else if (nr == 0) {
    ...
		return -1;
	}

Because dso__load() correctly calls dso__set_loaded(dso) before returning,
subsequent calls to map__load() hit the early success path and return 0.
As a result, does the very first symbol lookup for a libbfd-loaded map
spuriously fail and cause perf to discard the sample?

>  			break;

[Severity: Low]
Does breaking out of the loop here unintentionally clobber the libbfd symtab
type if syms_ss was already set?

If a prior loop iteration populates syms_ss (e.g., from an ELF file) and a
subsequent iteration successfully loads libbfd symbols (bfdrc == 0), the
loop breaks here to return the libbfd symbols.

However, since syms_ss is non-NULL from the prior iteration, the post-loop
cleanup bypass fails:

tools/perf/util/symbol.c:dso__load() {
    ...
	if (!runtime_ss && !syms_ss)
		goto out_free;
    ...
	if (syms_ss)
		ret = dso__load_sym(dso, map, syms_ss, runtime_ss, kmod);
    ...
}

Because execution falls through to dso__load_sym(), does it unconditionally
call dso__set_symtab_type(dso, syms_ss->type) and silently overwrite the
newly added symtab_type, mixing the symbols?

>  		}

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

Re: [PATCH v2 04/10] perf symbol: Set binary/symtab type for split kallsyms

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: Incorrect binary_type copied to split kallsyms DSOs
--

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

perf symbol: Set binary/symtab type for split kallsyms

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

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index cf099cbd29f26..fc6e4dff282bc 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1099,6 +1099,8 @@ static int maps__split_kallsyms(struct maps *kmaps, struct dso *dso, u64 delta,
>  				return -1;
>  
>  			dso__set_kernel(ndso, dso__kernel(dso));
> +			dso__set_binary_type(ndso, dso__binary_type(dso));
> +			dso__set_symtab_type(ndso, dso__symtab_type(dso));

[Severity: Medium]
Are we copying an outdated binary_type into the new split DSOs here?

Looking at the execution flow, maps__split_kallsyms() is called via
dso__load_kallsyms() from within dso__load_kernel_sym():

dso__load_kernel_sym() {
	...
do_kallsyms:
	err = dso__load_kallsyms(dso, kallsyms_filename, map);
	...
	if (err > 0 && !dso__is_kcore(dso)) {
		...
		dso__set_binary_type(dso, DSO_BINARY_TYPE__KALLSYMS);
	...
}

Because dso__load_kallsyms() executes before dso__load_kernel_sym() updates
the parent dso's binary_type to DSO_BINARY_TYPE__KALLSYMS, the value returned
by dso__binary_type(dso) during the split operation may still be the old
stale value.

Could this prevent the split kallsyms DSOs from receiving the correct
binary and symtab types?

>  			dso__set_loaded(ndso);
>  
>  			curr_map = map__new2(pos->start, ndso);

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

Re: [PATCH v2 10/10] perf test: Fix dso data tests

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf test: NULL pointer dereference on dso__new() failure
- [Medium] perf: NULL pointer dereference when dso__new() fails
--

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

perf test: Fix dso data tests

This commit fixes the dso data tests by bypassing the ELF check for binary
types so that read/write operations on the test file can proceed properly.

> diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
> index 46bc3f5972600..5a027ffab541b 100644
> --- a/tools/perf/tests/dso-data.c
> +++ b/tools/perf/tests/dso-data.c
> @@ -138,6 +138,9 @@ static int test__dso_data(struct test_suite *test __maybe_unused, int subtest __
>  	dsos__init(&machine.dsos);
>  
>  	dso = dso__new(file);
> +	/* override the binary type for dso data access */
> +	dso__set_binary_type(dso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO);
> +	dso__set_bintype_searched(dso);

[Severity: Medium]
Will this crash if dso__new() fails?

If memory allocation fails and dso__new() returns NULL, these newly added
calls will unconditionally dereference the pointer, causing a segmentation
fault instead of allowing dsos__add() to handle the NULL safely and fail
the test gracefully.

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

[ ... ]

> @@ -225,6 +228,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);
> +		/* override the binary type for dso data access */
> +		dso__set_binary_type(dso, DSO_BINARY_TYPE__SYSTEM_PATH_DSO);
> +		dso__set_bintype_searched(dso);

[Severity: Medium]
Similarly, does this bypass the subsequent NULL check for dso?

By unconditionally dereferencing dso, a failure from dso__new() will cause
a crash before reaching the explicit TEST_ASSERT_VAL() check intended to safely
handle this exact allocation failure.

>  		TEST_ASSERT_VAL("failed to get dso", dso);
>  		TEST_ASSERT_VAL("failed to add dso", !dsos__add(dsos, dso));
>  		dso__put(dso);

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] perf: string truncation in symbol__disassemble() due to sizeof(pointer)
- [High] perf: NULL pointer dereference in LLVM disassembly backend for BPF
- [High] perf annotate: UAF in dso__get_filename due to missing dso__lock
--

commit b52fe9a1d630e0d03a547c6ca49efe1679043beb
Author: Namhyung Kim <namhyung@kernel.org>
Subject: perf annotate: Simplify symbol__disassemble() with dso binary_type

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.

> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 5478c134e7e3b..07a345beb6250 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
[ ... ]
> @@ -1566,24 +1488,30 @@ 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;
> +
> +	symfs_filename = dso__get_filename(dso, "", &decomp, dbt);

[Severity: High]
Does this call to dso__get_filename() need to be protected by dso__lock()?

The previous code had an explicit mutex_lock(dso__lock(dso)) around the
access to dso__nsinfo(dso). dso__get_filename() reads and uses
dso__nsinfo(dso) via dso__filename_with_chroot(), but lacks internal
locking.

If another thread concurrently processes an MMAP event and calls
dso__set_nsinfo(), could the nsinfo pointer be freed between the check and
its use, leading to a use-after-free?

> +	if (symfs_filename == NULL && dbt != DSO_BINARY_TYPE__BPF_PROG_INFO &&
> +	    dbt != DSO_BINARY_TYPE__BPF_IMAGE)
> +		return SYMBOL_ANNOTATE_ERRNO__COULDNT_DETERMINE_FILE_TYPE;

[Severity: High]
When annotating BPF programs, this explicitly allows symfs_filename to be
NULL. However, could this NULL pointer be passed down to the LLVM
disassembly backend?

In the LLVM backend:
symbol__disassemble_llvm()
  llvm_name_for_code(..., filename, ...)

Inside llvm-c-helpers.cpp, symbolizer->symbolizeCode(dso_name, ...)
implicitly constructs an llvm::StringRef from the dso_name pointer.

If the pointer is NULL, won't this perform a strlen(NULL) and cause a
segmentation fault? Similarly, on PowerPC, could symbol__disassemble_raw()
invoke pr_debug("%s", filename) or open(filename, ...) with the NULL pointer,
triggering undefined behavior?

>  
>  	pr_debug("%s: filename=%s, sym=%s, start=%#" PRIx64 ", end=%#" PRIx64 "\n", __func__,
> -		 symfs_filename, sym->name, map__unmap_ip(map, sym->start),
> -		 map__unmap_ip(map, sym->end));
> +		 symfs_filename ?: dso__long_name(dso), sym->name,
> +		 map__unmap_ip(map, sym->start), map__unmap_ip(map, sym->end));
>  
>  	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;
> @@ -1593,17 +1521,6 @@ int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
>  			delete_extract = true;
>  			strlcpy(symfs_filename, kce.extract_filename, sizeof(symfs_filename));

[Severity: High]
Since symfs_filename is now a char * pointer rather than an array of size
PATH_MAX, doesn't sizeof(symfs_filename) evaluate to 8 (on 64-bit systems)?

Will this cause strlcpy() to truncate the kcore extraction path to 7
characters, leading to ENOENT failures in the downstream disassemblers?

>  		}

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

Re: [PATCH v2 06/10] perf tools: Factor out dso__find_binary_type()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] perf: Guest kernel modules fail to open due to hardcoded root_dir
- [High] perf: Strict .text section check breaks data-only DSOs and NO_LIBELF builds
- [High] perf: Concurrent Read-Modify-Write data race on struct dso bitfields
- [High] perf: Use-After-Free of dso->nsinfo in __dso__find_binary_type due to missing dso__lock
- [Medium] perf: Explicitly resolved binary types are unconditionally overwritten
- [Medium] perf: Bypassing do_open() permanently caches EMFILE failures as NOT_FOUND
- [Low] perf: Redundant double decompression of compressed kernel modules
--

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

perf tools: Factor out dso__find_binary_type()

Separate out the logic to detect binary type of a DSO.  It checks if the
candidate file contains ".text" section.  It should be called under the
_dso__data_open_lock.

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

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 913877b84b2d4..f5054c18a2e93 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -833,7 +833,7 @@ 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)
> +static enum dso_binary_type __dso__find_binary_type(struct dso *dso)
>  	EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
>  {
>  	enum dso_binary_type binary_type_data[] = {
> @@ -842,25 +842,103 @@ static void try_to_open_dso(struct dso *dso, struct machine *machine)
>  		DSO_BINARY_TYPE__NOT_FOUND,
>  	};
>  	int i = 0;
> -	struct dso_data *dso_data = dso__data(dso);
> +	char *path;
> +	bool found = false;
> +	bool decomp = false;
>  
> -	if (dso_data->fd >= 0)
> -		return;
> +	if (dso__bintype_searched(dso))
> +		return dso__binary_type(dso);
> +	dso__set_bintype_searched(dso);

[Severity: High]
Does this code introduce a data race on the struct dso bitfields?

The dso__set_bintype_searched() function modifies the bintype_searched
bitfield under the global _dso__data_open_lock. However, adjacent bitfields
like debuginfo_searched and long_name_allocated share the same memory
location and are modified under different locks (dso__lock(dso) and
dsos->lock).

Concurrent execution could cause compiler-generated read-modify-write
operations to overlap, resulting in dropped flags or memory leaks if
long_name_allocated is clobbered.

[ ... ]

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

[Severity: Medium]
Does this fall-through discard valid binary types that have already been
resolved elsewhere in the system?

When symbol.c successfully resolves and sets the binary type (such as
DSO_BINARY_TYPE__SYSTEM_PATH_DSO or DSO_BINARY_TYPE__BUILD_ID_CACHE),
this switch statement deliberately falls through. The loop below then
unconditionally overwrites the previously valid binary type.

If the fallback types lack a .text section (such as data-only DSOs), the
state is permanently left as NOT_FOUND, preventing perf from reading them.

[ ... ]

>  	do {
>  		dso__set_binary_type(dso, binary_type_data[i++]);
>  
> -		dso_data->fd = open_dso(dso, machine);
> -		if (dso_data->fd >= 0)
> -			goto out;
> +		path = dso__get_filename(dso, "", &decomp, dso__binary_type(dso));

[Severity: High]
Does hardcoding the root directory to an empty string break path resolution
for guest kernel modules?

When the fallback logic looks up types like DSO_BINARY_TYPE__GUEST_KMODULE,
it passes "" to dso__get_filename() instead of machine->root_dir.

This causes dso__get_filename() to construct an invalid path missing the
guest's root directory. The file cannot be found, filename__has_section()
fails, and subsequent calls to open_dso() will fail.

[Severity: High]
Does calling dso__get_filename() without holding dso__lock(dso) cause a
use-after-free of dso->nsinfo?

The new __dso__find_binary_type() helper executes with only the global
_dso__data_open_lock held. It calls dso__get_filename(), which invokes
dso__filename_with_chroot() and dereferences dso->nsinfo to read the
process ID.

Concurrently, another thread could call dso__set_nsinfo() (which acquires
dso__lock(dso)), replacing the nsinfo pointer and freeing the old one via
nsinfo__put(). Since this read path ignores dso__lock(dso), it can read
the freed pointer.

> +		if (path == NULL)
> +			continue;
> +
> +		found = filename__has_section(path, ".text");

[Severity: High]
Does this strict .text section check break data-only DSOs and NO_LIBELF
builds?

Prior to this patch, try_to_open_dso() discovered the DSO by calling
open_dso(). If the file existed and was readable, the open succeeded. This
patch introduces a strict requirement that a DSO must contain a .text
section.

Perf commonly profiles memory data accesses (like perf mem or perf c2c)
hitting data-only DSOs. For these files, filename__has_section() evaluates
to false, forcing the binary type to NOT_FOUND. Furthermore, if perf is
compiled without libelf (NO_LIBELF=1), filename__has_section() is stubbed
to unconditionally return false, breaking user DSO loading entirely.

[Severity: Medium]
Can this bypass of do_open() permanently cache EMFILE failures as NOT_FOUND?

The filename__has_section() function uses a raw open() without the FD limit
(EMFILE) handling and LRU eviction logic normally provided by do_open() and
close_first_dso().

If perf hits RLIMIT_NOFILE, open() returns -1, and filename__has_section()
silently returns false. __dso__find_binary_type() then assumes the binary
is invalid, sets bintype_searched = 1, and permanently caches
DSO_BINARY_TYPE__NOT_FOUND, breaking data access to the DSO for the rest
of the execution.

[ ... ]

> +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);
> +	dso_data->fd = open_dso(dso, machine);

[Severity: Low]
Does this result in redundant double decompression of compressed kernel
modules?

The call to dso__get_filename() in __dso__find_binary_type() decompresses
a compressed kernel module to a temporary file. After checking for a .text
section, dso__put_filename() unlinks this temporary file.

Immediately after returning, try_to_open_dso() calls open_dso(), which
ultimately calls dso__get_filename() again, forcing a second decompression
of the exact same kernel module.

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