[PATCH] devfreq: stop monitor before calling governor GOV_STOP

Mukesh Ojha posted 1 patch 3 days, 6 hours ago
drivers/devfreq/devfreq.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
[PATCH] devfreq: stop monitor before calling governor GOV_STOP
Posted by Mukesh Ojha 3 days, 6 hours ago
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