Re: [PATCH v3] perf record: Fix destructor invocation and event counting in fdarray__filter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The patch completely breaks the 'nonfilterable' semantics for system-wide perf events, erroneously causing them to be filtered and their mmap references to be prematurely dropped on POLLHUP.
- [Medium] The patch is based on a fabricated premise regarding the active event count (`nr`) increment, making its core logical changes redundant while introducing regressions.

Pre-existing issues:
- [High] Unhandled POLLHUP on non_perf_event descriptors causes premature termination due to thread and evlist pollfd index mismatch.
--

--- Patch [1]: [PATCH v3] perf record: Fix destructor invocation and event counting in fdarray__filter ---
commit 251ec969368be86f9fe7f805b44d3a39b2228d7f
Author: Ian Rogers <irogers@google.com>

perf record: Fix destructor invocation and event counting in fdarray__filter

This commit modifies fdarray__filter() to change how nonfilterable and non-perf
events are handled during poll events. It removes the early exit for
nonfilterable descriptors and adds a conditional block before incrementing the
active event count.

> When processing POLLHUP or POLLERR for an event in fdarray__filter, the
> function invokes its destructor callback but fails to re-inject the
> nonfilterable exclusion masks into the active event count (nr) increment
> condition. Because auxiliary and control pipe descriptors never enter the
> POLLHUP event match block, their active event count is unconditionally
> incremented, preventing the return value from reaching zero on target
> process exit and trapping __cmd_record() in an infinite drain poll hang.

[Severity: Medium]
Before this patch, descriptors marked with fdarray_flag__nonfilterable hit a
continue at the very top of the loop, skipping the loop body entirely,
including the ++nr increment.

It appears they were never unconditionally incrementing the active event count,
meaning the patch might be attempting to fix a non-existent bug by modifying
the increment conditionally instead of fixing the root cause.

Is the description of the original behavior in the commit message accurate?

> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> index 67b73481df27e..f97afbd6f07ac 100644
> --- a/tools/lib/api/fd/array.c
> +++ b/tools/lib/api/fd/array.c
> @@ -116,9 +116,6 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  		return 0;
>  
>  	for (fd = 0; fd < fda->nr; ++fd) {
> -		if (fda->priv[fd].flags & fdarray_flag__nonfilterable)
> -			continue;
> -

[Severity: High]
System-wide events are assigned fdarray_flag__nonfilterable but not
fdarray_flag__non_perf_event, as seen in mmap_per_evsel():

    flgs = evsel->system_wide ? fdarray_flag__nonfilterable :
           fdarray_flag__default;

Without the early continue, when these events receive a POLLHUP or POLLERR,
they will now fall into the revents block below, prematurely trigger their
destructor (unmapping the perf event), and have their fd set to -1.

Does removing this early continue break the semantics for system-wide perf
events?

>  		if (!fda->entries[fd].events)
>  			continue;
>  
> @@ -126,6 +123,9 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  			if (entry_destructor)
>  				entry_destructor(fda, fd, arg);
>  
> +			if (fda->priv[fd].flags & fdarray_flag__non_perf_event)
> +				continue;
> +

[Severity: High]
This isn't a bug introduced by this patch, but when the ctl_fd receives a
POLLHUP, it skips setting its fd to -1 in the thread's poll array.

Later, evlist__ctlfd_process() handles the command and calls
evlist__finalize_ctlfd(), which explicitly sets the core evlist's poll array
fd to -1.

On the next iteration, a mismatch is detected:

tools/perf/builtin-record.c:record__update_evlist_pollfd_from_thread() {
    ...
    pr_err("Thread and evlist pollfd index mismatch\n");
    err = -EINVAL;
    ...
}

Could this cause perf record to abort with -EINVAL and data loss when a
control descriptor is closed?

>  			/*
>  			 * Set fd to -1 so poll() ignores this entry; otherwise
>  			 * POLLHUP/POLLERR are still reported for events=0 fds
[ ... ]
> @@ -136,7 +136,9 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  			continue;
>  		}
>  
> -		++nr;
> +		if (!(fda->priv[fd].flags & fdarray_flag__nonfilterable) &&
> +		    !(fda->priv[fd].flags & fdarray_flag__non_perf_event))
> +			++nr;
>  	}

[Severity: Medium]
As noted in the commit message review, nonfilterable descriptors used to skip
the loop entirely via the early continue at the top.

Since they were never reaching this point, is this new check redundant?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723050409.991665-1-irogers@google.com?part=1