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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch fails to fix the `-EINVAL` pollfd mismatch abort and introduces dead code because it relies on an invalid assumption about `fdarray__filter()`.
- [Medium] The newly added unit test `test__fdarray__filter` asserts an incorrect invariant about `fdarray__filter()`, causing deterministic test failure.
--

--- Patch [1]: [PATCH v7] perf record: Fix unhandled POLLHUP on non_perf_event descriptors ---
commit c8946dae2d12736f0f97f47367d99aaa40d80c15
Author: Ian Rogers <[email protected]>
Subject: perf record: Fix unhandled POLLHUP on non_perf_event descriptors

This commit attempts to fix an abort during perf record teardown caused by
index mismatches between a thread's localized poll array and the core evlist
poll array when handling POLLHUP. It propagates the finalized -1 state from
the core evlist back to the thread's local array before the equivalence check.
It also adds a unit test to verify that fdarray__filter preserves nonfilterable
items.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index e915390556752..1ddf3ad2808df 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1180,6 +1180,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");

[Severity: High]
Does this fix actually resolve the mismatch? A POLLHUP event on the control
descriptor causes fdarray__filter to unconditionally clear t_entries[t_pos].events
to 0.

Since e_entries has not yet been processed by evlist__ctlfd_process, its events
is still POLLIN. This means the e_entries condition evaluates to false, making
the new block dead code.

Consequently, the strict equivalence check below evaluates to true (POLLIN != 0),
causing the function to abort with -EINVAL.

> diff --git a/tools/perf/tests/fdarray.c b/tools/perf/tests/fdarray.c
> index 40983c3574b12..23860edb8ef09 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!");

[Severity: Medium]
Will this test pass? The fdarray__filter function in tools/lib/api/fd/array.c
actively clears events and revents prior to checking the nonfilterable flag:

if (fda->entries[fd].revents & revents) {
	...
	fda->entries[fd].revents = fda->entries[fd].events = 0;
	continue;
}

if (!(fda->priv[fd].flags & fdarray_flag__nonfilterable))

This implementation contradicts the test's assumption that the fields remain
untouched, causing the fda->entries[2].revents != POLLHUP assertion to
deterministically fail.

> +		goto out_delete;
> +	}
> +
> +	fda->priv[2].flags = 0; /* reset flags */
> +
>  	pr_debug("\n");
>  
>  	err = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1