Re: [PATCH RFC 04/10] PM / devfreq: Add new target_freq attribute flag for governors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] An ABBA deadlock between kernfs active references and `devfreq_list_lock` can occur when a sysfs attribute in `gov_attr_group` is accessed concurrently with a governor switch.
- [High] Reading the `governor` sysfs attribute can cause a Use-After-Free due to a missing lock.
--

--- Patch [4]: [PATCH RFC 04/10] PM / devfreq: Add new target_freq attribute flag for governors ---
commit 270fe8b349c3594b09be3beb4f4386f10ac713aa
Author: Sibi Sankar <sibi.sankar@oss.qualcomm.com>

PM / devfreq: Add new target_freq attribute flag for governors

This commit adds a new DEVFREQ_GOV_ATTR_TARGET_FREQ flag for devfreq governors.
It gates the existing target_freq sysfs read on this flag, returning -EINVAL
if the active governor does not advertise it.

> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index f08fc6966eaee..2f27a239e34a6 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1530,6 +1530,12 @@ static ssize_t target_freq_show(struct device *dev,
>  {
>  	struct devfreq *df = to_devfreq(dev);
>  
> +	guard(mutex)(&devfreq_list_lock);
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying the
devfreq sysfs locking here, could there be an ABBA deadlock between kernfs
active references and devfreq_list_lock involving the governor attributes?

If a userspace process writes to the governor sysfs file, governor_store()
takes devfreq_list_lock and calls sysfs_update_group():

drivers/devfreq/devfreq.c:governor_store() {
    ...
    ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);
    ...
}

This causes create_files() to unconditionally remove the group's attributes
(like timer) via kernfs_remove_by_name(), which waits for all active
references to drain:

fs/sysfs/group.c:create_files() {
    ...
    kernfs_remove_by_name(parent, (*attr)->name);
    ...
}

Concurrently, if another userspace process reads the timer sysfs file:

drivers/devfreq/devfreq.c:timer_show() {
    ...
    guard(mutex)(&devfreq_list_lock);
    ...
}

The sysfs core holds the active reference to the timer node and calls
timer_show(), which blocks trying to acquire devfreq_list_lock, completing
the ABBA deadlock.

[Severity: High]
This is a pre-existing issue, but while we are adding devfreq_list_lock here
to safely access the governor, does governor_show() have a Use-After-Free
risk because it lacks this lock?

If governor_show() accesses df->governor without holding devfreq_list_lock:

drivers/devfreq/devfreq.c:governor_show() {
    ...
    return sprintf(buf, "%s\n", df->governor->name);
}

Concurrently, if a governor module is unloaded, devfreq_remove_governor()
acquires devfreq_list_lock, stops the governor, and clears df->governor.
After it returns, the module memory is freed. If governor_show() evaluates
df->governor as non-NULL before it's cleared, gets preempted, and then
resumes after the module is freed, could the dereference of
df->governor->name access freed memory?

> +	if (!df->profile || !df->governor ||
> +	    !IS_SUPPORTED_ATTR(df->governor->attrs, TARGET_FREQ))
> +		return -EINVAL;
> +
>  	return sprintf(buf, "%lu\n", df->previous_freq);
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-rfc_v8_scmi_memlat-v1-0-cb732bcff1f4@oss.qualcomm.com?part=4