Re: [PATCH v3] perf: Fix null pointer access in is_include_guest_event()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added NULL check on `event->pmu` in `__perf_event_read_cpu()` is vulnerable to a compiler-driven TOCTOU race condition against concurrent PMU unregistration, leading to a NULL pointer dereference.

Pre-existing issues:
- [High] The early state check in `perf_event_read_local()` is insufficient to protect against a concurrent PMU detachment, leading to a NULL pointer dereference when accessing `event->pmu->read(event)`.
--

commit 0ef394b915f311447476beda7d3775da5d00cc70
Author: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

perf: Fix null pointer access in is_include_guest_event()

This commit addresses a kernel panic observed during PMU event unbinding
by adding null pointer checks for event->pmu in multiple functions. It
also accounts for mediated events inside __pmu_detach_event() and adds
an early state check in perf_event_read_local().

Fixes: eff95e170275 ("perf: Add APIs to create/release mediated guest vPMUs")

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a6c8e38a31104..63654e2121248 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4794,6 +4794,9 @@ static int __perf_event_read_cpu(struct perf_event *event, int event_cpu)
>  	if ((unsigned)event_cpu >= nr_cpu_ids)
>  		return event_cpu;
>  
> +	if (!event->pmu)
> +		return -ENODEV;
> +
>  	if (event->group_caps & PERF_EV_CAP_READ_SCOPE) {
>  		const struct cpumask *cpumask = perf_scope_cpu_topology_cpumask(event->pmu->scope, event_cpu);

[Severity: High]
Does this code allow a time-of-check to time-of-use (TOCTOU) race condition?

If a concurrent __pmu_detach_event() on another CPU sets event->pmu = NULL,
the compiler could emit separate loads for the check and the subsequent
event->pmu->scope access since the pointer is not cached with READ_ONCE() or
similar. Can this lead to a NULL pointer dereference?

[ ... ]

> @@ -4917,6 +4920,11 @@ int perf_event_read_local(struct perf_event *event, u64 *value,
>  		goto out;
>  	}
>  
> +	if (READ_ONCE(event->state) <= PERF_EVENT_STATE_REVOKED) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
>  	/*
>  	 * Get the event CPU numbers, and adjust them to local if the event is
>  	 * a per-package event that can be read locally

[ ... ]

>  	/*
>  	 * If the event is currently on this CPU, its either a per-task event,
>  	 * or local to this CPU. Furthermore it means its ACTIVE (otherwise
>  	 * oncpu == -1).
>  	 */
>  	if (event_oncpu == smp_processor_id())
>  		event->pmu->read(event);

[Severity: High]
This is a pre-existing issue, but does this early state check sufficiently
protect against a concurrent PMU detachment?

While perf_event_read_local() runs with local IRQs disabled,
perf_pmu_unregister() on another CPU could send an IPI to detach the event
and then set event->pmu = NULL. 

If this concurrent detachment occurs after the early state check, can the
subsequent event->pmu->read(event) call dereference a NULL pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831215214.4169965-1-vinay.belgaumkar@intel.com?part=1