1. Patchew
  2. linux
  3. [PATCH v2 00/10] perf: Refactor/add fallbacks for reading build-id and debuglink
  4. View patch

[PATCH v2 10/10] perf symbol: Fix ENOENT case for filename__read_build_id

Ian Rogers posted 10 patches 2 months, 1 week ago
[PATCH v2 10/10] perf symbol: Fix ENOENT case for filename__read_build_id
Posted by Ian Rogers 2 months, 1 week ago 
Some callers of filename__read_build_id assume the error value must be
-1, fix by making them handle all < 0 values.

If is_regular_file fails in filename__read_build_id then it could be
the file is missing (ENOENT) and it would be wrong to return
-EWOULDBLOCK in that case. Fix the logic so -EWOULDBLOCK is only
reported if other errors with stat haven't occurred.

Fixes: 834ebb5678d7 ("perf tools: Don't read build-ids from non-regular files")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-buildid-cache.c | 6 ++++--
 tools/perf/util/symbol.c           | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index c98104481c8a..539e779e3268 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -276,12 +276,14 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
 {
 	char filename[PATH_MAX];
 	struct build_id bid = { .size = 0, };
+	int err;
 
 	if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
 		return true;
 
-	if (filename__read_build_id(filename, &bid) == -1) {
-		if (errno == ENOENT)
+	err = filename__read_build_id(filename, &bid);
+	if (err < 0) {
+		if (err == -ENOENT)
 			return false;
 
 		pr_warning("Problems with %s file, consider removing it from the cache\n",
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 76dc5b70350a..f43e30019e21 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2008,8 +2008,9 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
 	if (!filename)
 		return -EFAULT;
 
+	errno = 0;
 	if (!is_regular_file(filename))
-		return -EWOULDBLOCK;
+		return errno == 0 ? -EWOULDBLOCK : -errno;
 
 	err = kmod_path__parse(&m, filename);
 	if (err)
-- 
2.52.0.158.g65b55ccf14-goog
Re: [PATCH v2 10/10] perf symbol: Fix ENOENT case for filename__read_build_id
Posted by Ian Rogers 2 months ago 
On Mon, Dec 1, 2025 at 12:55 PM Ian Rogers <irogers@google.com> wrote:
>
> Some callers of filename__read_build_id assume the error value must be
> -1, fix by making them handle all < 0 values.
>
> If is_regular_file fails in filename__read_build_id then it could be
> the file is missing (ENOENT) and it would be wrong to return
> -EWOULDBLOCK in that case. Fix the logic so -EWOULDBLOCK is only
> reported if other errors with stat haven't occurred.
>
> Fixes: 834ebb5678d7 ("perf tools: Don't read build-ids from non-regular files")

We might want to prioritize this fix.

> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-buildid-cache.c | 6 ++++--
>  tools/perf/util/symbol.c           | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> index c98104481c8a..539e779e3268 100644
> --- a/tools/perf/builtin-buildid-cache.c
> +++ b/tools/perf/builtin-buildid-cache.c
> @@ -276,12 +276,14 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
>  {
>         char filename[PATH_MAX];
>         struct build_id bid = { .size = 0, };
> +       int err;
>
>         if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
>                 return true;
>
> -       if (filename__read_build_id(filename, &bid) == -1) {

This check here is clearly wrong when -EWOULDBLOCK is returned from
James' change.

> -               if (errno == ENOENT)
> +       err = filename__read_build_id(filename, &bid);
> +       if (err < 0) {
> +               if (err == -ENOENT)
>                         return false;
>
>                 pr_warning("Problems with %s file, consider removing it from the cache\n",
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 76dc5b70350a..f43e30019e21 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -2008,8 +2008,9 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
>         if (!filename)
>                 return -EFAULT;
>
> +       errno = 0;
>         if (!is_regular_file(filename))
> -               return -EWOULDBLOCK;
> +               return errno == 0 ? -EWOULDBLOCK : -errno;

I've made the fix after the other changes as it is simpler to fix in
one filename__read_build_id rather than all the libbfd, .. variants.
If we don't want the series in the short-term perhaps we still want to
carry some parts of this fix.

Thanks,
Ian

>
>         err = kmod_path__parse(&m, filename);
>         if (err)
> --
> 2.52.0.158.g65b55ccf14-goog
>
Re: [PATCH v2 10/10] perf symbol: Fix ENOENT case for filename__read_build_id
Posted by Namhyung Kim 2 months ago 
Hi Ian,

On Fri, Dec 05, 2025 at 11:40:12AM -0800, Ian Rogers wrote:
> On Mon, Dec 1, 2025 at 12:55 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Some callers of filename__read_build_id assume the error value must be
> > -1, fix by making them handle all < 0 values.
> >
> > If is_regular_file fails in filename__read_build_id then it could be
> > the file is missing (ENOENT) and it would be wrong to return
> > -EWOULDBLOCK in that case. Fix the logic so -EWOULDBLOCK is only
> > reported if other errors with stat haven't occurred.
> >
> > Fixes: 834ebb5678d7 ("perf tools: Don't read build-ids from non-regular files")
> 
> We might want to prioritize this fix.

Sure.

> 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/builtin-buildid-cache.c | 6 ++++--
> >  tools/perf/util/symbol.c           | 3 ++-
> >  2 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> > index c98104481c8a..539e779e3268 100644
> > --- a/tools/perf/builtin-buildid-cache.c
> > +++ b/tools/perf/builtin-buildid-cache.c
> > @@ -276,12 +276,14 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
> >  {
> >         char filename[PATH_MAX];
> >         struct build_id bid = { .size = 0, };
> > +       int err;
> >
> >         if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
> >                 return true;
> >
> > -       if (filename__read_build_id(filename, &bid) == -1) {
> 
> This check here is clearly wrong when -EWOULDBLOCK is returned from
> James' change.
> 
> > -               if (errno == ENOENT)
> > +       err = filename__read_build_id(filename, &bid);
> > +       if (err < 0) {
> > +               if (err == -ENOENT)
> >                         return false;
> >
> >                 pr_warning("Problems with %s file, consider removing it from the cache\n",
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index 76dc5b70350a..f43e30019e21 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -2008,8 +2008,9 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
> >         if (!filename)
> >                 return -EFAULT;
> >
> > +       errno = 0;
> >         if (!is_regular_file(filename))
> > -               return -EWOULDBLOCK;
> > +               return errno == 0 ? -EWOULDBLOCK : -errno;
> 
> I've made the fix after the other changes as it is simpler to fix in
> one filename__read_build_id rather than all the libbfd, .. variants.
> If we don't want the series in the short-term perhaps we still want to
> carry some parts of this fix.

Yeah, I hesitate to merge a big series at this moment unless it's a
critical and obvious fix.  Can you please separate this change and send
it out?

Thanks,
Namhyung


> 
> >
> >         err = kmod_path__parse(&m, filename);
> >         if (err)
> > --
> > 2.52.0.158.g65b55ccf14-goog
> >
Re: [PATCH v2 10/10] perf symbol: Fix ENOENT case for filename__read_build_id
Posted by Ian Rogers 2 months ago 
On Fri, Dec 5, 2025 at 1:18 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Fri, Dec 05, 2025 at 11:40:12AM -0800, Ian Rogers wrote:
> > On Mon, Dec 1, 2025 at 12:55 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > Some callers of filename__read_build_id assume the error value must be
> > > -1, fix by making them handle all < 0 values.
> > >
> > > If is_regular_file fails in filename__read_build_id then it could be
> > > the file is missing (ENOENT) and it would be wrong to return
> > > -EWOULDBLOCK in that case. Fix the logic so -EWOULDBLOCK is only
> > > reported if other errors with stat haven't occurred.
> > >
> > > Fixes: 834ebb5678d7 ("perf tools: Don't read build-ids from non-regular files")
> >
> > We might want to prioritize this fix.
>
> Sure.
>
> >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/builtin-buildid-cache.c | 6 ++++--
> > >  tools/perf/util/symbol.c           | 3 ++-
> > >  2 files changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
> > > index c98104481c8a..539e779e3268 100644
> > > --- a/tools/perf/builtin-buildid-cache.c
> > > +++ b/tools/perf/builtin-buildid-cache.c
> > > @@ -276,12 +276,14 @@ static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
> > >  {
> > >         char filename[PATH_MAX];
> > >         struct build_id bid = { .size = 0, };
> > > +       int err;
> > >
> > >         if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
> > >                 return true;
> > >
> > > -       if (filename__read_build_id(filename, &bid) == -1) {
> >
> > This check here is clearly wrong when -EWOULDBLOCK is returned from
> > James' change.
> >
> > > -               if (errno == ENOENT)
> > > +       err = filename__read_build_id(filename, &bid);
> > > +       if (err < 0) {
> > > +               if (err == -ENOENT)
> > >                         return false;
> > >
> > >                 pr_warning("Problems with %s file, consider removing it from the cache\n",
> > > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > > index 76dc5b70350a..f43e30019e21 100644
> > > --- a/tools/perf/util/symbol.c
> > > +++ b/tools/perf/util/symbol.c
> > > @@ -2008,8 +2008,9 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
> > >         if (!filename)
> > >                 return -EFAULT;
> > >
> > > +       errno = 0;
> > >         if (!is_regular_file(filename))
> > > -               return -EWOULDBLOCK;
> > > +               return errno == 0 ? -EWOULDBLOCK : -errno;
> >
> > I've made the fix after the other changes as it is simpler to fix in
> > one filename__read_build_id rather than all the libbfd, .. variants.
> > If we don't want the series in the short-term perhaps we still want to
> > carry some parts of this fix.
>
> Yeah, I hesitate to merge a big series at this moment unless it's a
> critical and obvious fix.  Can you please separate this change and send
> it out?

Sent in:
https://lore.kernel.org/lkml/20251207022345.909535-1-irogers@google.com/

Thanks,
Ian


> Thanks,
> Namhyung
>
>
> >
> > >
> > >         err = kmod_path__parse(&m, filename);
> > >         if (err)
> > > --
> > > 2.52.0.158.g65b55ccf14-goog
> > >

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.