[PATCH] devfreq: stop monitor before calling governor GOV_STOP

[PATCH] devfreq: stop monitor before calling governor GOV_STOP

[Mukesh Ojha]

The following NULL pointer dereference is observed when devfreq_monitor
fires after userspace_exit() has freed governor_data:

  pc : devfreq_userspace_func+0x10/0x2c
  Code: 39402109 --> ldrb w9, [x0, #8]   (x0 = NULL)
  Call trace:
   devfreq_userspace_func+0x10/0x2c
   devfreq_monitor+0x34/0x134
   process_scheduled_works+0x1d8/0x804
   worker_thread+0x1c0/0x458

The fault address 0x8 is userspace_data.valid — an 8-byte unsigned long
(user_frequency) precedes it, so governor_data == NULL is the cause.

The race: devfreq_resume_device() reads df->governor without holding
devfreq_list_lock, while governor_store() updates it under that lock.
The stale pointer dispatches GOV_RESUME to the old governor
(simple_ondemand) after the switch has already completed:

  CPU 0 (devfreq_resume_device)        CPU 1 (governor_store)

  read df->governor → simple_ondemand
                                       lock(devfreq_list_lock)
                                       GOV_STOP → monitor stopped
                                       df->governor = userspace
                                       GOV_START → governor_data alloc'd
                                       unlock(devfreq_list_lock)
  simple_ondemand->event_handler
    (DEVFREQ_GOV_RESUME)
    → devfreq_monitor_resume()
        stop_polling == true → re-queue work  ← stray monitor!
        stop_polling = false

devfreq_monitor is now queued with df->governor == userspace. When
devfreq_remove_device() or governor_store() next calls userspace GOV_STOP,
userspace_exit() frees governor_data. The stray monitor fires,
dereferences NULL governor_data, and crashes.

Call devfreq_monitor_stop() before every GOV_STOP dispatch in the core,
ensuring the polling work is fully cancelled before any governor tears down
its private data.  devfreq_monitor_stop() is safe unconditionally:
IRQ_DRIVEN governors return immediately, polling governors that already
call it in their own GOV_STOP handler make the second call a no-op
(stop_polling already true), and governors that never polled get a harmless
cancel_delayed_work_sync() on an empty queue.

Four sites are fixed: devfreq_remove_device(), governor_store(),
devfreq_remove_governor() (governor module unload), and timer_store()
(timer-type change performs a GOV_STOP + GOV_START cycle).

Fixes: 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices which can idle")
Cc: stable@vger.kernel.org
Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/devfreq/devfreq.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index f08fc6966eae..7538f20657b7 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -993,9 +993,12 @@ int devfreq_remove_device(struct devfreq *devfreq)
 
 	devfreq_cooling_unregister(devfreq->cdev);
 
-	if (devfreq->governor)
+	if (devfreq->governor) {
+		devfreq_monitor_stop(devfreq);
 		devfreq->governor->event_handler(devfreq,
 						 DEVFREQ_GOV_STOP, NULL);
+	}
+
 	device_unregister(&devfreq->dev);
 
 	return 0;
@@ -1344,6 +1347,7 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
 
 		if (!strncmp(devfreq->governor->name, governor->name,
 			     DEVFREQ_NAME_LEN)) {
+			devfreq_monitor_stop(devfreq);
 			ret = devfreq->governor->event_handler(devfreq,
 						DEVFREQ_GOV_STOP, NULL);
 			if (ret) {
@@ -1416,6 +1420,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	 * Stop the current governor and remove the specific sysfs files
 	 * which depend on current governor.
 	 */
+	devfreq_monitor_stop(df);
 	ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
 	if (ret) {
 		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
@@ -1863,6 +1868,7 @@ static ssize_t timer_store(struct device *dev, struct device_attribute *attr,
 	df->profile->timer = timer;
 	mutex_unlock(&df->lock);
 
+	devfreq_monitor_stop(df);
 	ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
 	if (ret) {
 		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
-- 
2.53.0

Re: [PATCH] devfreq: stop monitor before calling governor GOV_STOP

[Mukesh Ojha]

On Tue, Jul 21, 2026 at 10:41:41PM +0530, Mukesh Ojha wrote:
> The following NULL pointer dereference is observed when devfreq_monitor
> fires after userspace_exit() has freed governor_data:
> 
>   pc : devfreq_userspace_func+0x10/0x2c
>   Code: 39402109 --> ldrb w9, [x0, #8]   (x0 = NULL)
>   Call trace:
>    devfreq_userspace_func+0x10/0x2c
>    devfreq_monitor+0x34/0x134
>    process_scheduled_works+0x1d8/0x804
>    worker_thread+0x1c0/0x458
> 
> The fault address 0x8 is userspace_data.valid — an 8-byte unsigned long
> (user_frequency) precedes it, so governor_data == NULL is the cause.
> 
> The race: devfreq_resume_device() reads df->governor without holding
> devfreq_list_lock, while governor_store() updates it under that lock.
> The stale pointer dispatches GOV_RESUME to the old governor
> (simple_ondemand) after the switch has already completed:
> 
>   CPU 0 (devfreq_resume_device)        CPU 1 (governor_store)
> 
>   read df->governor → simple_ondemand
>                                        lock(devfreq_list_lock)
>                                        GOV_STOP → monitor stopped
>                                        df->governor = userspace
>                                        GOV_START → governor_data alloc'd
>                                        unlock(devfreq_list_lock)
>   simple_ondemand->event_handler
>     (DEVFREQ_GOV_RESUME)
>     → devfreq_monitor_resume()
>         stop_polling == true → re-queue work  ← stray monitor!
>         stop_polling = false
> 
> devfreq_monitor is now queued with df->governor == userspace. When
> devfreq_remove_device() or governor_store() next calls userspace GOV_STOP,
> userspace_exit() frees governor_data. The stray monitor fires,
> dereferences NULL governor_data, and crashes.
> 
> Call devfreq_monitor_stop() before every GOV_STOP dispatch in the core,
> ensuring the polling work is fully cancelled before any governor tears down
> its private data.  devfreq_monitor_stop() is safe unconditionally:
> IRQ_DRIVEN governors return immediately, polling governors that already
> call it in their own GOV_STOP handler make the second call a no-op
> (stop_polling already true), and governors that never polled get a harmless
> cancel_delayed_work_sync() on an empty queue.
> 
> Four sites are fixed: devfreq_remove_device(), governor_store(),
> devfreq_remove_governor() (governor module unload), and timer_store()
> (timer-type change performs a GOV_STOP + GOV_START cycle).
> 
> Fixes: 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices which can idle")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

Can we consider reviewing fix for the mentioned issue ?

> ---
>  drivers/devfreq/devfreq.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index f08fc6966eae..7538f20657b7 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -993,9 +993,12 @@ int devfreq_remove_device(struct devfreq *devfreq)
>  
>  	devfreq_cooling_unregister(devfreq->cdev);
>  
> -	if (devfreq->governor)
> +	if (devfreq->governor) {
> +		devfreq_monitor_stop(devfreq);
>  		devfreq->governor->event_handler(devfreq,
>  						 DEVFREQ_GOV_STOP, NULL);
> +	}
> +
>  	device_unregister(&devfreq->dev);
>  
>  	return 0;
> @@ -1344,6 +1347,7 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
>  
>  		if (!strncmp(devfreq->governor->name, governor->name,
>  			     DEVFREQ_NAME_LEN)) {
> +			devfreq_monitor_stop(devfreq);
>  			ret = devfreq->governor->event_handler(devfreq,
>  						DEVFREQ_GOV_STOP, NULL);
>  			if (ret) {
> @@ -1416,6 +1420,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	 * Stop the current governor and remove the specific sysfs files
>  	 * which depend on current governor.
>  	 */
> +	devfreq_monitor_stop(df);
>  	ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
>  	if (ret) {
>  		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
> @@ -1863,6 +1868,7 @@ static ssize_t timer_store(struct device *dev, struct device_attribute *attr,
>  	df->profile->timer = timer;
>  	mutex_unlock(&df->lock);
>  
> +	devfreq_monitor_stop(df);
>  	ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
>  	if (ret) {
>  		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
> -- 
> 2.53.0
> 

-- 
-Mukesh Ojha

Re: [PATCH] devfreq: stop monitor before calling governor GOV_STOP

[Mukesh Ojha]

On Mon, Aug 03, 2026 at 11:46:14PM +0530, Mukesh Ojha wrote:
> On Tue, Jul 21, 2026 at 10:41:41PM +0530, Mukesh Ojha wrote:
> > The following NULL pointer dereference is observed when devfreq_monitor
> > fires after userspace_exit() has freed governor_data:
> > 
> >   pc : devfreq_userspace_func+0x10/0x2c
> >   Code: 39402109 --> ldrb w9, [x0, #8]   (x0 = NULL)
> >   Call trace:
> >    devfreq_userspace_func+0x10/0x2c
> >    devfreq_monitor+0x34/0x134
> >    process_scheduled_works+0x1d8/0x804
> >    worker_thread+0x1c0/0x458
> > 
> > The fault address 0x8 is userspace_data.valid — an 8-byte unsigned long
> > (user_frequency) precedes it, so governor_data == NULL is the cause.
> > 
> > The race: devfreq_resume_device() reads df->governor without holding
> > devfreq_list_lock, while governor_store() updates it under that lock.
> > The stale pointer dispatches GOV_RESUME to the old governor
> > (simple_ondemand) after the switch has already completed:
> > 
> >   CPU 0 (devfreq_resume_device)        CPU 1 (governor_store)
> > 
> >   read df->governor → simple_ondemand
> >                                        lock(devfreq_list_lock)
> >                                        GOV_STOP → monitor stopped
> >                                        df->governor = userspace
> >                                        GOV_START → governor_data alloc'd
> >                                        unlock(devfreq_list_lock)
> >   simple_ondemand->event_handler
> >     (DEVFREQ_GOV_RESUME)
> >     → devfreq_monitor_resume()
> >         stop_polling == true → re-queue work  ← stray monitor!
> >         stop_polling = false
> > 
> > devfreq_monitor is now queued with df->governor == userspace. When
> > devfreq_remove_device() or governor_store() next calls userspace GOV_STOP,
> > userspace_exit() frees governor_data. The stray monitor fires,
> > dereferences NULL governor_data, and crashes.
> > 
> > Call devfreq_monitor_stop() before every GOV_STOP dispatch in the core,
> > ensuring the polling work is fully cancelled before any governor tears down
> > its private data.  devfreq_monitor_stop() is safe unconditionally:
> > IRQ_DRIVEN governors return immediately, polling governors that already
> > call it in their own GOV_STOP handler make the second call a no-op
> > (stop_polling already true), and governors that never polled get a harmless
> > cancel_delayed_work_sync() on an empty queue.
> > 
> > Four sites are fixed: devfreq_remove_device(), governor_store(),
> > devfreq_remove_governor() (governor module unload), and timer_store()
> > (timer-type change performs a GOV_STOP + GOV_START cycle).
> > 
> > Fixes: 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices which can idle")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> 
> Can we consider reviewing fix for the mentioned issue ?

Change still applies on linux-next tip, So no need of a rebase.

Can I get review on this ?

-- 
-Mukesh Ojha

Re: [PATCH] devfreq: stop monitor before calling governor GOV_STOP

[Jie Zhan]

On 9/24/2026 8:32 PM, Mukesh Ojha wrote:
> On Mon, Aug 03, 2026 at 11:46:14PM +0530, Mukesh Ojha wrote:
>> On Tue, Jul 21, 2026 at 10:41:41PM +0530, Mukesh Ojha wrote:
>>> The following NULL pointer dereference is observed when devfreq_monitor
>>> fires after userspace_exit() has freed governor_data:
>>>
>>>   pc : devfreq_userspace_func+0x10/0x2c
>>>   Code: 39402109 --> ldrb w9, [x0, #8]   (x0 = NULL)
>>>   Call trace:
>>>    devfreq_userspace_func+0x10/0x2c
>>>    devfreq_monitor+0x34/0x134
>>>    process_scheduled_works+0x1d8/0x804
>>>    worker_thread+0x1c0/0x458
>>>
>>> The fault address 0x8 is userspace_data.valid — an 8-byte unsigned long
>>> (user_frequency) precedes it, so governor_data == NULL is the cause.
>>>
>>> The race: devfreq_resume_device() reads df->governor without holding
>>> devfreq_list_lock, while governor_store() updates it under that lock.
>>> The stale pointer dispatches GOV_RESUME to the old governor
>>> (simple_ondemand) after the switch has already completed:
>>>
>>>   CPU 0 (devfreq_resume_device)        CPU 1 (governor_store)
>>>
>>>   read df->governor → simple_ondemand
>>>                                        lock(devfreq_list_lock)
>>>                                        GOV_STOP → monitor stopped
>>>                                        df->governor = userspace
>>>                                        GOV_START → governor_data alloc'd
>>>                                        unlock(devfreq_list_lock)
>>>   simple_ondemand->event_handler
>>>     (DEVFREQ_GOV_RESUME)
>>>     → devfreq_monitor_resume()
>>>         stop_polling == true → re-queue work  ← stray monitor!
>>>         stop_polling = false
>>>
>>> devfreq_monitor is now queued with df->governor == userspace. When
>>> devfreq_remove_device() or governor_store() next calls userspace GOV_STOP,
>>> userspace_exit() frees governor_data. The stray monitor fires,
>>> dereferences NULL governor_data, and crashes.
>>>
>>> Call devfreq_monitor_stop() before every GOV_STOP dispatch in the core,
>>> ensuring the polling work is fully cancelled before any governor tears down
>>> its private data.  devfreq_monitor_stop() is safe unconditionally:
>>> IRQ_DRIVEN governors return immediately, polling governors that already
>>> call it in their own GOV_STOP handler make the second call a no-op
>>> (stop_polling already true), and governors that never polled get a harmless
>>> cancel_delayed_work_sync() on an empty queue.
>>>
>>> Four sites are fixed: devfreq_remove_device(), governor_store(),
>>> devfreq_remove_governor() (governor module unload), and timer_store()
>>> (timer-type change performs a GOV_STOP + GOV_START cycle).
>>>
>>> Fixes: 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices which can idle")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
>>
>> Can we consider reviewing fix for the mentioned issue ?
> 
> Change still applies on linux-next tip, So no need of a rebase.
> 
> Can I get review on this ?
> 
Unfortunately devfreq hasn't been taken patches for a few months.  AFAIK, a
few patchsets are pending to be queued up before any review can carry on.