Re: [PATCH v6] 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 synchronization logic added to `record__update_evlist_pollfd_from_thread` is flawed and fails to prevent the `-EINVAL` index mismatch abort on `POLLHUP`.
- [Medium] The newly added unit test `test__fdarray__filter` fails unconditionally, breaking the perf test suite.
--

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

perf record: Fix unhandled POLLHUP on non_perf_event descriptors

This patch attempts to fix a premature teardown during recording sessions by 
propagating the finalized teardown status (-1) from the core evlist back to 
the thread's local pollfd array when a POLLHUP is received. It also adds a 
unit test to verify that fdarray__filter() properly preserves items marked 
as nonfilterable.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index e915390556752..719869efdde59 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1180,6 +1180,16 @@ 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) {
> +			/*
> +			 * e_entries might have been finalized by evlist__finalize_ctlfd().
> +			 * We must propagate it to t_entries to avoid index mismatches
> +			 * and to prevent a poll storm on the next iteration.
> +			 */
> +			t_entries[t_pos].fd = -1;
> +			t_entries[t_pos].events = 0;
> +		}

[Severity: High]
Does this synchronization logic happen in the correct order to prevent the
-EINVAL index mismatch?

Looking at the main loop in __cmd_record(), fdarray__filter() mutates the
thread's local pollfd array (zeroing events) before
record__update_evlist_pollfd_from_thread() is called:

tools/perf/builtin-record.c:__cmd_record() {
    ...
    if (fdarray__filter(&thread->pollfd, POLLERR | POLLHUP, ...) == 0)
        draining = true;

    err = record__update_evlist_pollfd_from_thread(rec, rec->evlist, thread);
    if (err)
        goto out_child;
    }

    if (evlist__ctlfd_process(rec->evlist, &cmd) > 0) {
    ...
}

Because evlist__ctlfd_process() hasn't finalized the core evlist's array
yet, e_entries[e_pos].events will still be POLLIN, bypassing this new check.

Since the thread's events field was just zeroed by fdarray__filter(),
the strict equivalence check below (POLLIN != 0) will still fail and abort
the session with -EINVAL.

> +
>  		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 40983c3574b12..2d3db7b754a17 100644
> --- a/tools/perf/tests/fdarray.c
> +++ b/tools/perf/tests/fdarray.c
> @@ -80,6 +80,30 @@ static int test__fdarray__filter(struct test_suite *test __maybe_unused, int sub

[ ... ]

> +	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;
> +	}

[Severity: Medium]
Will this test fail unconditionally?

Looking at fdarray__filter() in tools/lib/api/fd/array.c, it appears to
unconditionally clear the events and revents fields for any entry matching
the filter mask before the nonfilterable flag is ever evaluated:

tools/lib/api/fd/array.c:fdarray__filter() {
    ...
    if (fda->entries[fd].revents & revents) {
        if (entry_destructor)
            entry_destructor(fda, fd, arg);

        fda->entries[fd].revents = fda->entries[fd].events = 0;
        continue;
    }
    ...
}

Because revents is unconditionally zeroed, won't this check always evaluate
to true and cause the perf test suite to fail?

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

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