Re: [PATCH RFC v7 6/9] PM / devfreq: Add a governor for tracking remote device frequencies

[sashiko-bot@kernel.org]

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

New issues:
- [High] DEVFREQ_GOV_FLAG_TRACK_REMOTE inappropriately clamps the tracked frequency to local PM QoS limits and bypasses OPP rounding, breaking statistics and tracepoints.
- [High] Unloading an `IMMUTABLE` governor bypasses the immutable restriction, allowing incompatible governors to bind and crash the kernel via a missing `.target` callback check.
- [Medium] Missing validation of `get_cur_freq` during governor start causes unbounded dmesg log spam.

Pre-existing issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race conditions and unprotected reads during governor removal lead to NULL pointer dereferences or Use-After-Free crashes.
--

commit a561d57fcfe231ac7e70741f77838221fbcf0815
Author: Sibi Sankar <sibi.sankar@oss.qualcomm.com>

PM / devfreq: Add a governor for tracking remote device frequencies

This commit adds a new "remote" devfreq governor that uses the track_remote
flag to expose a remote device's frequency to userspace. It deliberately
does not advertise the target frequency attribute since the kernel cannot
set it on a remote-managed device.

> diff --git a/drivers/devfreq/governor_remote.c b/drivers/devfreq/governor_remote.c
> new file mode 100644
> index 0000000000000..df3819757e56f
> --- /dev/null
> +++ b/drivers/devfreq/governor_remote.c
> @@ -0,0 +1,73 @@

[ ... ]

> +static int devfreq_remote_track_handler(struct devfreq *devfreq, unsigned int event, void *data)
> +{
> +	switch (event) {
> +	case DEVFREQ_GOV_START:
> +		devfreq_monitor_start(devfreq);
> +		break;

[Severity: Medium]
Will this cause unbounded dmesg log spam if a device fails to provide the
get_cur_freq callback?

If a device registers with the remote governor but lacks the
devfreq->profile->get_cur_freq callback, the governor successfully starts.

On every polling interval, devfreq_monitor() calls update_devfreq(), which
invokes devfreq_remote_track_func(). This returns -ENXIO, causing
update_devfreq() to log 'dvfs failed with (-6) error' to the console.

Since the delayed work is re-queued unconditionally, this will spam the log.
Should DEVFREQ_GOV_START validate the presence of get_cur_freq and return
-EINVAL if it is missing?

[ ... ]

> +static struct devfreq_governor devfreq_remote_track = {
> +	.name = DEVFREQ_GOV_REMOTE,
> +	.attrs = DEVFREQ_GOV_ATTR_POLLING_INTERVAL
> +		| DEVFREQ_GOV_ATTR_TIMER,
> +	.flags = DEVFREQ_GOV_FLAG_IMMUTABLE
> +		| DEVFREQ_GOV_FLAG_TRACK_REMOTE,

[Severity: High]
Does the use of DEVFREQ_GOV_FLAG_TRACK_REMOTE here cause the tracked
frequency to be clamped and bypass OPP rounding?

When the remote governor returns the actual frequency via get_target_freq(),
devfreq_update_target() incorrectly clamps this observed frequency against the
local PM QoS min_freq and max_freq bounds.

The clamped frequency is then passed to devfreq_set_target(). Because
DEVFREQ_GOV_FLAG_TRACK_REMOTE is set, devfreq_set_target() bypasses the
profile->target() callback and the frequency is never rounded to a valid
discrete OPP.

This clamped, unrounded frequency is recorded into trans_stat, time_in_state,
and tracepoints, causing them to report false data if the remote device
operates outside local bounds.

Furthermore, because the frequency typically doesn't match a discrete
freq_table entry, devfreq_update_status() returns -EINVAL, spamming the
log with 'Couldn't update frequency transition information'.

[ ... ]

> +static void __exit devfreq_remote_track_exit(void)
> +{
> +	int ret;
> +
> +	ret = devfreq_remove_governor(&devfreq_remote_track);

[Severity: High]
Can unloading this module bypass the DEVFREQ_GOV_FLAG_IMMUTABLE restriction
and lead to a NULL pointer dereference?

Devices using the remote governor omit the .target callback because
frequencies are managed externally. The governor uses
DEVFREQ_GOV_FLAG_IMMUTABLE to prevent users from switching to a governor that
would invoke the missing callback.

When this module is unloaded, devfreq_remove_governor() forcefully stops it
and clears devfreq->governor. The core forgets the device was immutable,
allowing a user to bind a non-remote governor via sysfs.

When the new governor calls devfreq_update_target(), it eventually invokes
devfreq_set_target().

Since devfreq_set_target() unconditionally calls devfreq->profile->target()
without checking if it is NULL, would this result in a kernel panic?

[Severity: High]
This isn't a bug introduced by this patch, but does calling
devfreq_remove_governor() here expose a race condition leading to a
use-after-free or NULL pointer dereference?

Devfreq governor modules are not pinned with try_module_get() as the
devfreq_governor struct lacks an owner field, meaning they can be unloaded
while devices are actively using them.

devfreq_remove_governor() sets devfreq->governor = NULL while holding
devfreq_list_lock but without acquiring devfreq->lock.

Concurrently, execution paths like devfreq_update_target() read or check
devfreq->governor while holding only devfreq->lock.

If a concurrent PM QoS notifier or sysfs write passes the
if (!devfreq->governor) check, and the governor module is removed immediately
after, the subsequent call to devfreq->governor->get_target_freq() will
dereference a NULL pointer or execute unmapped module text.

> +	if (ret)
> +		pr_err("%s: failed to remove governor %d\n", __func__, ret);
> +}
> +module_exit(devfreq_remote_track_exit);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-rfc_v7_scmi_memlat-v7-0-f3f68c608f25@oss.qualcomm.com?part=6