diff mbox series

[v2] cpufreq: cancel any pending policy update work scheduled before freeing

Message ID 20191018105815.5714-1-sudeep.holla@arm.com (mailing list archive)
State Mainlined, archived
Headers show
Series [v2] cpufreq: cancel any pending policy update work scheduled before freeing | expand

Commit Message

Sudeep Holla Oct. 18, 2019, 10:58 a.m. UTC 
Schedules policy update work may end up racing with the freeing of the
policy and unregistering the driver.

One possible race is as below where the cpufreq_driver is unregistered
but the scheduled work gets executed at later stage when cpufreq_driver
is NULL(i.e. after freeing the policy and driver)

Unable to handle kernel NULL pointer dereference at virtual address 0000001c
pgd = (ptrval)
[0000001c] *pgd=80000080204003, *pmd=00000000
Internal error: Oops: 206 [#1] SMP THUMB2
Modules linked in:
CPU: 0 PID: 34 Comm: kworker/0:1 Not tainted 5.4.0-rc3-00006-g67f5a8081a4b #86
Hardware name: ARM-Versatile Express
Workqueue: events handle_update
PC is at cpufreq_set_policy+0x58/0x228
LR is at dev_pm_qos_read_value+0x77/0xac
Control: 70c5387d  Table: 80203000  DAC: fffffffd
Process kworker/0:1 (pid: 34, stack limit = 0x(ptrval))
	(cpufreq_set_policy) from (refresh_frequency_limits.part.24+0x37/0x48)
	(refresh_frequency_limits.part.24) from (handle_update+0x2f/0x38)
	(handle_update) from (process_one_work+0x16d/0x3cc)
	(process_one_work) from (worker_thread+0xff/0x414)
	(worker_thread) from (kthread+0xff/0x100)
	(kthread) from (ret_from_fork+0x11/0x28)

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/cpufreq/cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)

v1->v2:
	- Dropped check for work_pending as it can still be racy
	- Changed from flush_work to cancel_work_sync

--
2.17.1

Comments

Rafael J. Wysocki Oct. 20, 2019, 11:50 p.m. UTC | #1 
On Fri, Oct 18, 2019 at 12:59 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Schedules policy update work may end up racing with the freeing of the
> policy and unregistering the driver.
>
> One possible race is as below where the cpufreq_driver is unregistered
> but the scheduled work gets executed at later stage when cpufreq_driver
> is NULL(i.e. after freeing the policy and driver)
>
> Unable to handle kernel NULL pointer dereference at virtual address 0000001c
> pgd = (ptrval)
> [0000001c] *pgd=80000080204003, *pmd=00000000
> Internal error: Oops: 206 [#1] SMP THUMB2
> Modules linked in:
> CPU: 0 PID: 34 Comm: kworker/0:1 Not tainted 5.4.0-rc3-00006-g67f5a8081a4b #86
> Hardware name: ARM-Versatile Express
> Workqueue: events handle_update
> PC is at cpufreq_set_policy+0x58/0x228
> LR is at dev_pm_qos_read_value+0x77/0xac
> Control: 70c5387d  Table: 80203000  DAC: fffffffd
> Process kworker/0:1 (pid: 34, stack limit = 0x(ptrval))
>         (cpufreq_set_policy) from (refresh_frequency_limits.part.24+0x37/0x48)
>         (refresh_frequency_limits.part.24) from (handle_update+0x2f/0x38)
>         (handle_update) from (process_one_work+0x16d/0x3cc)
>         (process_one_work) from (worker_thread+0xff/0x414)
>         (worker_thread) from (kthread+0xff/0x100)
>         (kthread) from (ret_from_fork+0x11/0x28)
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/cpufreq/cpufreq.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> v1->v2:
>         - Dropped check for work_pending as it can still be racy
>         - Changed from flush_work to cancel_work_sync
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e93cba29fbb2..8ad9c2859841 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1279,6 +1279,8 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>         }
>
>         dev_pm_qos_remove_request(policy->min_freq_req);
> +       /* cancel any pending policy->update work before freeing the policy */
> +       cancel_work_sync(&policy->update);
>         kfree(policy->min_freq_req);
>
>         cpufreq_policy_put_kobj(policy);
> --

Applying as a fix for 5.4, thanks!
Viresh Kumar Oct. 21, 2019, 2:16 a.m. UTC | #2 
On 18-10-19, 11:58, Sudeep Holla wrote:
> Schedules policy update work may end up racing with the freeing of the
> policy and unregistering the driver.
> 
> One possible race is as below where the cpufreq_driver is unregistered
> but the scheduled work gets executed at later stage when cpufreq_driver
> is NULL(i.e. after freeing the policy and driver)
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000001c
> pgd = (ptrval)
> [0000001c] *pgd=80000080204003, *pmd=00000000
> Internal error: Oops: 206 [#1] SMP THUMB2
> Modules linked in:
> CPU: 0 PID: 34 Comm: kworker/0:1 Not tainted 5.4.0-rc3-00006-g67f5a8081a4b #86
> Hardware name: ARM-Versatile Express
> Workqueue: events handle_update
> PC is at cpufreq_set_policy+0x58/0x228
> LR is at dev_pm_qos_read_value+0x77/0xac
> Control: 70c5387d  Table: 80203000  DAC: fffffffd
> Process kworker/0:1 (pid: 34, stack limit = 0x(ptrval))
> 	(cpufreq_set_policy) from (refresh_frequency_limits.part.24+0x37/0x48)
> 	(refresh_frequency_limits.part.24) from (handle_update+0x2f/0x38)
> 	(handle_update) from (process_one_work+0x16d/0x3cc)
> 	(process_one_work) from (worker_thread+0xff/0x414)
> 	(worker_thread) from (kthread+0xff/0x100)
> 	(kthread) from (ret_from_fork+0x11/0x28)
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/cpufreq/cpufreq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> v1->v2:
> 	- Dropped check for work_pending as it can still be racy
> 	- Changed from flush_work to cancel_work_sync
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e93cba29fbb2..8ad9c2859841 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1279,6 +1279,8 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>  	}
> 
>  	dev_pm_qos_remove_request(policy->min_freq_req);
> +	/* cancel any pending policy->update work before freeing the policy */
> +	cancel_work_sync(&policy->update);
>  	kfree(policy->min_freq_req);
> 
>  	cpufreq_policy_put_kobj(policy);

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
diff mbox series

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e93cba29fbb2..8ad9c2859841 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1279,6 +1279,8 @@  static void cpufreq_policy_free(struct cpufreq_policy *policy)
 	}

 	dev_pm_qos_remove_request(policy->min_freq_req);
+	/* cancel any pending policy->update work before freeing the policy */
+	cancel_work_sync(&policy->update);
 	kfree(policy->min_freq_req);

 	cpufreq_policy_put_kobj(policy);