* [PATCH] perf tools: Don't read build-ids from non-regular files
@ 2025-11-21 14:20 James Clark
2025-11-21 17:13 ` Ian Rogers
0 siblings, 1 reply; 3+ messages in thread
From: James Clark @ 2025-11-21 14:20 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter
Cc: linux-perf-users, linux-kernel, James Clark
Simplify the build ID reading code by removing the non-blocking option.
Having to pass the correct option to this function was fragile and a
mistake would result in a hang, see the linked fix. Furthermore,
compressed files are always opened blocking anyway, ignoring the
non-blocking option.
We also don't expect to read build IDs from non-regular files. The only
hits to this function that are non-regular are devices that won't be elf
files with build IDs, for example "/dev/dri/renderD129".
Now instead of opening these as non-blocking and failing to read, we
skip them. Even if something like a pipe or character device did have a
build ID, I don't think it would have worked because you need to call
read() in a loop, check for -EAGAIN and handle timeouts to make
non-blocking reads work.
Link: https://lore.kernel.org/linux-perf-users/20251022-james-perf-fix-dso-block-v1-1-c4faab150546@linaro.org/
Signed-off-by: James Clark <james.clark@linaro.org>
---
I looked at the paths for hits to this function and found the following
categories:
Devices, which non-block reads were unsuccessfully attempted, but are
now skipped:
* /dev/dri/renderD129
Non-valid paths, where 'non-regular' is irrelevant e.g.:
* [vdso]
* ... (deleted)
* anon_inode:i915.gem
Shared memory. These are regular and blocking reads work, so no changes
there:
* /dev/shm/.org.chromium.Chromium.ejhzXY
Symlinks to regular files. Also no change as is_regular_file() is true
for these too.
---
tools/perf/bench/inject-buildid.c | 2 +-
tools/perf/builtin-buildid-cache.c | 8 ++++----
tools/perf/builtin-inject.c | 4 ++--
tools/perf/tests/pe-file-parsing.c | 4 ++--
tools/perf/tests/sdt.c | 2 +-
tools/perf/util/build-id.c | 4 ++--
tools/perf/util/debuginfo.c | 2 +-
tools/perf/util/dsos.c | 4 ++--
tools/perf/util/libbfd.c | 4 ++--
tools/perf/util/libbfd.h | 5 ++---
tools/perf/util/symbol-elf.c | 13 ++++++-------
tools/perf/util/symbol-minimal.c | 9 ++++++---
tools/perf/util/symbol.c | 5 ++---
tools/perf/util/symbol.h | 2 +-
tools/perf/util/synthetic-events.c | 2 +-
15 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/tools/perf/bench/inject-buildid.c b/tools/perf/bench/inject-buildid.c
index 12387ea88b9a..aad572a78d7f 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, /*block=*/true) < 0)
+ if (filename__read_build_id(fpath, &bid) < 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 2e0f2004696a..c98104481c8a 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, /*block=*/true);
+ err = filename__read_build_id(filename, &bid);
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, /*block=*/true);
+ err = filename__read_build_id(filename, &bid);
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, /*block=*/true) == -1) {
+ if (filename__read_build_id(filename, &bid) == -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, /*block=*/true);
+ err = filename__read_build_id(filename, &bid);
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 bd9245d2dd41..0e40f2a4bd18 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -668,12 +668,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, /*block=*/true) > 0)
+ if (filename__read_build_id(dso__long_name(dso), &bid) > 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, /*block=*/true) > 0)
+ if (new_name && filename__read_build_id(new_name, &bid) > 0)
dso__set_build_id(dso, &bid);
free(new_name);
}
diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
index 8b31d1d05f90..30c7da79e109 100644
--- a/tools/perf/tests/pe-file-parsing.c
+++ b/tools/perf/tests/pe-file-parsing.c
@@ -37,7 +37,7 @@ static int run_dir(const char *d)
size_t idx;
scnprintf(filename, PATH_MAX, "%s/pe-file.exe", d);
- ret = filename__read_build_id(filename, &bid, /*block=*/true);
+ ret = filename__read_build_id(filename, &bid);
TEST_ASSERT_VAL("Failed to read build_id",
ret == sizeof(expect_build_id));
TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,
@@ -49,7 +49,7 @@ static int run_dir(const char *d)
!strcmp(debuglink, expect_debuglink));
scnprintf(debugfile, PATH_MAX, "%s/%s", d, debuglink);
- ret = filename__read_build_id(debugfile, &bid, /*block=*/true);
+ ret = filename__read_build_id(debugfile, &bid);
TEST_ASSERT_VAL("Failed to read debug file build_id",
ret == sizeof(expect_build_id));
TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,
diff --git a/tools/perf/tests/sdt.c b/tools/perf/tests/sdt.c
index 6132f1af3e22..93baee2eae42 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, /*block=*/true);
+ err = filename__read_build_id(filename, &bid);
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 35505a1ffd11..fdb35133fde4 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -122,7 +122,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, /*block=*/true);
+ ret = filename__read_build_id(pathname, &bid);
if (ret < 0)
return ret;
@@ -848,7 +848,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, /*block=*/true);
+ ret = filename__read_build_id(filename, bid);
nsinfo__mountns_exit(&nsc);
return ret;
diff --git a/tools/perf/util/debuginfo.c b/tools/perf/util/debuginfo.c
index bb9ebd84ec2d..4a559b3e8cdc 100644
--- a/tools/perf/util/debuginfo.c
+++ b/tools/perf/util/debuginfo.c
@@ -115,7 +115,7 @@ struct debuginfo *debuginfo__new(const char *path)
* 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)
+ if (filename__read_build_id(path, &bid) > 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 64c1d65b0149..0a7645c7fae7 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, /*block=*/true) > 0) {
+ if (filename__read_build_id(dso__long_name(dso), &bid) > 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, /*block=*/true) > 0) {
+ if (new_name && filename__read_build_id(new_name, &bid) > 0) {
dso__set_build_id(dso, &bid);
args->have_build_id = true;
}
diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
index 01147fbf73b3..91b431eae0d7 100644
--- a/tools/perf/util/libbfd.c
+++ b/tools/perf/util/libbfd.c
@@ -383,13 +383,13 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
return err;
}
-int libbfd__read_build_id(const char *filename, struct build_id *bid, bool block)
+int libbfd__read_build_id(const char *filename, struct build_id *bid)
{
size_t size = sizeof(bid->data);
int err = -1, fd;
bfd *abfd;
- fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
+ fd = open(filename, O_RDONLY);
if (fd < 0)
return -1;
diff --git a/tools/perf/util/libbfd.h b/tools/perf/util/libbfd.h
index e300f171d1bd..953886f3d62f 100644
--- a/tools/perf/util/libbfd.h
+++ b/tools/perf/util/libbfd.h
@@ -25,7 +25,7 @@ void dso__free_a2l_libbfd(struct dso *dso);
int symbol__disassemble_libbfd(const char *filename, struct symbol *sym,
struct annotate_args *args);
-int libbfd__read_build_id(const char *filename, struct build_id *bid, bool block);
+int libbfd__read_build_id(const char *filename, struct build_id *bid);
int libbfd_filename__read_debuglink(const char *filename, char *debuglink, size_t size);
@@ -59,8 +59,7 @@ static inline int symbol__disassemble_libbfd(const char *filename __always_unuse
}
static inline int libbfd__read_build_id(const char *filename __always_unused,
- struct build_id *bid __always_unused,
- bool block __always_unused)
+ struct build_id *bid __always_unused)
{
return -1;
}
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 9e820599bab3..6b35c5bec610 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -860,20 +860,20 @@ static int elf_read_build_id(Elf *elf, void *bf, size_t size)
return err;
}
-static int read_build_id(const char *filename, struct build_id *bid, bool block)
+static int read_build_id(const char *filename, struct build_id *bid)
{
size_t size = sizeof(bid->data);
int fd, err;
Elf *elf;
- err = libbfd__read_build_id(filename, bid, block);
+ err = libbfd__read_build_id(filename, bid);
if (err >= 0)
goto out;
if (size < BUILD_ID_SIZE)
goto out;
- fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
+ fd = open(filename, O_RDONLY);
if (fd < 0)
goto out;
@@ -894,13 +894,13 @@ static int read_build_id(const char *filename, struct build_id *bid, bool block)
return err;
}
-int filename__read_build_id(const char *filename, struct build_id *bid, bool block)
+int filename__read_build_id(const char *filename, struct build_id *bid)
{
struct kmod_path m = { .name = NULL, };
char path[PATH_MAX];
int err;
- if (!filename)
+ if (!filename || !is_regular_file(filename))
return -EFAULT;
err = kmod_path__parse(&m, filename);
@@ -918,10 +918,9 @@ int filename__read_build_id(const char *filename, struct build_id *bid, bool blo
}
close(fd);
filename = path;
- block = true;
}
- err = read_build_id(filename, bid, block);
+ err = read_build_id(filename, bid);
if (m.comp)
unlink(filename);
diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index aeb253248895..b811db4157ab 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, bool block)
+int filename__read_build_id(const char *filename, struct build_id *bid)
{
int fd, ret = -1;
bool need_swap = false, elf32;
@@ -102,7 +102,10 @@ int filename__read_build_id(const char *filename, struct build_id *bid, bool blo
void *phdr, *buf = NULL;
ssize_t phdr_size, ehdr_size, buf_size = 0;
- fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
+ if (!filename || !is_regular_file(filename))
+ return -EFAULT;
+
+ fd = open(filename, O_RDONLY);
if (fd < 0)
return -1;
@@ -323,7 +326,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, /*block=*/true) > 0)
+ if (filename__read_build_id(ss->name, &bid) > 0)
dso__set_build_id(dso, &bid);
return 0;
}
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index cc26b7bf302b..d8fc5ea77f84 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1743,14 +1743,13 @@ 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. Don't block in case path
- * isn't for a regular file.
+ * DSO_BINARY_TYPE__BUILDID_DEBUGINFO to work.
*/
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, /*block=*/false) > 0)
+ if (filename__read_build_id(name, &bid) > 0)
dso__set_build_id(dso, &bid);
}
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 347106218799..3fb5d146d9b1 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, bool block);
+int filename__read_build_id(const char *filename, struct build_id *id);
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 5aa44c4aba62..2ba9fa25e00a 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, /*block=*/false) > 0 ? 0 : -1;
+ rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
nsinfo__mountns_exit(&nc);
nsinfo__put(nsi);
---
base-commit: 134852abcbfa5f6a4a17619c8f44c857f800ae03
change-id: 20251119-james-perf-non-block-3296c96909aa
Best regards,
--
James Clark <james.clark@linaro.org>
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] perf tools: Don't read build-ids from non-regular files
2025-11-21 14:20 [PATCH] perf tools: Don't read build-ids from non-regular files James Clark
@ 2025-11-21 17:13 ` Ian Rogers
2025-11-24 10:26 ` James Clark
0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2025-11-21 17:13 UTC (permalink / raw)
To: James Clark
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Adrian Hunter, linux-perf-users, linux-kernel
On Fri, Nov 21, 2025 at 6:21 AM James Clark <james.clark@linaro.org> wrote:
>
> Simplify the build ID reading code by removing the non-blocking option.
> Having to pass the correct option to this function was fragile and a
> mistake would result in a hang, see the linked fix. Furthermore,
> compressed files are always opened blocking anyway, ignoring the
> non-blocking option.
>
> We also don't expect to read build IDs from non-regular files. The only
> hits to this function that are non-regular are devices that won't be elf
> files with build IDs, for example "/dev/dri/renderD129".
>
> Now instead of opening these as non-blocking and failing to read, we
> skip them. Even if something like a pipe or character device did have a
> build ID, I don't think it would have worked because you need to call
> read() in a loop, check for -EAGAIN and handle timeouts to make
> non-blocking reads work.
>
> Link: https://lore.kernel.org/linux-perf-users/20251022-james-perf-fix-dso-block-v1-1-c4faab150546@linaro.org/
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> I looked at the paths for hits to this function and found the following
> categories:
>
> Devices, which non-block reads were unsuccessfully attempted, but are
> now skipped:
> * /dev/dri/renderD129
>
> Non-valid paths, where 'non-regular' is irrelevant e.g.:
> * [vdso]
> * ... (deleted)
> * anon_inode:i915.gem
>
> Shared memory. These are regular and blocking reads work, so no changes
> there:
> * /dev/shm/.org.chromium.Chromium.ejhzXY
>
> Symlinks to regular files. Also no change as is_regular_file() is true
> for these too.
> ---
> tools/perf/bench/inject-buildid.c | 2 +-
> tools/perf/builtin-buildid-cache.c | 8 ++++----
> tools/perf/builtin-inject.c | 4 ++--
> tools/perf/tests/pe-file-parsing.c | 4 ++--
> tools/perf/tests/sdt.c | 2 +-
> tools/perf/util/build-id.c | 4 ++--
> tools/perf/util/debuginfo.c | 2 +-
> tools/perf/util/dsos.c | 4 ++--
> tools/perf/util/libbfd.c | 4 ++--
> tools/perf/util/libbfd.h | 5 ++---
> tools/perf/util/symbol-elf.c | 13 ++++++-------
> tools/perf/util/symbol-minimal.c | 9 ++++++---
> tools/perf/util/symbol.c | 5 ++---
> tools/perf/util/symbol.h | 2 +-
> tools/perf/util/synthetic-events.c | 2 +-
> 15 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/tools/perf/bench/inject-buildid.c b/tools/perf/bench/inject-buildid.c
> index 12387ea88b9a..aad572a78d7f 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, /*block=*/true) < 0)
> + if (filename__read_build_id(fpath, &bid) < 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 2e0f2004696a..c98104481c8a 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, /*block=*/true);
> + err = filename__read_build_id(filename, &bid);
> 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, /*block=*/true);
> + err = filename__read_build_id(filename, &bid);
> 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, /*block=*/true) == -1) {
> + if (filename__read_build_id(filename, &bid) == -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, /*block=*/true);
> + err = filename__read_build_id(filename, &bid);
> 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 bd9245d2dd41..0e40f2a4bd18 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -668,12 +668,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, /*block=*/true) > 0)
> + if (filename__read_build_id(dso__long_name(dso), &bid) > 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, /*block=*/true) > 0)
> + if (new_name && filename__read_build_id(new_name, &bid) > 0)
> dso__set_build_id(dso, &bid);
> free(new_name);
> }
> diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> index 8b31d1d05f90..30c7da79e109 100644
> --- a/tools/perf/tests/pe-file-parsing.c
> +++ b/tools/perf/tests/pe-file-parsing.c
> @@ -37,7 +37,7 @@ static int run_dir(const char *d)
> size_t idx;
>
> scnprintf(filename, PATH_MAX, "%s/pe-file.exe", d);
> - ret = filename__read_build_id(filename, &bid, /*block=*/true);
> + ret = filename__read_build_id(filename, &bid);
> TEST_ASSERT_VAL("Failed to read build_id",
> ret == sizeof(expect_build_id));
> TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,
> @@ -49,7 +49,7 @@ static int run_dir(const char *d)
> !strcmp(debuglink, expect_debuglink));
>
> scnprintf(debugfile, PATH_MAX, "%s/%s", d, debuglink);
> - ret = filename__read_build_id(debugfile, &bid, /*block=*/true);
> + ret = filename__read_build_id(debugfile, &bid);
> TEST_ASSERT_VAL("Failed to read debug file build_id",
> ret == sizeof(expect_build_id));
> TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,
> diff --git a/tools/perf/tests/sdt.c b/tools/perf/tests/sdt.c
> index 6132f1af3e22..93baee2eae42 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, /*block=*/true);
> + err = filename__read_build_id(filename, &bid);
> 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 35505a1ffd11..fdb35133fde4 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -122,7 +122,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, /*block=*/true);
> + ret = filename__read_build_id(pathname, &bid);
> if (ret < 0)
> return ret;
>
> @@ -848,7 +848,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, /*block=*/true);
> + ret = filename__read_build_id(filename, bid);
> nsinfo__mountns_exit(&nsc);
>
> return ret;
> diff --git a/tools/perf/util/debuginfo.c b/tools/perf/util/debuginfo.c
> index bb9ebd84ec2d..4a559b3e8cdc 100644
> --- a/tools/perf/util/debuginfo.c
> +++ b/tools/perf/util/debuginfo.c
> @@ -115,7 +115,7 @@ struct debuginfo *debuginfo__new(const char *path)
> * 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)
> + if (filename__read_build_id(path, &bid) > 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 64c1d65b0149..0a7645c7fae7 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, /*block=*/true) > 0) {
> + if (filename__read_build_id(dso__long_name(dso), &bid) > 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, /*block=*/true) > 0) {
> + if (new_name && filename__read_build_id(new_name, &bid) > 0) {
> dso__set_build_id(dso, &bid);
> args->have_build_id = true;
> }
> diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
> index 01147fbf73b3..91b431eae0d7 100644
> --- a/tools/perf/util/libbfd.c
> +++ b/tools/perf/util/libbfd.c
> @@ -383,13 +383,13 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
> return err;
> }
>
> -int libbfd__read_build_id(const char *filename, struct build_id *bid, bool block)
> +int libbfd__read_build_id(const char *filename, struct build_id *bid)
> {
> size_t size = sizeof(bid->data);
> int err = -1, fd;
> bfd *abfd;
>
> - fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
> + fd = open(filename, O_RDONLY);
We will need the is_regular_file test here.
> if (fd < 0)
> return -1;
>
> diff --git a/tools/perf/util/libbfd.h b/tools/perf/util/libbfd.h
> index e300f171d1bd..953886f3d62f 100644
> --- a/tools/perf/util/libbfd.h
> +++ b/tools/perf/util/libbfd.h
> @@ -25,7 +25,7 @@ void dso__free_a2l_libbfd(struct dso *dso);
> int symbol__disassemble_libbfd(const char *filename, struct symbol *sym,
> struct annotate_args *args);
>
> -int libbfd__read_build_id(const char *filename, struct build_id *bid, bool block);
> +int libbfd__read_build_id(const char *filename, struct build_id *bid);
>
> int libbfd_filename__read_debuglink(const char *filename, char *debuglink, size_t size);
>
> @@ -59,8 +59,7 @@ static inline int symbol__disassemble_libbfd(const char *filename __always_unuse
> }
>
> static inline int libbfd__read_build_id(const char *filename __always_unused,
> - struct build_id *bid __always_unused,
> - bool block __always_unused)
> + struct build_id *bid __always_unused)
> {
> return -1;
> }
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 9e820599bab3..6b35c5bec610 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -860,20 +860,20 @@ static int elf_read_build_id(Elf *elf, void *bf, size_t size)
> return err;
> }
>
> -static int read_build_id(const char *filename, struct build_id *bid, bool block)
> +static int read_build_id(const char *filename, struct build_id *bid)
> {
> size_t size = sizeof(bid->data);
> int fd, err;
> Elf *elf;
>
> - err = libbfd__read_build_id(filename, bid, block);
> + err = libbfd__read_build_id(filename, bid);
> if (err >= 0)
> goto out;
>
> if (size < BUILD_ID_SIZE)
> goto out;
>
> - fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
> + fd = open(filename, O_RDONLY);
> if (fd < 0)
> goto out;
>
> @@ -894,13 +894,13 @@ static int read_build_id(const char *filename, struct build_id *bid, bool block)
> return err;
> }
>
> -int filename__read_build_id(const char *filename, struct build_id *bid, bool block)
> +int filename__read_build_id(const char *filename, struct build_id *bid)
> {
> struct kmod_path m = { .name = NULL, };
> char path[PATH_MAX];
> int err;
>
> - if (!filename)
> + if (!filename || !is_regular_file(filename))
> return -EFAULT;
I wonder if -EWOULDBLOCK would be a better return code for the
!is_regular_file test?
>
> err = kmod_path__parse(&m, filename);
> @@ -918,10 +918,9 @@ int filename__read_build_id(const char *filename, struct build_id *bid, bool blo
> }
> close(fd);
> filename = path;
> - block = true;
> }
>
> - err = read_build_id(filename, bid, block);
> + err = read_build_id(filename, bid);
>
> if (m.comp)
> unlink(filename);
> diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
> index aeb253248895..b811db4157ab 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, bool block)
> +int filename__read_build_id(const char *filename, struct build_id *bid)
> {
> int fd, ret = -1;
> bool need_swap = false, elf32;
> @@ -102,7 +102,10 @@ int filename__read_build_id(const char *filename, struct build_id *bid, bool blo
> void *phdr, *buf = NULL;
> ssize_t phdr_size, ehdr_size, buf_size = 0;
>
> - fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
> + if (!filename || !is_regular_file(filename))
> + return -EFAULT;
Same here.
Thanks,
Ian
> +
> + fd = open(filename, O_RDONLY);
> if (fd < 0)
> return -1;
>
> @@ -323,7 +326,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, /*block=*/true) > 0)
> + if (filename__read_build_id(ss->name, &bid) > 0)
> dso__set_build_id(dso, &bid);
> return 0;
> }
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index cc26b7bf302b..d8fc5ea77f84 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1743,14 +1743,13 @@ 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. Don't block in case path
> - * isn't for a regular file.
> + * DSO_BINARY_TYPE__BUILDID_DEBUGINFO to work.
> */
> 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, /*block=*/false) > 0)
> + if (filename__read_build_id(name, &bid) > 0)
> dso__set_build_id(dso, &bid);
> }
>
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 347106218799..3fb5d146d9b1 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, bool block);
> +int filename__read_build_id(const char *filename, struct build_id *id);
> 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 5aa44c4aba62..2ba9fa25e00a 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, /*block=*/false) > 0 ? 0 : -1;
> + rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
>
> nsinfo__mountns_exit(&nc);
> nsinfo__put(nsi);
>
> ---
> base-commit: 134852abcbfa5f6a4a17619c8f44c857f800ae03
> change-id: 20251119-james-perf-non-block-3296c96909aa
>
> Best regards,
> --
> James Clark <james.clark@linaro.org>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] perf tools: Don't read build-ids from non-regular files
2025-11-21 17:13 ` Ian Rogers
@ 2025-11-24 10:26 ` James Clark
0 siblings, 0 replies; 3+ messages in thread
From: James Clark @ 2025-11-24 10:26 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Adrian Hunter, linux-perf-users, linux-kernel
On 21/11/2025 5:13 pm, Ian Rogers wrote:
> On Fri, Nov 21, 2025 at 6:21 AM James Clark <james.clark@linaro.org> wrote:
>>
>> Simplify the build ID reading code by removing the non-blocking option.
>> Having to pass the correct option to this function was fragile and a
>> mistake would result in a hang, see the linked fix. Furthermore,
>> compressed files are always opened blocking anyway, ignoring the
>> non-blocking option.
>>
>> We also don't expect to read build IDs from non-regular files. The only
>> hits to this function that are non-regular are devices that won't be elf
>> files with build IDs, for example "/dev/dri/renderD129".
>>
>> Now instead of opening these as non-blocking and failing to read, we
>> skip them. Even if something like a pipe or character device did have a
>> build ID, I don't think it would have worked because you need to call
>> read() in a loop, check for -EAGAIN and handle timeouts to make
>> non-blocking reads work.
>>
>> Link: https://lore.kernel.org/linux-perf-users/20251022-james-perf-fix-dso-block-v1-1-c4faab150546@linaro.org/
>> Signed-off-by: James Clark <james.clark@linaro.org>
>> ---
>> I looked at the paths for hits to this function and found the following
>> categories:
>>
>> Devices, which non-block reads were unsuccessfully attempted, but are
>> now skipped:
>> * /dev/dri/renderD129
>>
>> Non-valid paths, where 'non-regular' is irrelevant e.g.:
>> * [vdso]
>> * ... (deleted)
>> * anon_inode:i915.gem
>>
>> Shared memory. These are regular and blocking reads work, so no changes
>> there:
>> * /dev/shm/.org.chromium.Chromium.ejhzXY
>>
>> Symlinks to regular files. Also no change as is_regular_file() is true
>> for these too.
>> ---
>> tools/perf/bench/inject-buildid.c | 2 +-
>> tools/perf/builtin-buildid-cache.c | 8 ++++----
>> tools/perf/builtin-inject.c | 4 ++--
>> tools/perf/tests/pe-file-parsing.c | 4 ++--
>> tools/perf/tests/sdt.c | 2 +-
>> tools/perf/util/build-id.c | 4 ++--
>> tools/perf/util/debuginfo.c | 2 +-
>> tools/perf/util/dsos.c | 4 ++--
>> tools/perf/util/libbfd.c | 4 ++--
>> tools/perf/util/libbfd.h | 5 ++---
>> tools/perf/util/symbol-elf.c | 13 ++++++-------
>> tools/perf/util/symbol-minimal.c | 9 ++++++---
>> tools/perf/util/symbol.c | 5 ++---
>> tools/perf/util/symbol.h | 2 +-
>> tools/perf/util/synthetic-events.c | 2 +-
>> 15 files changed, 35 insertions(+), 35 deletions(-)
>>
>> diff --git a/tools/perf/bench/inject-buildid.c b/tools/perf/bench/inject-buildid.c
>> index 12387ea88b9a..aad572a78d7f 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, /*block=*/true) < 0)
>> + if (filename__read_build_id(fpath, &bid) < 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 2e0f2004696a..c98104481c8a 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, /*block=*/true);
>> + err = filename__read_build_id(filename, &bid);
>> 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, /*block=*/true);
>> + err = filename__read_build_id(filename, &bid);
>> 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, /*block=*/true) == -1) {
>> + if (filename__read_build_id(filename, &bid) == -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, /*block=*/true);
>> + err = filename__read_build_id(filename, &bid);
>> 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 bd9245d2dd41..0e40f2a4bd18 100644
>> --- a/tools/perf/builtin-inject.c
>> +++ b/tools/perf/builtin-inject.c
>> @@ -668,12 +668,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, /*block=*/true) > 0)
>> + if (filename__read_build_id(dso__long_name(dso), &bid) > 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, /*block=*/true) > 0)
>> + if (new_name && filename__read_build_id(new_name, &bid) > 0)
>> dso__set_build_id(dso, &bid);
>> free(new_name);
>> }
>> diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
>> index 8b31d1d05f90..30c7da79e109 100644
>> --- a/tools/perf/tests/pe-file-parsing.c
>> +++ b/tools/perf/tests/pe-file-parsing.c
>> @@ -37,7 +37,7 @@ static int run_dir(const char *d)
>> size_t idx;
>>
>> scnprintf(filename, PATH_MAX, "%s/pe-file.exe", d);
>> - ret = filename__read_build_id(filename, &bid, /*block=*/true);
>> + ret = filename__read_build_id(filename, &bid);
>> TEST_ASSERT_VAL("Failed to read build_id",
>> ret == sizeof(expect_build_id));
>> TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,
>> @@ -49,7 +49,7 @@ static int run_dir(const char *d)
>> !strcmp(debuglink, expect_debuglink));
>>
>> scnprintf(debugfile, PATH_MAX, "%s/%s", d, debuglink);
>> - ret = filename__read_build_id(debugfile, &bid, /*block=*/true);
>> + ret = filename__read_build_id(debugfile, &bid);
>> TEST_ASSERT_VAL("Failed to read debug file build_id",
>> ret == sizeof(expect_build_id));
>> TEST_ASSERT_VAL("Wrong build_id", !memcmp(bid.data, expect_build_id,
>> diff --git a/tools/perf/tests/sdt.c b/tools/perf/tests/sdt.c
>> index 6132f1af3e22..93baee2eae42 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, /*block=*/true);
>> + err = filename__read_build_id(filename, &bid);
>> 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 35505a1ffd11..fdb35133fde4 100644
>> --- a/tools/perf/util/build-id.c
>> +++ b/tools/perf/util/build-id.c
>> @@ -122,7 +122,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, /*block=*/true);
>> + ret = filename__read_build_id(pathname, &bid);
>> if (ret < 0)
>> return ret;
>>
>> @@ -848,7 +848,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, /*block=*/true);
>> + ret = filename__read_build_id(filename, bid);
>> nsinfo__mountns_exit(&nsc);
>>
>> return ret;
>> diff --git a/tools/perf/util/debuginfo.c b/tools/perf/util/debuginfo.c
>> index bb9ebd84ec2d..4a559b3e8cdc 100644
>> --- a/tools/perf/util/debuginfo.c
>> +++ b/tools/perf/util/debuginfo.c
>> @@ -115,7 +115,7 @@ struct debuginfo *debuginfo__new(const char *path)
>> * 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)
>> + if (filename__read_build_id(path, &bid) > 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 64c1d65b0149..0a7645c7fae7 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, /*block=*/true) > 0) {
>> + if (filename__read_build_id(dso__long_name(dso), &bid) > 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, /*block=*/true) > 0) {
>> + if (new_name && filename__read_build_id(new_name, &bid) > 0) {
>> dso__set_build_id(dso, &bid);
>> args->have_build_id = true;
>> }
>> diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
>> index 01147fbf73b3..91b431eae0d7 100644
>> --- a/tools/perf/util/libbfd.c
>> +++ b/tools/perf/util/libbfd.c
>> @@ -383,13 +383,13 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
>> return err;
>> }
>>
>> -int libbfd__read_build_id(const char *filename, struct build_id *bid, bool block)
>> +int libbfd__read_build_id(const char *filename, struct build_id *bid)
>> {
>> size_t size = sizeof(bid->data);
>> int err = -1, fd;
>> bfd *abfd;
>>
>> - fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
>> + fd = open(filename, O_RDONLY);
>
> We will need the is_regular_file test here.
>
It's only currently called from places where it's already checked. But
it's a public function, so it makes sense to check again. Will add it.
>> if (fd < 0)
>> return -1;
>>
>> diff --git a/tools/perf/util/libbfd.h b/tools/perf/util/libbfd.h
>> index e300f171d1bd..953886f3d62f 100644
>> --- a/tools/perf/util/libbfd.h
>> +++ b/tools/perf/util/libbfd.h
>> @@ -25,7 +25,7 @@ void dso__free_a2l_libbfd(struct dso *dso);
>> int symbol__disassemble_libbfd(const char *filename, struct symbol *sym,
>> struct annotate_args *args);
>>
>> -int libbfd__read_build_id(const char *filename, struct build_id *bid, bool block);
>> +int libbfd__read_build_id(const char *filename, struct build_id *bid);
>>
>> int libbfd_filename__read_debuglink(const char *filename, char *debuglink, size_t size);
>>
>> @@ -59,8 +59,7 @@ static inline int symbol__disassemble_libbfd(const char *filename __always_unuse
>> }
>>
>> static inline int libbfd__read_build_id(const char *filename __always_unused,
>> - struct build_id *bid __always_unused,
>> - bool block __always_unused)
>> + struct build_id *bid __always_unused)
>> {
>> return -1;
>> }
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 9e820599bab3..6b35c5bec610 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -860,20 +860,20 @@ static int elf_read_build_id(Elf *elf, void *bf, size_t size)
>> return err;
>> }
>>
>> -static int read_build_id(const char *filename, struct build_id *bid, bool block)
>> +static int read_build_id(const char *filename, struct build_id *bid)
>> {
>> size_t size = sizeof(bid->data);
>> int fd, err;
>> Elf *elf;
>>
>> - err = libbfd__read_build_id(filename, bid, block);
>> + err = libbfd__read_build_id(filename, bid);
>> if (err >= 0)
>> goto out;
>>
>> if (size < BUILD_ID_SIZE)
>> goto out;
>>
>> - fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
>> + fd = open(filename, O_RDONLY);
>> if (fd < 0)
>> goto out;
>>
>> @@ -894,13 +894,13 @@ static int read_build_id(const char *filename, struct build_id *bid, bool block)
>> return err;
>> }
>>
>> -int filename__read_build_id(const char *filename, struct build_id *bid, bool block)
>> +int filename__read_build_id(const char *filename, struct build_id *bid)
>> {
>> struct kmod_path m = { .name = NULL, };
>> char path[PATH_MAX];
>> int err;
>>
>> - if (!filename)
>> + if (!filename || !is_regular_file(filename))
>> return -EFAULT;
>
> I wonder if -EWOULDBLOCK would be a better return code for the
> !is_regular_file test?
>
Yep makes sense to me.
Thanks
James
>>
>> err = kmod_path__parse(&m, filename);
>> @@ -918,10 +918,9 @@ int filename__read_build_id(const char *filename, struct build_id *bid, bool blo
>> }
>> close(fd);
>> filename = path;
>> - block = true;
>> }
>>
>> - err = read_build_id(filename, bid, block);
>> + err = read_build_id(filename, bid);
>>
>> if (m.comp)
>> unlink(filename);
>> diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
>> index aeb253248895..b811db4157ab 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, bool block)
>> +int filename__read_build_id(const char *filename, struct build_id *bid)
>> {
>> int fd, ret = -1;
>> bool need_swap = false, elf32;
>> @@ -102,7 +102,10 @@ int filename__read_build_id(const char *filename, struct build_id *bid, bool blo
>> void *phdr, *buf = NULL;
>> ssize_t phdr_size, ehdr_size, buf_size = 0;
>>
>> - fd = open(filename, block ? O_RDONLY : (O_RDONLY | O_NONBLOCK));
>> + if (!filename || !is_regular_file(filename))
>> + return -EFAULT;
>
> Same here.
>
> Thanks,
> Ian
>
>> +
>> + fd = open(filename, O_RDONLY);
>> if (fd < 0)
>> return -1;
>>
>> @@ -323,7 +326,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, /*block=*/true) > 0)
>> + if (filename__read_build_id(ss->name, &bid) > 0)
>> dso__set_build_id(dso, &bid);
>> return 0;
>> }
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index cc26b7bf302b..d8fc5ea77f84 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -1743,14 +1743,13 @@ 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. Don't block in case path
>> - * isn't for a regular file.
>> + * DSO_BINARY_TYPE__BUILDID_DEBUGINFO to work.
>> */
>> 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, /*block=*/false) > 0)
>> + if (filename__read_build_id(name, &bid) > 0)
>> dso__set_build_id(dso, &bid);
>> }
>>
>> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
>> index 347106218799..3fb5d146d9b1 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, bool block);
>> +int filename__read_build_id(const char *filename, struct build_id *id);
>> 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 5aa44c4aba62..2ba9fa25e00a 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, /*block=*/false) > 0 ? 0 : -1;
>> + rc = filename__read_build_id(event->filename, &bid) > 0 ? 0 : -1;
>>
>> nsinfo__mountns_exit(&nc);
>> nsinfo__put(nsi);
>>
>> ---
>> base-commit: 134852abcbfa5f6a4a17619c8f44c857f800ae03
>> change-id: 20251119-james-perf-non-block-3296c96909aa
>>
>> Best regards,
>> --
>> James Clark <james.clark@linaro.org>
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-11-24 10:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-21 14:20 [PATCH] perf tools: Don't read build-ids from non-regular files James Clark
2025-11-21 17:13 ` Ian Rogers
2025-11-24 10:26 ` James Clark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).