All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] perf evsel: Restrict incremental open error unwind
@ 2026-09-12  3:11 Hui Su
  2026-09-12  3:24 ` sashiko-bot
  2026-09-14  1:17 ` Namhyung Kim
  0 siblings, 2 replies; 5+ messages in thread
From: Hui Su @ 2026-09-12  3:11 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Namhyung Kim
  Cc: Ian Rogers, Adrian Hunter, James Clark, Jiri Olsa,
	linux-perf-users, linux-kernel, Hui Su

evsel__open_cpu(), evsel__tool_pmu_open() and
evsel__hwmon_pmu_open() support opening a subrange of a CPU map, bounded
by [start_cpu_map_idx, end_cpu_map_idx).

Their error paths, however, unwind using "while (--idx >= 0)" all the way
back to CPU index 0 regardless of start_cpu_map_idx. If lower CPU indices
were opened by an earlier invocation, a later incremental open failure
therefore closes those existing file descriptors and overwrites their
slots with -1.

Before this fix, running the new incremental open test demonstrates this
corruption:

  $ perf test -v "Tool PMU"
   12: Tool PMU:
   12.1: Tool PMU                                                      : Ok
   12.2: Tool PMU leader                                               : Ok
   12.3: Incremental open error unwind boundary:
     FAILED: CPU 0 FD overwritten: FD(evsel, 0, 0)=-1, expected 4
  FAILED!

Stop the unwind at start_cpu_map_idx so that only file descriptors
opened by the current invocation are rolled back.

After this fix:

  $ perf test -v "Tool PMU"
   12: Tool PMU:
   12.1: Tool PMU                                                      : Ok
   12.2: Tool PMU leader                                               : Ok
   12.3: Incremental open error unwind boundary                        : Ok

Fixes: 4804e0111662 ("perf stat: Use affinity for opening events")
Fixes: 240505b2d0ad ("perf tool_pmu: Factor tool events into their own PMU")
Fixes: 53cc0b351ec9 ("perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs")
Signed-off-by: Hui Su <sh_def@163.com>
---
 tools/perf/tests/tool_pmu.c | 79 +++++++++++++++++++++++++++++++++++++
 tools/perf/util/evsel.c     |  2 +-
 tools/perf/util/hwmon_pmu.c |  2 +-
 tools/perf/util/tool_pmu.c  |  2 +-
 4 files changed, 82 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/tool_pmu.c b/tools/perf/tests/tool_pmu.c
index c6c5ebf0e935..667160f9e2cd 100644
--- a/tools/perf/tests/tool_pmu.c
+++ b/tools/perf/tests/tool_pmu.c
@@ -1,8 +1,16 @@
 // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
+#include <errno.h>
+#include <fcntl.h>
+#include <string.h>
+#include <unistd.h>
+#include <internal/xyarray.h>
+#include "cpumap.h"
 #include "debug.h"
 #include "evlist.h"
+#include "evsel.h"
 #include "parse-events.h"
 #include "tests.h"
+#include "thread_map.h"
 #include "tool_pmu.h"
 
 static int do_test(enum tool_pmu_event ev, bool with_pmu)
@@ -100,9 +108,80 @@ static int test__tool_pmu_with_pmu(struct test_suite *test __maybe_unused,
 	return TEST_OK;
 }
 
+static int test__tool_pmu_incremental_open_unwind(struct test_suite *test __maybe_unused,
+						  int subtest __maybe_unused)
+{
+	struct evlist *evlist = evlist__new();
+	struct parse_events_error err;
+	struct perf_cpu_map *cpus = NULL;
+	struct perf_thread_map *threads = NULL;
+	struct evsel *evsel;
+	int ret = TEST_FAIL, fd0 = -1;
+
+	if (!evlist)
+		return TEST_FAIL;
+
+	parse_events_error__init(&err);
+	if (parse_events(evlist, "tool/user_time/", &err)) {
+		parse_events_error__exit(&err);
+		evlist__put(evlist);
+		return TEST_FAIL;
+	}
+	parse_events_error__exit(&err);
+
+	evsel = evlist__first(evlist);
+	cpus = perf_cpu_map__new("0,1");
+	threads = thread_map__new_by_tid(getpid());
+	if (!cpus || !threads)
+		goto out;
+
+	/* Step 1: Open CPU index 0 successfully */
+	if (evsel__open_per_cpu_and_thread(evsel, cpus, 0, threads) < 0) {
+		pr_debug("Failed to open CPU index 0\n");
+		goto out;
+	}
+
+	fd0 = (*(int *)xyarray__entry(evsel->core.fd, 0, 0));
+	if (fd0 < 0 || fcntl(fd0, F_GETFD) < 0) {
+		pr_debug("CPU index 0 FD is invalid (%d)\n", fd0);
+		goto out;
+	}
+
+	/* Step 2: Intentionally trigger failure on CPU index 1 */
+	evsel->core.attr.sample_period = 1; /* Not supported for tool PMU -> -EINVAL */
+	if (evsel__open_per_cpu_and_thread(evsel, cpus, 1, threads) >= 0) {
+		pr_debug("Unexpected success opening CPU index 1 with sample_period=1\n");
+		goto out;
+	}
+
+	/*
+	 * Step 3: Check that CPU index 0's FD was NOT destroyed by CPU index 1's unwind.
+	 */
+	if ((*(int *)xyarray__entry(evsel->core.fd, 0, 0)) != fd0) {
+		pr_debug("FAILED: CPU 0 FD overwritten: FD(evsel, 0, 0)=%d, expected %d\n",
+			 (*(int *)xyarray__entry(evsel->core.fd, 0, 0)), fd0);
+		goto out;
+	}
+
+	if (fcntl(fd0, F_GETFD) < 0) {
+		pr_debug("FAILED: CPU 0 FD %d was closed by error unwind! errno=%d (%s)\n",
+			 fd0, errno, strerror(errno));
+		goto out;
+	}
+
+	ret = TEST_OK;
+out:
+	evsel__close(evsel);
+	perf_cpu_map__put(cpus);
+	perf_thread_map__put(threads);
+	evlist__put(evlist);
+	return ret;
+}
+
 static struct test_case tests__tool_pmu[] = {
 	TEST_CASE("Parsing without PMU name", tool_pmu_without_pmu),
 	TEST_CASE("Parsing with PMU name", tool_pmu_with_pmu),
+	TEST_CASE("Incremental open error unwind boundary", tool_pmu_incremental_open_unwind),
 	{	.name = NULL, }
 };
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index d4cb455f4a7d..eb429087ebfe 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3127,7 +3127,7 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
 			FD(evsel, idx, thread) = -1;
 		}
 		thread = nthreads;
-	} while (--idx >= 0);
+	} while (--idx >= start_cpu_map_idx);
 	errno = old_errno;
 out:
 	if (err)
diff --git a/tools/perf/util/hwmon_pmu.c b/tools/perf/util/hwmon_pmu.c
index ed544dca70c3..96aeb379e169 100644
--- a/tools/perf/util/hwmon_pmu.c
+++ b/tools/perf/util/hwmon_pmu.c
@@ -803,7 +803,7 @@ int evsel__hwmon_pmu_open(struct evsel *evsel,
 			FD(evsel, idx, thread) = -1;
 		}
 		thread = nthreads;
-	} while (--idx >= 0);
+	} while (--idx >= start_cpu_map_idx);
 	close(dir);
 	return err;
 }
diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
index 5c30854b4644..d2f7591747d9 100644
--- a/tools/perf/util/tool_pmu.c
+++ b/tools/perf/util/tool_pmu.c
@@ -349,7 +349,7 @@ int evsel__tool_pmu_open(struct evsel *evsel,
 			FD(evsel, idx, thread) = -1;
 		}
 		thread = nthreads;
-	} while (--idx >= 0);
+	} while (--idx >= start_cpu_map_idx);
 	errno = old_errno;
 	return err;
 }
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf evsel: Restrict incremental open error unwind
  2026-09-12  3:11 [PATCH] perf evsel: Restrict incremental open error unwind Hui Su
@ 2026-09-12  3:24 ` sashiko-bot
  2026-09-14  1:17 ` Namhyung Kim
  1 sibling, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-09-12  3:24 UTC (permalink / raw)
  To: Hui Su; +Cc: linux-perf-users

> evsel__open_cpu(), evsel__tool_pmu_open() and
> evsel__hwmon_pmu_open() support opening a subrange of a CPU map, bounded
> by [start_cpu_map_idx, end_cpu_map_idx).
> 
> Their error paths, however, unwind using "while (--idx >= 0)" all the way
> back to CPU index 0 regardless of start_cpu_map_idx. If lower CPU indices
> were opened by an earlier invocation, a later incremental open failure
> therefore closes those existing file descriptors and overwrites their
> slots with -1.
> 
> Before this fix, running the new incremental open test demonstrates this
> corruption:
> 
>   $ perf test -v "Tool PMU"
>    12: Tool PMU:
> [ ... ]
> 
> Fixes: 4804e0111662 ("perf stat: Use affinity for opening events")
> Fixes: 240505b2d0ad ("perf tool_pmu: Factor tool events into their own PMU")
> Fixes: 53cc0b351ec9 ("perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs")
> Signed-off-by: Hui Su <sh_def@163.com>

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912031112.1814574-1-sh_def@163.com?part=1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf evsel: Restrict incremental open error unwind
  2026-09-12  3:11 [PATCH] perf evsel: Restrict incremental open error unwind Hui Su
  2026-09-12  3:24 ` sashiko-bot
@ 2026-09-14  1:17 ` Namhyung Kim
  2026-09-21 16:00   ` Ian Rogers
  1 sibling, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2026-09-14  1:17 UTC (permalink / raw)
  To: Hui Su
  Cc: Arnaldo Carvalho de Melo, Ian Rogers, Adrian Hunter, James Clark,
	Jiri Olsa, linux-perf-users, linux-kernel

On Sat, Sep 12, 2026 at 12:11:10PM +0900, Hui Su wrote:
> evsel__open_cpu(), evsel__tool_pmu_open() and
> evsel__hwmon_pmu_open() support opening a subrange of a CPU map, bounded
> by [start_cpu_map_idx, end_cpu_map_idx).
> 
> Their error paths, however, unwind using "while (--idx >= 0)" all the way
> back to CPU index 0 regardless of start_cpu_map_idx. If lower CPU indices
> were opened by an earlier invocation, a later incremental open failure
> therefore closes those existing file descriptors and overwrites their
> slots with -1.
> 
> Before this fix, running the new incremental open test demonstrates this
> corruption:
> 
>   $ perf test -v "Tool PMU"
>    12: Tool PMU:
>    12.1: Tool PMU                                                      : Ok
>    12.2: Tool PMU leader                                               : Ok
>    12.3: Incremental open error unwind boundary:
>      FAILED: CPU 0 FD overwritten: FD(evsel, 0, 0)=-1, expected 4
>   FAILED!
> 
> Stop the unwind at start_cpu_map_idx so that only file descriptors
> opened by the current invocation are rolled back.
> 
> After this fix:
> 
>   $ perf test -v "Tool PMU"
>    12: Tool PMU:
>    12.1: Tool PMU                                                      : Ok
>    12.2: Tool PMU leader                                               : Ok
>    12.3: Incremental open error unwind boundary                        : Ok
> 
> Fixes: 4804e0111662 ("perf stat: Use affinity for opening events")
> Fixes: 240505b2d0ad ("perf tool_pmu: Factor tool events into their own PMU")
> Fixes: 53cc0b351ec9 ("perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs")

Maybe we just need to last one?

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> Signed-off-by: Hui Su <sh_def@163.com>
> ---
>  tools/perf/tests/tool_pmu.c | 79 +++++++++++++++++++++++++++++++++++++
>  tools/perf/util/evsel.c     |  2 +-
>  tools/perf/util/hwmon_pmu.c |  2 +-
>  tools/perf/util/tool_pmu.c  |  2 +-
>  4 files changed, 82 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/tests/tool_pmu.c b/tools/perf/tests/tool_pmu.c
> index c6c5ebf0e935..667160f9e2cd 100644
> --- a/tools/perf/tests/tool_pmu.c
> +++ b/tools/perf/tests/tool_pmu.c
> @@ -1,8 +1,16 @@
>  // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <internal/xyarray.h>
> +#include "cpumap.h"
>  #include "debug.h"
>  #include "evlist.h"
> +#include "evsel.h"
>  #include "parse-events.h"
>  #include "tests.h"
> +#include "thread_map.h"
>  #include "tool_pmu.h"
>  
>  static int do_test(enum tool_pmu_event ev, bool with_pmu)
> @@ -100,9 +108,80 @@ static int test__tool_pmu_with_pmu(struct test_suite *test __maybe_unused,
>  	return TEST_OK;
>  }
>  
> +static int test__tool_pmu_incremental_open_unwind(struct test_suite *test __maybe_unused,
> +						  int subtest __maybe_unused)
> +{
> +	struct evlist *evlist = evlist__new();
> +	struct parse_events_error err;
> +	struct perf_cpu_map *cpus = NULL;
> +	struct perf_thread_map *threads = NULL;
> +	struct evsel *evsel;
> +	int ret = TEST_FAIL, fd0 = -1;
> +
> +	if (!evlist)
> +		return TEST_FAIL;
> +
> +	parse_events_error__init(&err);
> +	if (parse_events(evlist, "tool/user_time/", &err)) {
> +		parse_events_error__exit(&err);
> +		evlist__put(evlist);
> +		return TEST_FAIL;
> +	}
> +	parse_events_error__exit(&err);
> +
> +	evsel = evlist__first(evlist);
> +	cpus = perf_cpu_map__new("0,1");
> +	threads = thread_map__new_by_tid(getpid());
> +	if (!cpus || !threads)
> +		goto out;
> +
> +	/* Step 1: Open CPU index 0 successfully */
> +	if (evsel__open_per_cpu_and_thread(evsel, cpus, 0, threads) < 0) {
> +		pr_debug("Failed to open CPU index 0\n");
> +		goto out;
> +	}
> +
> +	fd0 = (*(int *)xyarray__entry(evsel->core.fd, 0, 0));
> +	if (fd0 < 0 || fcntl(fd0, F_GETFD) < 0) {
> +		pr_debug("CPU index 0 FD is invalid (%d)\n", fd0);
> +		goto out;
> +	}
> +
> +	/* Step 2: Intentionally trigger failure on CPU index 1 */
> +	evsel->core.attr.sample_period = 1; /* Not supported for tool PMU -> -EINVAL */
> +	if (evsel__open_per_cpu_and_thread(evsel, cpus, 1, threads) >= 0) {
> +		pr_debug("Unexpected success opening CPU index 1 with sample_period=1\n");
> +		goto out;
> +	}
> +
> +	/*
> +	 * Step 3: Check that CPU index 0's FD was NOT destroyed by CPU index 1's unwind.
> +	 */
> +	if ((*(int *)xyarray__entry(evsel->core.fd, 0, 0)) != fd0) {
> +		pr_debug("FAILED: CPU 0 FD overwritten: FD(evsel, 0, 0)=%d, expected %d\n",
> +			 (*(int *)xyarray__entry(evsel->core.fd, 0, 0)), fd0);
> +		goto out;
> +	}
> +
> +	if (fcntl(fd0, F_GETFD) < 0) {
> +		pr_debug("FAILED: CPU 0 FD %d was closed by error unwind! errno=%d (%s)\n",
> +			 fd0, errno, strerror(errno));
> +		goto out;
> +	}
> +
> +	ret = TEST_OK;
> +out:
> +	evsel__close(evsel);
> +	perf_cpu_map__put(cpus);
> +	perf_thread_map__put(threads);
> +	evlist__put(evlist);
> +	return ret;
> +}
> +
>  static struct test_case tests__tool_pmu[] = {
>  	TEST_CASE("Parsing without PMU name", tool_pmu_without_pmu),
>  	TEST_CASE("Parsing with PMU name", tool_pmu_with_pmu),
> +	TEST_CASE("Incremental open error unwind boundary", tool_pmu_incremental_open_unwind),
>  	{	.name = NULL, }
>  };
>  
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d4cb455f4a7d..eb429087ebfe 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3127,7 +3127,7 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
>  			FD(evsel, idx, thread) = -1;
>  		}
>  		thread = nthreads;
> -	} while (--idx >= 0);
> +	} while (--idx >= start_cpu_map_idx);
>  	errno = old_errno;
>  out:
>  	if (err)
> diff --git a/tools/perf/util/hwmon_pmu.c b/tools/perf/util/hwmon_pmu.c
> index ed544dca70c3..96aeb379e169 100644
> --- a/tools/perf/util/hwmon_pmu.c
> +++ b/tools/perf/util/hwmon_pmu.c
> @@ -803,7 +803,7 @@ int evsel__hwmon_pmu_open(struct evsel *evsel,
>  			FD(evsel, idx, thread) = -1;
>  		}
>  		thread = nthreads;
> -	} while (--idx >= 0);
> +	} while (--idx >= start_cpu_map_idx);
>  	close(dir);
>  	return err;
>  }
> diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
> index 5c30854b4644..d2f7591747d9 100644
> --- a/tools/perf/util/tool_pmu.c
> +++ b/tools/perf/util/tool_pmu.c
> @@ -349,7 +349,7 @@ int evsel__tool_pmu_open(struct evsel *evsel,
>  			FD(evsel, idx, thread) = -1;
>  		}
>  		thread = nthreads;
> -	} while (--idx >= 0);
> +	} while (--idx >= start_cpu_map_idx);
>  	errno = old_errno;
>  	return err;
>  }
> -- 
> 2.55.0
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf evsel: Restrict incremental open error unwind
  2026-09-14  1:17 ` Namhyung Kim
@ 2026-09-21 16:00   ` Ian Rogers
  2026-09-22 12:35     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2026-09-21 16:00 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Hui Su, Arnaldo Carvalho de Melo, Adrian Hunter, James Clark,
	Jiri Olsa, linux-perf-users, linux-kernel

On Sun, Sep 13, 2026 at 6:17 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Sat, Sep 12, 2026 at 12:11:10PM +0900, Hui Su wrote:
> > evsel__open_cpu(), evsel__tool_pmu_open() and
> > evsel__hwmon_pmu_open() support opening a subrange of a CPU map, bounded
> > by [start_cpu_map_idx, end_cpu_map_idx).
> >
> > Their error paths, however, unwind using "while (--idx >= 0)" all the way
> > back to CPU index 0 regardless of start_cpu_map_idx. If lower CPU indices
> > were opened by an earlier invocation, a later incremental open failure
> > therefore closes those existing file descriptors and overwrites their
> > slots with -1.
> >
> > Before this fix, running the new incremental open test demonstrates this
> > corruption:
> >
> >   $ perf test -v "Tool PMU"
> >    12: Tool PMU:
> >    12.1: Tool PMU                                                      : Ok
> >    12.2: Tool PMU leader                                               : Ok
> >    12.3: Incremental open error unwind boundary:
> >      FAILED: CPU 0 FD overwritten: FD(evsel, 0, 0)=-1, expected 4
> >   FAILED!
> >
> > Stop the unwind at start_cpu_map_idx so that only file descriptors
> > opened by the current invocation are rolled back.
> >
> > After this fix:
> >
> >   $ perf test -v "Tool PMU"
> >    12: Tool PMU:
> >    12.1: Tool PMU                                                      : Ok
> >    12.2: Tool PMU leader                                               : Ok
> >    12.3: Incremental open error unwind boundary                        : Ok
> >
> > Fixes: 4804e0111662 ("perf stat: Use affinity for opening events")
> > Fixes: 240505b2d0ad ("perf tool_pmu: Factor tool events into their own PMU")
> > Fixes: 53cc0b351ec9 ("perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs")
>
> Maybe we just need to last one?
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> > Signed-off-by: Hui Su <sh_def@163.com>
> > ---
> >  tools/perf/tests/tool_pmu.c | 79 +++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/evsel.c     |  2 +-
> >  tools/perf/util/hwmon_pmu.c |  2 +-
> >  tools/perf/util/tool_pmu.c  |  2 +-
> >  4 files changed, 82 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/tests/tool_pmu.c b/tools/perf/tests/tool_pmu.c
> > index c6c5ebf0e935..667160f9e2cd 100644
> > --- a/tools/perf/tests/tool_pmu.c
> > +++ b/tools/perf/tests/tool_pmu.c
> > @@ -1,8 +1,16 @@
> >  // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <internal/xyarray.h>
> > +#include "cpumap.h"
> >  #include "debug.h"
> >  #include "evlist.h"
> > +#include "evsel.h"
> >  #include "parse-events.h"
> >  #include "tests.h"
> > +#include "thread_map.h"
> >  #include "tool_pmu.h"
> >
> >  static int do_test(enum tool_pmu_event ev, bool with_pmu)
> > @@ -100,9 +108,80 @@ static int test__tool_pmu_with_pmu(struct test_suite *test __maybe_unused,
> >       return TEST_OK;
> >  }
> >
> > +static int test__tool_pmu_incremental_open_unwind(struct test_suite *test __maybe_unused,
> > +                                               int subtest __maybe_unused)
> > +{
> > +     struct evlist *evlist = evlist__new();
> > +     struct parse_events_error err;
> > +     struct perf_cpu_map *cpus = NULL;
> > +     struct perf_thread_map *threads = NULL;
> > +     struct evsel *evsel;
> > +     int ret = TEST_FAIL, fd0 = -1;
> > +
> > +     if (!evlist)
> > +             return TEST_FAIL;
> > +
> > +     parse_events_error__init(&err);
> > +     if (parse_events(evlist, "tool/user_time/", &err)) {
> > +             parse_events_error__exit(&err);
> > +             evlist__put(evlist);
> > +             return TEST_FAIL;
> > +     }
> > +     parse_events_error__exit(&err);
> > +
> > +     evsel = evlist__first(evlist);
> > +     cpus = perf_cpu_map__new("0,1");
> > +     threads = thread_map__new_by_tid(getpid());
> > +     if (!cpus || !threads)
> > +             goto out;
> > +
> > +     /* Step 1: Open CPU index 0 successfully */
> > +     if (evsel__open_per_cpu_and_thread(evsel, cpus, 0, threads) < 0) {
> > +             pr_debug("Failed to open CPU index 0\n");
> > +             goto out;
> > +     }
> > +
> > +     fd0 = (*(int *)xyarray__entry(evsel->core.fd, 0, 0));
> > +     if (fd0 < 0 || fcntl(fd0, F_GETFD) < 0) {
> > +             pr_debug("CPU index 0 FD is invalid (%d)\n", fd0);
> > +             goto out;
> > +     }
> > +
> > +     /* Step 2: Intentionally trigger failure on CPU index 1 */
> > +     evsel->core.attr.sample_period = 1; /* Not supported for tool PMU -> -EINVAL */
> > +     if (evsel__open_per_cpu_and_thread(evsel, cpus, 1, threads) >= 0) {
> > +             pr_debug("Unexpected success opening CPU index 1 with sample_period=1\n");
> > +             goto out;
> > +     }
> > +
> > +     /*
> > +      * Step 3: Check that CPU index 0's FD was NOT destroyed by CPU index 1's unwind.
> > +      */
> > +     if ((*(int *)xyarray__entry(evsel->core.fd, 0, 0)) != fd0) {
> > +             pr_debug("FAILED: CPU 0 FD overwritten: FD(evsel, 0, 0)=%d, expected %d\n",
> > +                      (*(int *)xyarray__entry(evsel->core.fd, 0, 0)), fd0);
> > +             goto out;
> > +     }
> > +
> > +     if (fcntl(fd0, F_GETFD) < 0) {
> > +             pr_debug("FAILED: CPU 0 FD %d was closed by error unwind! errno=%d (%s)\n",
> > +                      fd0, errno, strerror(errno));
> > +             goto out;
> > +     }
> > +
> > +     ret = TEST_OK;
> > +out:
> > +     evsel__close(evsel);
> > +     perf_cpu_map__put(cpus);
> > +     perf_thread_map__put(threads);
> > +     evlist__put(evlist);
> > +     return ret;
> > +}
> > +
> >  static struct test_case tests__tool_pmu[] = {
> >       TEST_CASE("Parsing without PMU name", tool_pmu_without_pmu),
> >       TEST_CASE("Parsing with PMU name", tool_pmu_with_pmu),
> > +     TEST_CASE("Incremental open error unwind boundary", tool_pmu_incremental_open_unwind),
> >       {       .name = NULL, }
> >  };
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index d4cb455f4a7d..eb429087ebfe 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -3127,7 +3127,7 @@ static int evsel__open_cpu(struct evsel *evsel, struct perf_cpu_map *cpus,
> >                       FD(evsel, idx, thread) = -1;
> >               }
> >               thread = nthreads;
> > -     } while (--idx >= 0);
> > +     } while (--idx >= start_cpu_map_idx);
> >       errno = old_errno;
> >  out:
> >       if (err)
> > diff --git a/tools/perf/util/hwmon_pmu.c b/tools/perf/util/hwmon_pmu.c
> > index ed544dca70c3..96aeb379e169 100644
> > --- a/tools/perf/util/hwmon_pmu.c
> > +++ b/tools/perf/util/hwmon_pmu.c
> > @@ -803,7 +803,7 @@ int evsel__hwmon_pmu_open(struct evsel *evsel,
> >                       FD(evsel, idx, thread) = -1;
> >               }
> >               thread = nthreads;
> > -     } while (--idx >= 0);
> > +     } while (--idx >= start_cpu_map_idx);
> >       close(dir);
> >       return err;
> >  }
> > diff --git a/tools/perf/util/tool_pmu.c b/tools/perf/util/tool_pmu.c
> > index 5c30854b4644..d2f7591747d9 100644
> > --- a/tools/perf/util/tool_pmu.c
> > +++ b/tools/perf/util/tool_pmu.c
> > @@ -349,7 +349,7 @@ int evsel__tool_pmu_open(struct evsel *evsel,
> >                       FD(evsel, idx, thread) = -1;
> >               }
> >               thread = nthreads;
> > -     } while (--idx >= 0);
> > +     } while (--idx >= start_cpu_map_idx);
> >       errno = old_errno;
> >       return err;
> >  }
> > --
> > 2.55.0
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf evsel: Restrict incremental open error unwind
  2026-09-21 16:00   ` Ian Rogers
@ 2026-09-22 12:35     ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2026-09-22 12:35 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Namhyung Kim, Hui Su, Adrian Hunter, James Clark, Jiri Olsa,
	linux-perf-users, linux-kernel

On Mon, Sep 21, 2026 at 09:00:43AM -0700, Ian Rogers wrote:
> On Sun, Sep 13, 2026 at 6:17 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Sat, Sep 12, 2026 at 12:11:10PM +0900, Hui Su wrote:
> > > evsel__open_cpu(), evsel__tool_pmu_open() and
> > > evsel__hwmon_pmu_open() support opening a subrange of a CPU map, bounded
> > > by [start_cpu_map_idx, end_cpu_map_idx).
> > >
> > > Their error paths, however, unwind using "while (--idx >= 0)" all the way
> > > back to CPU index 0 regardless of start_cpu_map_idx. If lower CPU indices
> > > were opened by an earlier invocation, a later incremental open failure
> > > therefore closes those existing file descriptors and overwrites their
> > > slots with -1.
> > >
> > > Before this fix, running the new incremental open test demonstrates this
> > > corruption:
> > >
> > >   $ perf test -v "Tool PMU"
> > >    12: Tool PMU:
> > >    12.1: Tool PMU                                                      : Ok
> > >    12.2: Tool PMU leader                                               : Ok
> > >    12.3: Incremental open error unwind boundary:
> > >      FAILED: CPU 0 FD overwritten: FD(evsel, 0, 0)=-1, expected 4
> > >   FAILED!
> > >
> > > Stop the unwind at start_cpu_map_idx so that only file descriptors
> > > opened by the current invocation are rolled back.
> > >
> > > After this fix:
> > >
> > >   $ perf test -v "Tool PMU"
> > >    12: Tool PMU:
> > >    12.1: Tool PMU                                                      : Ok
> > >    12.2: Tool PMU leader                                               : Ok
> > >    12.3: Incremental open error unwind boundary                        : Ok
> > >
> > > Fixes: 4804e0111662 ("perf stat: Use affinity for opening events")
> > > Fixes: 240505b2d0ad ("perf tool_pmu: Factor tool events into their own PMU")
> > > Fixes: 53cc0b351ec9 ("perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs")
> >
> > Maybe we just need to last one?
> >
> > Acked-by: Namhyung Kim <namhyung@kernel.org>
> 
> Reviewed-by: Ian Rogers <irogers@google.com>

Thanks, applied to perf-tools-next, for v7.4.

- Arnaldo

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-09-22 12:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-12  3:11 [PATCH] perf evsel: Restrict incremental open error unwind Hui Su
2026-09-12  3:24 ` sashiko-bot
2026-09-14  1:17 ` Namhyung Kim
2026-09-21 16:00   ` Ian Rogers
2026-09-22 12:35     ` Arnaldo Carvalho de Melo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.