Re: [PATCH v2 2/2] perf/core: Ignore overflows while disable is pending

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Removing the synchronous PMU stop causes an NMI storm and temporary CPU livelocks when an event reaches its limit.
- [High] Spurious execution of BPF programs and AUX handlers thousands of times beyond the requested event limit.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race clobbers POLL_HUP and allows sample recording beyond the limit.
--

commit 17c3cee582aa984c752ecbdcaf9da672f9cf95c3
Author: Leo Yan <leo.yan@arm.com>
perf/core: Ignore overflows while disable is pending

This commit removes the direct PMU stop when an event reaches its limit,
relying instead on the irq_work queued by perf_event_disable_inatomic().
It also adds a check for pending_disable to skip further overflow processing.

Link: https://lore.kernel.org/lkml/aICYAqM5EQUlTqtX@li-2b55cdcc-350b-11b2-a85c-a78bff51fc11.ibm.com/

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
[ ... ]
> @@ -10745,12 +10745,18 @@ static int __perf_event_overflow(struct perf_event *event,
>  	 * events
>  	 */
>  
> +	/*
> +	 * Disable is pending, skip further overflow processing so the pending
> +	 * POLL_HUP is preserved and no samples are recorded beyond the limit.
> +	 */
> +	if (event->pending_disable)
> +		goto out;
> +

Looking at __perf_event_overflow(), this pending_disable check is placed after
the executions of perf_event_aux_pause() and bpf_overflow_handler().

Since removing the synchronous PMU stop delays hardware stopping until the
irq_work runs, could this cause the attached BPF programs and AUX handlers to
be spuriously invoked thousands of times beyond the event limit before this
check skips the rest of the function?

>  	event->pending_kill = POLL_IN;
>  	if (events && atomic_dec_and_test(&event->event_limit)) {
>  		ret = 1;
>  		event->pending_kill = POLL_HUP;
>  		perf_event_disable_inatomic(event);
> -		event->pmu->stop(event, 0);

Does removing the direct PMU stop here cause an NMI storm?

Because NMIs have higher priority than standard interrupts used for irq_work,
it appears a high-frequency PMU event could immediately generate another NMI
upon returning from the interrupt handler. Architecture-specific handlers
typically re-arm the counter, potentially creating a tight NMI loop that
starves the irq_work until __perf_event_account_interrupt() forces throttling
by reaching max_samples_per_tick.

Additionally, can a race condition occur when the irq_work eventually runs?

__perf_pending_disable() clears the flag before stopping the hardware:

__perf_pending_disable()
    if (event->pending_disable) {
        event->pending_disable = 0;
        perf_event_disable_local(event);
    }

If an NMI fires in the exact window between clearing the flag and the PMU
being disabled, __perf_event_overflow() would see pending_disable == 0.
It would then bypass the new goto out check, unconditionally set
event->pending_kill = POLL_IN, clobber the previously set POLL_HUP, and
record a sample beyond the requested limit.

>  	}
>  
>  	if (event->attr.sigtrap) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-arm_cs_clean_perf_handle-v2-0-cbb29c3b3661@arm.com?part=2