Re: [PATCH v7 12/20] perf: Add perf_pmu_resched_update()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Deadlock vulnerability in exported perf_pmu_resched_update() API due to missing interrupt disablement
- [Medium] Undocumented strict atomic context constraints for the update callback risk PREEMPT_RT lock inversion
--

commit bfd872050e6147889915de2de8e49972cd31d5f4
Author: Colton Lewis <coltonlewis@google.com>

perf: Add perf_pmu_resched_update()

This commit introduces perf_pmu_resched_update() to allow dynamic modification
of PMU guest counter reservations safely. It takes a callback that is executed
between scheduling perf events out and back in.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 89b40e4397177..62fec73caabad 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c

[ ... ]

> @@ -3021,6 +3022,9 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
>  	else if (event_type & EVENT_PINNED)
>  		ctx_sched_out(&cpuctx->ctx, pmu, EVENT_FLEXIBLE);
>  
> +	if (update)
> +		update(pmu, data);

Are there restrictions on what this update callback can do?

Because the callback is synchronously invoked while holding the perf context
raw_spinlock_t locks, any attempt to acquire a standard spinlock_t, allocate
memory, or block will trigger a sleeping while atomic bug or a lock inversion
panic on PREEMPT_RT kernels.

Could we document these strict atomic context requirements so future callers
using this exported API do not inadvertently crash the kernel?

> +
>  	perf_event_sched_in(cpuctx, task_ctx, pmu, 0);
>  
>  	for_each_epc(epc, &cpuctx->ctx, pmu, 0)

[ ... ]

> @@ -3032,6 +3036,24 @@ static void ctx_resched(struct perf_cpu_context *cpuctx,
>  	}
>  }
>  
> +static void ctx_resched(struct perf_cpu_context *cpuctx,
> +			struct perf_event_context *task_ctx,
> +			struct pmu *pmu, enum event_type_t event_type)
> +{
> +	__ctx_resched(cpuctx, task_ctx, pmu, event_type, NULL, NULL);
> +}
> +
> +void perf_pmu_resched_update(struct pmu *pmu, void (*update)(struct pmu *, void *), void *data)
> +{
> +	struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context);
> +	struct perf_event_context *task_ctx = cpuctx->task_ctx;
> +
> +	perf_ctx_lock(cpuctx, task_ctx);

Can this cause a deadlock if a hardware interrupt fires?

Looking at perf_ctx_lock(), it uses raw_spin_lock(&ctx->lock) which disables
preemption but leaves local interrupts enabled. If a Performance Monitoring
Interrupt or a timer tick executing perf_event_task_tick() fires on this CPU
while the lock is held, the interrupt handler will attempt to acquire the
same raw spinlock.

Should this function disable local interrupts (e.g., using local_irq_save())
before retrieving the CPU pointer and acquiring the locks?

> +	__ctx_resched(cpuctx, task_ctx, pmu, EVENT_ALL|EVENT_CPU, update, data);
> +	perf_ctx_unlock(cpuctx, task_ctx);
> +}
> +EXPORT_SYMBOL_GPL(perf_pmu_resched_update);

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