Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The lockless checks for revoked state and NULL PMU pointers are insufficient to prevent a Use-After-Free of the PMU structure or a NULL pointer dereference during concurrent unregistration. -- commit 4a985bf8ccfd6853c89a279d4b7181e83c33bcef Author: Vinay Belgaumkar <[email protected]> perf: Add checks to prevent null ptr access This commit adds a check for revoked states in perf_event_read_local() and a null pointer check for the PMU in __perf_event_read_cpu(). > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 7777e82aad5e7..059f82f0cadd4 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -4788,14 +4788,19 @@ static inline const struct cpumask > *perf_scope_cpu_topology_cpumask(unsigned int > > static int __perf_event_read_cpu(struct perf_event *event, int event_cpu) > { > + struct pmu *pmu = READ_ONCE(event->pmu); > int local_cpu = smp_processor_id(); > u16 local_pkg, event_pkg; > > if ((unsigned)event_cpu >= nr_cpu_ids) > return event_cpu; > > + if (!pmu) > + return -ENODEV; > + [Severity: High] This is a pre-existing issue, but does this lockless check provide a false sense of security against concurrent PMU unregistration? A concurrent perf_pmu_unregister() can set event->pmu = NULL and free the PMU memory. Since this check relies on a cached pointer without holding locks that serialize against unregistration (e.g., ctx->lock), the PMU can be freed immediately after this check passes. > if (event->group_caps & PERF_EV_CAP_READ_SCOPE) { > - const struct cpumask *cpumask = > perf_scope_cpu_topology_cpumask(event->pmu->scope, event_cpu); > + const struct cpumask *cpumask = > perf_scope_cpu_topology_cpumask(pmu->scope, > + > event_cpu); [Severity: High] This is a pre-existing issue, but can this lead to a Use-After-Free when accessing pmu->scope? If the PMU is unregistered and freed by another CPU after we cache it but before we dereference it here, pmu->scope will read from freed memory. [ ... ] > @@ -4917,6 +4922,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; > + } > + [Severity: High] This is a pre-existing issue, but is this check vulnerable to a TOCTOU race? The event state can change immediately after this check if the event context is inactive or belongs to a different CPU, allowing a concurrent unregistration to detach the event and free the PMU while perf_event_read_local() continues executing. This leaves the subsequent call to event->pmu->read(event) unprotected, potentially resulting in a NULL pointer dereference or Use-After-Free. > /* > * Get the event CPU numbers, and adjust them to local if the event is > * a per-package event that can be read locally -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
