Re: [PATCH 2/4] perf/core: Add PERF_PMU_SCOPE_DEVICE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf: WARN_ON_ONCE DoS during CPU hotplug for restricted device PMUs
- [Low] perf: spurious -ENODEV instead of -EBUSY during PMU migration
--

commit 97f2867d7b192326f20b5dc11e93789631bd5a2f
Author: Haris Okanovic <harisokn@amazon.com>

perf/core: Add PERF_PMU_SCOPE_DEVICE

Add a "device" pmu::scope for PMUs counted on a single CPU drawn
from a driver-supplied set instead of a fixed topology.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index dacc7375506a7..6274fea465a38 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
[ ... ]
> @@ -14109,6 +14216,12 @@ SYSCALL_DEFINE5(perf_event_open,
>  		}
>  	}
>  
> +	/* The PMU may have moved since it was picked, retry if so. */
> +	if (perf_event_pmu_moved(event)) {
> +		err = -EBUSY;
> +		goto err_locked;
> +	}
> +

[Severity: Low]
During a PMU migration caused by a CPU hotplug, a concurrent
sys_perf_event_open() could see cpuctx->online == 0. Does the existing
check for !cpuctx->online (which returns -ENODEV) eclipse this new
perf_event_pmu_moved() check, breaking the intent to return -EBUSY?

[ ... ]
> @@ -14406,6 +14519,12 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
>  		}
>  	}
>  
> +	/* The PMU may have moved since it was picked, retry if so. */
> +	if (perf_event_pmu_moved(event)) {
> +		err = -EBUSY;
> +		goto err_pmu_ctx;
> +	}
> +

[Severity: Low]
Similarly, does the !cpuctx->online check in perf_event_create_kernel_counter()
hide the -EBUSY return path when a PMU migrates due to CPU hotplug?

[ ... ]
> @@ -15202,6 +15322,36 @@ static void perf_swevent_init_cpu(unsigned int cpu)
>  	mutex_unlock(&swhash->hlist_mutex);
>  }
>  
> +static void perf_pmu_rebind(struct pmu *pmu)
> +{
> +	int ret;
> +
> +	/*
> +	 * An empty mask legitimately happens for the last CPU in the
> +	 * perf_reboot() teardown, when there is nothing left to migrate to.
> +	 */
> +	if (cpumask_empty(perf_online_mask))
> +		return;
> +
> +	/* Failing to pick out of every online CPU is a PMU bug. */
> +	ret = perf_pmu_set_cpu(pmu, perf_online_mask);
> +	WARN_ON_ONCE(ret);
> +}

[Severity: High]
If a device PMU implements filter_allowed_cpus() to restrict its valid execution
CPUs, and a user hot-unplugs all of those restricted CPUs while keeping at least
one other system CPU online, perf_pmu_set_cpu() accurately returns -ENODEV.
Will this legitimately return -ENODEV and trigger the WARN_ON_ONCE() in
perf_pmu_rebind(), leading to a denial of service if panic_on_warn is enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013185603.1347265-1-harisokn@amazon.com?part=2