[PATCH v8] perf record: Fix unhandled POLLHUP on non_perf_event descriptors

Ian Rogers posted 1 patch 1 month, 1 week ago
tools/lib/api/fd/array.c    |  4 ++++
tools/perf/builtin-record.c | 15 +++++++++++++++
tools/perf/tests/fdarray.c  | 32 ++++++++++++++++++++++++++++++++
3 files changed, 51 insertions(+)
[PATCH v8] perf record: Fix unhandled POLLHUP on non_perf_event descriptors
Posted by Ian Rogers 1 month, 1 week ago
When processing POLLHUP on a non_perf_event control descriptor,
evlist__finalize_ctlfd() is invoked to finalize the setup, correctly
setting the core evlist's poll array file descriptor to -1. However,
this finalized teardown state is never propagated back to the
individual thread's local replica of the pollfd array.

Consequently, on the next iteration of the main recording loop,
record__update_evlist_pollfd_from_thread() performs a strict
equivalence check between the core evlist's array and the thread's
localized poll array, detecting that the fd values no longer match.
This causes an immediate -EINVAL abort and a premature teardown.

Fix the underlying logic within
record__update_evlist_pollfd_from_thread() to sustainably propagate
the finalized teardown statuses (-1) originating from the core evlist
back to the thread's localized poll structure. This correctly
maintains synchronization and entirely prevents the unhandled
index mismatch crashes.

Additionally, add a unit test that explicitly validates that
fdarray__filter() preserves its invariants regarding
fdarray_flag__nonfilterable items to guard against regressions.

Fixes: fb4751e79c45 ("perf record: Fix teardown hang on system-wide multi-threaded sessions")
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/api/fd/array.c    |  4 ++++
 tools/perf/builtin-record.c | 15 +++++++++++++++
 tools/perf/tests/fdarray.c  | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+)

diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
index 67b73481df27..3200746d1657 100644
--- a/tools/lib/api/fd/array.c
+++ b/tools/lib/api/fd/array.c
@@ -116,6 +116,10 @@ int fdarray__filter(struct fdarray *fda, short revents,
 		return 0;
 
 	for (fd = 0; fd < fda->nr; ++fd) {
+		/*
+		 * Explicitly bypass nonfilterable items (e.g. control descriptors).
+		 * This ensures their fd, events, and revents remain completely untouched.
+		 */
 		if (fda->priv[fd].flags & fdarray_flag__nonfilterable)
 			continue;
 
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index a57987851cf0..ad81458989b5 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1169,6 +1169,21 @@ static int record__update_evlist_pollfd_from_thread(struct record *rec,
 		int e_pos = rec->index_map[i].evlist_pollfd_index;
 		int t_pos = rec->index_map[i].thread_pollfd_index;
 
+		if (e_entries[e_pos].fd == -1 || e_entries[e_pos].events == 0) {
+			/*
+			 * If the control file descriptor was closed, then evlist__ctlfd_process()
+			 * will have called evlist__finalize_ctlfd() on the PREVIOUS loop iteration
+			 * to cleanly set the core evlist's e_entries[e_pos].fd to -1.
+			 *
+			 * Since nonfilterable items are skipped by fdarray__filter(), the
+			 * thread's local t_entries[t_pos] retains its original state.
+			 * We must explicitly propagate the finalized -1 state to t_entries
+			 * BEFORE evaluating the strict equivalence check below.
+			 */
+			t_entries[t_pos].fd = -1;
+			t_entries[t_pos].events = 0;
+		}
+
 		if (e_entries[e_pos].fd != t_entries[t_pos].fd ||
 		    e_entries[e_pos].events != t_entries[t_pos].events) {
 			pr_err("Thread and evlist pollfd index mismatch\n");
diff --git a/tools/perf/tests/fdarray.c b/tools/perf/tests/fdarray.c
index 40983c3574b1..23860edb8ef0 100644
--- a/tools/perf/tests/fdarray.c
+++ b/tools/perf/tests/fdarray.c
@@ -80,6 +80,38 @@ static int test__fdarray__filter(struct test_suite *test __maybe_unused, int sub
 		goto out_delete;
 	}
 
+	fdarray__init_revents(fda, POLLHUP);
+	fda->priv[2].flags = fdarray_flag__nonfilterable;
+
+	pr_debug("\nfiltering all but fda->entries[2] (nonfilterable):");
+	fdarray__fprintf_prefix(fda, "before", stderr);
+
+	/*
+	 * Note: fdarray__filter() in tools/lib/api/fd/array.c evaluates the
+	 * fdarray_flag__nonfilterable flag at the very top of its loop via an
+	 * early continue. Therefore, it completely skips all processing for this
+	 * descriptor, guaranteeing its fd, events, and revents fields remain
+	 * entirely untouched by the filter mask evaluation below it.
+	 */
+	nr_fds = fdarray__filter(fda, POLLHUP, NULL, NULL);
+	fdarray__fprintf_prefix(fda, " after", stderr);
+
+	if (nr_fds != 0) {
+		pr_debug("\nfdarray__filter()=%d != 0, should be 0\n",
+			 nr_fds);
+		goto out_delete;
+	}
+	if (fda->entries[2].fd == -1) {
+		pr_debug("\nfdarray__filter() illegally modified nonfilterable fd!");
+		goto out_delete;
+	}
+	if (fda->entries[2].revents != POLLHUP) {
+		pr_debug("\nfdarray__filter() illegally modified nonfilterable revents!");
+		goto out_delete;
+	}
+
+	fda->priv[2].flags = 0; /* reset flags */
+
 	pr_debug("\n");
 
 	err = 0;
-- 
2.55.0.766.g2966f0265a-goog