* [PATCH v1 1/2] perf symbol-minimal: Fix ehdr reading in filename__read_build_id
2025-08-23 0:00 [PATCH v1 0/2] Build ID mmap related fixes Ian Rogers
@ 2025-08-23 0:00 ` Ian Rogers
2025-08-23 0:00 ` [PATCH v1 2/2] perf symbol: Add blocking argument to filename__read_build_id Ian Rogers
2025-08-25 22:06 ` [PATCH v1 0/2] Build ID mmap related fixes Namhyung Kim
2 siblings, 0 replies; 4+ messages in thread
From: Ian Rogers @ 2025-08-23 0:00 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, James Clark, Collin Funk,
Masami Hiramatsu (Google), Stephen Brennan, Palmer Dabbelt,
Haibo Xu, Dmitry Vyukov, Song Liu, Thomas Falcon,
linux-perf-users, linux-kernel
The e_ident is part of the ehdr and so reading it a second time would
mean the read ehdr was displaced by 16-bytes. Switch from stdio to
open/read/lseek syscalls for similarity with the symbol-elf version of
the function and so that later changes can alter then open flags.
Fixes: fef8f648bb47 ("perf symbol: Fix use-after-free in filename__read_build_id")
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/symbol-minimal.c | 55 ++++++++++++++++----------------
1 file changed, 27 insertions(+), 28 deletions(-)
diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index 7201494c5c20..8d41bd7842df 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -4,7 +4,6 @@
#include <errno.h>
#include <unistd.h>
-#include <stdio.h>
#include <fcntl.h>
#include <string.h>
#include <stdlib.h>
@@ -88,11 +87,8 @@ int filename__read_debuglink(const char *filename __maybe_unused,
*/
int filename__read_build_id(const char *filename, struct build_id *bid)
{
- FILE *fp;
- int ret = -1;
+ int fd, ret = -1;
bool need_swap = false, elf32;
- u8 e_ident[EI_NIDENT];
- int i;
union {
struct {
Elf32_Ehdr ehdr32;
@@ -103,28 +99,27 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
Elf64_Phdr *phdr64;
};
} hdrs;
- void *phdr;
- size_t phdr_size;
- void *buf = NULL;
- size_t buf_size = 0;
+ void *phdr, *buf = NULL;
+ ssize_t phdr_size, ehdr_size, buf_size = 0;
- fp = fopen(filename, "r");
- if (fp == NULL)
+ fd = open(filename, O_RDONLY);
+ if (fd < 0)
return -1;
- if (fread(e_ident, sizeof(e_ident), 1, fp) != 1)
+ if (read(fd, hdrs.ehdr32.e_ident, EI_NIDENT) != EI_NIDENT)
goto out;
- if (memcmp(e_ident, ELFMAG, SELFMAG) ||
- e_ident[EI_VERSION] != EV_CURRENT)
+ if (memcmp(hdrs.ehdr32.e_ident, ELFMAG, SELFMAG) ||
+ hdrs.ehdr32.e_ident[EI_VERSION] != EV_CURRENT)
goto out;
- need_swap = check_need_swap(e_ident[EI_DATA]);
- elf32 = e_ident[EI_CLASS] == ELFCLASS32;
+ need_swap = check_need_swap(hdrs.ehdr32.e_ident[EI_DATA]);
+ elf32 = hdrs.ehdr32.e_ident[EI_CLASS] == ELFCLASS32;
+ ehdr_size = (elf32 ? sizeof(hdrs.ehdr32) : sizeof(hdrs.ehdr64)) - EI_NIDENT;
- if (fread(elf32 ? (void *)&hdrs.ehdr32 : (void *)&hdrs.ehdr64,
- elf32 ? sizeof(hdrs.ehdr32) : sizeof(hdrs.ehdr64),
- 1, fp) != 1)
+ if (read(fd,
+ (elf32 ? (void *)&hdrs.ehdr32 : (void *)&hdrs.ehdr64) + EI_NIDENT,
+ ehdr_size) != ehdr_size)
goto out;
if (need_swap) {
@@ -138,14 +133,18 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
hdrs.ehdr64.e_phnum = bswap_16(hdrs.ehdr64.e_phnum);
}
}
- phdr_size = elf32 ? hdrs.ehdr32.e_phentsize * hdrs.ehdr32.e_phnum
- : hdrs.ehdr64.e_phentsize * hdrs.ehdr64.e_phnum;
+ if ((elf32 && hdrs.ehdr32.e_phentsize != sizeof(Elf32_Phdr)) ||
+ (!elf32 && hdrs.ehdr64.e_phentsize != sizeof(Elf64_Phdr)))
+ goto out;
+
+ phdr_size = elf32 ? sizeof(Elf32_Phdr) * hdrs.ehdr32.e_phnum
+ : sizeof(Elf64_Phdr) * hdrs.ehdr64.e_phnum;
phdr = malloc(phdr_size);
if (phdr == NULL)
goto out;
- fseek(fp, elf32 ? hdrs.ehdr32.e_phoff : hdrs.ehdr64.e_phoff, SEEK_SET);
- if (fread(phdr, phdr_size, 1, fp) != 1)
+ lseek(fd, elf32 ? hdrs.ehdr32.e_phoff : hdrs.ehdr64.e_phoff, SEEK_SET);
+ if (read(fd, phdr, phdr_size) != phdr_size)
goto out_free;
if (elf32)
@@ -153,8 +152,8 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
else
hdrs.phdr64 = phdr;
- for (i = 0; i < elf32 ? hdrs.ehdr32.e_phnum : hdrs.ehdr64.e_phnum; i++) {
- size_t p_filesz;
+ for (int i = 0; i < (elf32 ? hdrs.ehdr32.e_phnum : hdrs.ehdr64.e_phnum); i++) {
+ ssize_t p_filesz;
if (need_swap) {
if (elf32) {
@@ -180,8 +179,8 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
goto out_free;
buf = tmp;
}
- fseek(fp, elf32 ? hdrs.phdr32[i].p_offset : hdrs.phdr64[i].p_offset, SEEK_SET);
- if (fread(buf, p_filesz, 1, fp) != 1)
+ lseek(fd, elf32 ? hdrs.phdr32[i].p_offset : hdrs.phdr64[i].p_offset, SEEK_SET);
+ if (read(fd, buf, p_filesz) != p_filesz)
goto out_free;
ret = read_build_id(buf, p_filesz, bid, need_swap);
@@ -194,7 +193,7 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
free(buf);
free(phdr);
out:
- fclose(fp);
+ close(fd);
return ret;
}
--
2.51.0.rc2.233.g662b1ed5c5-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v1 2/2] perf symbol: Add blocking argument to filename__read_build_id
2025-08-23 0:00 [PATCH v1 0/2] Build ID mmap related fixes Ian Rogers
2025-08-23 0:00 ` [PATCH v1 1/2] perf symbol-minimal: Fix ehdr reading in filename__read_build_id Ian Rogers
@ 2025-08-23 0:00 ` Ian Rogers
2025-08-25 22:06 ` [PATCH v1 0/2] Build ID mmap related fixes Namhyung Kim
2 siblings, 0 replies; 4+ messages in thread
From: Ian Rogers @ 2025-08-23 0:00 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, James Clark, Collin Funk,
Masami Hiramatsu (Google), Stephen Brennan, Palmer Dabbelt,
Haibo Xu, Dmitry Vyukov, Song Liu, Thomas Falcon,
linux-perf-users, linux-kernel
When synthesizing build-ids, for build ID mmap2 events, they will be
added for data mmaps if -d/--data is specified. The files opened for
their build IDs may block on the open causing perf to hang during
synthesis. There is some robustness in existing calls to
filename__read_build_id by checking the file path is to a regular
file, which unfortunately fails for symlinks. Rather than adding more
is_regular_file calls, switch filename__read_build_id to take a
"block" argument and specify O_NONBLOCK when this is false. The
existing is_regular_file checking callers and the event synthesis
callers are made to pass false and thereby avoiding the hang.
Fixes: 53b00ff358dc ("perf record: Make --buildid-mmap the default")
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/bench/inject-buildid.c | 2 +-
tools/perf/builtin-buildid-cache.c | 8 ++++----
tools/perf/builtin-inject.c | 4 ++--
tools/perf/tests/sdt.c | 2 +-
tools/perf/util/build-id.c | 4 ++--
tools/perf/util/debuginfo.c | 8 ++++++--
tools/perf/util/dsos.c | 4 ++--
tools/perf/util/symbol-elf.c | 9 +++++----
tools/perf/util/symbol-minimal.c | 6 +++---
tools/perf/util/symbol.c | 8 ++++----
tools/perf/util/symbol.h | 2 +-
tools/perf/util/synthetic-events.c | 2 +-
12 files changed, 32 insertions(+), 27 deletions(-)
diff --git a/tools/perf/bench/inject-buildid.c b/tools/perf/bench/inject-buildid.c
index aad572a78d7f..12387ea88b9a 100644
--- a/tools/perf/bench/inject-buildid.c
+++ b/tools/perf/bench/inject-buildid.c
@@ -85,7 +85,7 @@ static int add_dso(const char *fpath, const struct stat *sb __maybe_unused,
if (typeflag == FTW_D || typeflag == FTW_SL)
return 0;
- if (filename__read_build_id(fpath, &bid) < 0)
+ if (filename__read_build_id(fpath, &bid, /*block=*/true) < 0)
return 0;
dso->name = realpath(fpath, NULL);
diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index c98104481c8a..2e0f2004696a 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -180,7 +180,7 @@ static int build_id_cache__add_file(const char *filename, struct nsinfo *nsi)
struct nscookie nsc;
nsinfo__mountns_enter(nsi, &nsc);
- err = filename__read_build_id(filename, &bid);
+ err = filename__read_build_id(filename, &bid, /*block=*/true);
nsinfo__mountns_exit(&nsc);
if (err < 0) {
pr_debug("Couldn't read a build-id in %s\n", filename);
@@ -204,7 +204,7 @@ static int build_id_cache__remove_file(const char *filename, struct nsinfo *nsi)
int err;
nsinfo__mountns_enter(nsi, &nsc);
- err = filename__read_build_id(filename, &bid);
+ err = filename__read_build_id(filename, &bid, /*block=*/true);
nsinfo__mountns_exit(&nsc);
if (err < 0) {
pr_debug("Couldn't read a build-id in %s\n", filename);
@@ -280,7 +280,7 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
return true;
- if (filename__read_build_id(filename, &bid) == -1) {
+ if (filename__read_build_id(filename, &bid, /*block=*/true) == -1) {
if (errno == ENOENT)
return false;
@@ -309,7 +309,7 @@ static int build_id_cache__update_file(const char *filename, struct nsinfo *nsi)
int err;
nsinfo__mountns_enter(nsi, &nsc);
- err = filename__read_build_id(filename, &bid);
+ err = filename__read_build_id(filename, &bid, /*block=*/true);
nsinfo__mountns_exit(&nsc);
if (err < 0) {
pr_debug("Couldn't read a build-id in %s\n", filename);
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 40ba6a94f719..a114b3fa1bea 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -680,12 +680,12 @@ static int dso__read_build_id(struct dso *dso)
mutex_lock(dso__lock(dso));
nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
- if (filename__read_build_id(dso__long_name(dso), &bid) > 0)
+ if (filename__read_build_id(dso__long_name(dso), &bid, /*block=*/true) > 0)
dso__set_build_id(dso, &bid);
else if (dso__nsinfo(dso)) {
char *new_name = dso__filename_with_chroot(dso, dso__long_name(dso));
- if (new_name && filename__read_build_id(new_name, &bid) > 0)
+ if (new_name && filename__read_build_id(new_name, &bid, /*block=*/true) > 0)
dso__set_build_id(dso, &bid);
free(new_name);
}
diff --git a/tools/perf/tests/sdt.c b/tools/perf/tests/sdt.c
index 93baee2eae42..6132f1af3e22 100644
--- a/tools/perf/tests/sdt.c
+++ b/tools/perf/tests/sdt.c
@@ -31,7 +31,7 @@ static int build_id_cache__add_file(const char *filename)
struct build_id bid = { .size = 0, };
int err;
- err = filename__read_build_id(filename, &bid);
+ err = filename__read_build_id(filename, &bid, /*block=*/true);
if (err < 0) {
pr_debug("Failed to read build id of %s\n", filename);
return err;
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index a7018a3b0437..bf7f3268b9a2 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -115,7 +115,7 @@ int filename__snprintf_build_id(const char *pathname, char *sbuild_id, size_t sb
struct build_id bid = { .size = 0, };
int ret;
- ret = filename__read_build_id(pathname, &bid);
+ ret = filename__read_build_id(pathname, &bid, /*block=*/true);
if (ret < 0)
return ret;
@@ -841,7 +841,7 @@ static int filename__read_build_id_ns(const char *filename,
int ret;
nsinfo__mountns_enter(nsi, &nsc);
- ret = filename__read_build_id(filename, bid);
+ ret = filename__read_build_id(filename, bid, /*block=*/true);
nsinfo__mountns_exit(&nsc);
return ret;
diff --git a/tools/perf/util/debuginfo.c b/tools/perf/util/debuginfo.c
index a44c70f93156..bb9ebd84ec2d 100644
--- a/tools/perf/util/debuginfo.c
+++ b/tools/perf/util/debuginfo.c
@@ -110,8 +110,12 @@ struct debuginfo *debuginfo__new(const char *path)
if (!dso)
goto out;
- /* Set the build id for DSO_BINARY_TYPE__BUILDID_DEBUGINFO */
- if (is_regular_file(path) && filename__read_build_id(path, &bid) > 0)
+ /*
+ * Set the build id for DSO_BINARY_TYPE__BUILDID_DEBUGINFO. Don't block
+ * incase the path isn't for a regular file.
+ */
+ assert(!dso__has_build_id(dso));
+ if (filename__read_build_id(path, &bid, /*block=*/false) > 0)
dso__set_build_id(dso, &bid);
for (type = distro_dwarf_types;
diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index 0a7645c7fae7..64c1d65b0149 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -81,13 +81,13 @@ static int dsos__read_build_ids_cb(struct dso *dso, void *data)
return 0;
}
nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
- if (filename__read_build_id(dso__long_name(dso), &bid) > 0) {
+ if (filename__read_build_id(dso__long_name(dso), &bid, /*block=*/true) > 0) {
dso__set_build_id(dso, &bid);
args->have_build_id = true;
} else if (errno == ENOENT && dso__nsinfo(dso)) {
char *new_name = dso__filename_with_chroot(dso, dso__long_name(dso));
- if (new_name && filename__read_build_id(new_name, &bid) > 0) {
+ if (new_name && filename__read_build_id(new_name, &bid, /*block=*/true) > 0) {
dso__set_build_id(dso, &bid);
args->have_build_id = true;
}
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 6d2c280a1730..033c79231a54 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -902,7 +902,7 @@ static int read_build_id(const char *filename, struct build_id *bid)
#else // HAVE_LIBBFD_BUILDID_SUPPORT
-static int read_build_id(const char *filename, struct build_id *bid)
+static int read_build_id(const char *filename, struct build_id *bid, bool block)
{
size_t size = sizeof(bid->data);
int fd, err = -1;
@@ -911,7 +911,7 @@ static int read_build_id(const char *filename, struct build_id *bid)
if (size < BUILD_ID_SIZE)
goto out;
- fd = open(filename, O_RDONLY);
+ fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
if (fd < 0)
goto out;
@@ -934,7 +934,7 @@ static int read_build_id(const char *filename, struct build_id *bid)
#endif // HAVE_LIBBFD_BUILDID_SUPPORT
-int filename__read_build_id(const char *filename, struct build_id *bid)
+int filename__read_build_id(const char *filename, struct build_id *bid, bool block)
{
struct kmod_path m = { .name = NULL, };
char path[PATH_MAX];
@@ -958,9 +958,10 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
}
close(fd);
filename = path;
+ block = true;
}
- err = read_build_id(filename, bid);
+ err = read_build_id(filename, bid, block);
if (m.comp)
unlink(filename);
diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index 8d41bd7842df..41e4ebe5eac5 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -85,7 +85,7 @@ int filename__read_debuglink(const char *filename __maybe_unused,
/*
* Just try PT_NOTE header otherwise fails
*/
-int filename__read_build_id(const char *filename, struct build_id *bid)
+int filename__read_build_id(const char *filename, struct build_id *bid, bool block)
{
int fd, ret = -1;
bool need_swap = false, elf32;
@@ -102,7 +102,7 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
void *phdr, *buf = NULL;
ssize_t phdr_size, ehdr_size, buf_size = 0;
- fd = open(filename, O_RDONLY);
+ fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
if (fd < 0)
return -1;
@@ -323,7 +323,7 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
if (ret >= 0)
RC_CHK_ACCESS(dso)->is_64_bit = ret;
- if (filename__read_build_id(ss->name, &bid) > 0)
+ if (filename__read_build_id(ss->name, &bid, /*block=*/true) > 0)
dso__set_build_id(dso, &bid);
return 0;
}
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index e816e4220d33..3fed54de5401 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1869,14 +1869,14 @@ int dso__load(struct dso *dso, struct map *map)
/*
* Read the build id if possible. This is required for
- * DSO_BINARY_TYPE__BUILDID_DEBUGINFO to work
+ * DSO_BINARY_TYPE__BUILDID_DEBUGINFO to work. Don't block in case path
+ * isn't for a regular file.
*/
- if (!dso__has_build_id(dso) &&
- is_regular_file(dso__long_name(dso))) {
+ if (!dso__has_build_id(dso)) {
struct build_id bid = { .size = 0, };
__symbol__join_symfs(name, PATH_MAX, dso__long_name(dso));
- if (filename__read_build_id(name, &bid) > 0)
+ if (filename__read_build_id(name, &bid, /*block=*/false) > 0)
dso__set_build_id(dso, &bid);
}
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 3fb5d146d9b1..347106218799 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -140,7 +140,7 @@ struct symbol *dso__next_symbol(struct symbol *sym);
enum dso_type dso__type_fd(int fd);
-int filename__read_build_id(const char *filename, struct build_id *id);
+int filename__read_build_id(const char *filename, struct build_id *id, bool block);
int sysfs__read_build_id(const char *filename, struct build_id *bid);
int modules__parse(const char *filename, void *arg,
int (*process_module)(void *arg, const char *name,
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index cb2c1ace304a..fcd1fd13c30e 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -401,7 +401,7 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
nsi = nsinfo__new(event->pid);
nsinfo__mountns_enter(nsi, &nc);
- rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
+ rc = filename__read_build_id(event->filename, &bid, /*block=*/false) > 0 ? 0 : -1;
nsinfo__mountns_exit(&nc);
nsinfo__put(nsi);
--
2.51.0.rc2.233.g662b1ed5c5-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread