Re: [PATCH 3/4] perf/core: Allow userspace to select a device PMU's CPU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf/core: AB-BA deadlock between pmus_lock and sysfs active reference
--

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

perf/core: Allow userspace to select a device PMU's CPU

Allow userspace to select the CPU a device PMU is counted on by making the
'cpumask' sysfs attribute writable. This enables rebinding PMU devices
outside of hotplug events by writing an arbitrary CPU set.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c

[ ... ]

> +/*
> + * Move a device PMU to one of the CPUs written here. The PMU gets the final
> + * say: the write fails if it cannot be counted on any of them.
> + */
> +static ssize_t cpumask_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct pmu *pmu = dev_get_drvdata(dev);
> +	cpumask_var_t requested;
> +	ssize_t ret;
> +
> +	if (pmu->scope != PERF_PMU_SCOPE_DEVICE)
> +		return -EOPNOTSUPP;
> +
> +	if (!zalloc_cpumask_var(&requested, GFP_KERNEL))
> +		return -ENOMEM;
> +
> +	ret = cpulist_parse(buf, requested);
> +	if (ret)
> +		goto out;
> +
> +	mutex_lock(&pmus_lock);
> +	ret = perf_pmu_set_cpu(pmu, requested);
> +	mutex_unlock(&pmus_lock);
> +
> +	if (!ret)
> +		ret = count;
> +out:
> +	free_cpumask_var(requested);
> +
> +	return ret;
> +}
> +
> +static DEVICE_ATTR_RW(cpumask);

[Severity: High]
Could this result in an AB-BA deadlock between pmus_lock and the sysfs active
reference?

Since cpumask_store() is called from a sysfs store operation, it implicitly
holds a kernfs active reference while waiting for pmus_lock.

However, perf_pmu_register() holds pmus_lock while calling pmu_dev_alloc():

kernel/events/core.c:perf_pmu_register() {
    ...
    guard(mutex)(&pmus_lock);
    ...
    if (pmu_bus_running && !pmu->dev) {
        int ret = pmu_dev_alloc(pmu);
        if (ret)
            return ret;
    }
    ...
}

Inside pmu_dev_alloc(), if sysfs_update_groups() fails after device_add() has
exposed the sysfs attributes, the error path calls device_del():

kernel/events/core.c:pmu_dev_alloc() {
    ...
    ret = device_add(pmu->dev);
    if (ret)
        goto free_dev;

    if (pmu->attr_update) {
        ret = sysfs_update_groups(&pmu->dev->kobj, pmu->attr_update);
        if (ret)
            goto del_dev;
    }
    ...
del_dev:
    device_del(pmu->dev);
    ...
}

Because device_del() waits for all active sysfs references to drain, if a
concurrent userspace write is executing cpumask_store() and blocking on
pmus_lock, would device_del() wait indefinitely for the active reference to be
released, resulting in an AB-BA deadlock?

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