Re: [PATCH v1] PM / devfreq: governor_userspace: Guard against NULL governor_data during switch

[Finley Xiao <finley.xiao@rock-chips.com>]

在 2026/7/28 11:06, Jie Zhan 写道:
> Hi Finley,
>
> On 7/23/2026 4:08 PM, Finley Xiao wrote:
>> The userspace governor is the only one that uses devfreq->governor_data,
>> allocated in its DEVFREQ_GOV_START handler (userspace_init()) and freed
>> in its DEVFREQ_GOV_STOP handler (userspace_exit()).
>>
>> When switching governors via sysfs (governor_store in devfreq.c), the
>> sequence is:
>>
>> STOP(old) -> df->governor = userspace -> START(userspace) -> userspace_init
>>
>> Between the df->governor assignment and userspace_init() completing,
>> governor_data is still NULL (the previous governor never touched it,
>> and the core does not clear or pre-allocate it). During that window,
>> asynchronous paths that call update_devfreq() outside of the driver's
>> control -- the OPP notifier (devfreq.c:686) and the pm_qos notifier
>> (devfreq.c:707) -- run devfreq_update_target() under df->lock and reach
>> devfreq_userspace_func(), which dereferences governor_data without a
>> NULL check, causing:
>>
>>    Unable to handle kernel access to user memory outside uaccess routines
>>    pc : devfreq_userspace_func+0xc/0x28
>>    lr : devfreq_update_target+0x58/0x158
>>
>> Add a NULL check in devfreq_userspace_func() and fall back to
>> previous_freq, which is the same behaviour as the existing "no user
>> frequency specified yet" branch. This closes the race for all callers
>> since every path ultimately goes through get_target_freq.
>>
>> Additionally, take devfreq->lock around the kfree()/NULL assignment in
>> userspace_exit(). Without it, a concurrent OPP/pm_qos notifier running
>> under devfreq->lock could read governor_data after it is freed but
>> before it is set to NULL, constituting a use-after-free.
>>
>> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
>> ---
>>   drivers/devfreq/governor_userspace.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
>> index 3906ebedbae8..f7a485c43fd7 100644
>> --- a/drivers/devfreq/governor_userspace.c
>> +++ b/drivers/devfreq/governor_userspace.c
>> @@ -24,7 +24,7 @@ static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq)
>>   {
>>   	struct userspace_data *data = df->governor_data;
>>   
>> -	if (data->valid)
>> +	if (data && data->valid)
> This one is fine.
>>   		*freq = data->user_frequency;
>>   	else
>>   		*freq = df->previous_freq; /* No user freq specified yet */
>> @@ -110,8 +110,10 @@ static void userspace_exit(struct devfreq *devfreq)
>>   	if (devfreq->dev.kobj.sd)
>>   		sysfs_remove_group(&devfreq->dev.kobj, &dev_attr_group);
>>   
>> +	mutex_lock(&devfreq->lock);
>>   	kfree(devfreq->governor_data);
>>   	devfreq->governor_data = NULL;
>> +	mutex_unlock(&devfreq->lock);
> However, for this one, I'm not sure it's the proper place to add the lock.
>
> Why should we allow the whole governor switching to be concurrent with
> frequency switching?  I guess there might be something wrong elsewhere.
> The devfreq->lock should be held around governor switching.
> Can you have a try?  but be careful of deadlock because the governor
> switching currently holds devfreq_list_lock.
I tried to hold devfreq->lock around the governor switching, but the
GOV_STOP event handlers cannot run with devfreq->lock held. There are
three independent reasons:

1. userspace_exit(): sysfs_remove_group() must not be called with
    devfreq->lock held.  The removal waits for the in-flight sysfs readers
    of the "set_freq" attribute to release the kernfs active reference, and
    those readers take the active reference first and then block on
    devfreq->lock inside set_freq_show()/set_freq_store(). Removing the
    group while holding devfreq->lock waits for a reader that is waiting for
    the same lock, i.e. a deadlock.

2. simple_ondemand: devfreq_monitor_stop() (DEVFREQ_GOV_STOP) has to call
    cancel_delayed_work_sync() with devfreq->lock released, because the
    monitoring work (devfreq_monitor()) takes devfreq->lock. Waiting for
    the work while holding devfreq->lock deadlocks.  This is also why
    devfreq_monitor_stop() drops the lock before cancel_delayed_work_sync()
    today.

3. passive: the passive governor's DEVFREQ_GOV_STOP calls
    devfreq_unregister_notifier() -> srcu_notifier_chain_unregister() ->
    synchronize_srcu(), which waits for the in-flight notifier callbacks.
    Those callbacks (devfreq_passive_notifier_call()) take the child's
    devfreq->lock, so unregistering while holding the child's devfreq->lock
    can deadlock if the parent's frequency transition is in flight.

In addition, the GOV_START handlers take devfreq->lock themselves:
performance/powersave call update_devfreq() under the lock, simple_ondemand
calls devfreq_monitor_start() which takes the lock, and the passive
governor's cpufreq path takes it.  Holding devfreq->lock around the
switching would therefore also self-deadlock unless their internal locking
is removed as well.

Even if devfreq->lock were held around the whole switch,
the userspace governor would still need to hold the devfreq->lock
around the kfree itself: because of (1), userspace_exit() has to remove the
sysfs files with the lock released, so the handler would have to drop the
lock for the sysfs part and take it again for the kfree.  Holding the lock
around the switching would not remove the lock from the userspace governor;
it would only add locking around it and require restructuring all
governors.
>
> Thanks!
> Jie
>>   }
>>   
>>   static int devfreq_userspace_handler(struct devfreq *devfreq,

-- 
Best regards,

底层平台中心 肖锋（Finley Xiao）
************************************************************************************
瑞芯微电子股份有限公司
Rockchip Electronics Co., Ltd.
福建省福州市铜盘路软件大道89号软件园A区21号楼 350003
No.21 Building, A District, Fuzhou Software Park, Fuzhou, Fujian 350003, P.R. China
Tel: 0591-83991906-8602 Mobile: 18506057603
************************************************************************************