diff mbox series

[v2] cpufreq: check if policy is inactive early in __cpufreq_get

Message ID 20190107185153.23220-1-sudeep.holla@arm.com (mailing list archive)
State Mainlined
Delegated to: Rafael Wysocki
Headers show
Series [v2] cpufreq: check if policy is inactive early in __cpufreq_get | expand

Commit Message

Sudeep Holla Jan. 7, 2019, 6:51 p.m. UTC 
cpuinfo_cur_freq gets current CPU frequency as detected by hardware
while scaling_cur_freq last known CPU frequency. Some platforms may not
allow checking the CPU frequency of an offline CPU or the associated
resources may have been released via cpufreq_exit when the CPU gets
offlined, in which case the policy would have been invalidated already.
If we attempt to get current frequency from the hardware, it may result
in hang or crash.

For example on Juno, I see:

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000188
[0000000000000188] pgd=0000000000000000
Internal error: Oops: 96000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 5 PID: 4202 Comm: cat Not tainted 4.20.0-08251-ga0f2c0318a15-dirty #87
Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform
pstate: 40000005 (nZcv daif -PAN -UAO)
pc : scmi_cpufreq_get_rate+0x34/0xb0
lr : scmi_cpufreq_get_rate+0x34/0xb0
Call trace:
 scmi_cpufreq_get_rate+0x34/0xb0
 __cpufreq_get+0x34/0xc0
 show_cpuinfo_cur_freq+0x24/0x78
 show+0x40/0x60
 sysfs_kf_seq_show+0xc0/0x148
 kernfs_seq_show+0x44/0x50
 seq_read+0xd4/0x480
 kernfs_fop_read+0x15c/0x208
 __vfs_read+0x60/0x188
 vfs_read+0x94/0x150
 ksys_read+0x6c/0xd8
 __arm64_sys_read+0x24/0x30
 el0_svc_common+0x78/0x100
 el0_svc_handler+0x38/0x78
 el0_svc+0x8/0xc
---[ end trace 3d1024e58f77f6b2 ]---

So fix the issue by checking if the policy is invalid early in
__cpufreq_get before attempting to get the current frequency.

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 | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

v1->v2:
	- Instead of calling cpufreq_get directly from show_cpuinfo_cur_freq
	  fixed __cpufreq_get to handle invalid policy(moved handling
	  from cpufreq_get into __cpufreq_get)

--
2.17.1

Comments

Viresh Kumar Jan. 8, 2019, 5:03 a.m. UTC | #1 
On 07-01-19, 18:51, Sudeep Holla wrote:
> cpuinfo_cur_freq gets current CPU frequency as detected by hardware
> while scaling_cur_freq last known CPU frequency. Some platforms may not
> allow checking the CPU frequency of an offline CPU or the associated
> resources may have been released via cpufreq_exit when the CPU gets
> offlined, in which case the policy would have been invalidated already.
> If we attempt to get current frequency from the hardware, it may result
> in hang or crash.
> 
> For example on Juno, I see:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000188
> [0000000000000188] pgd=0000000000000000
> Internal error: Oops: 96000004 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 5 PID: 4202 Comm: cat Not tainted 4.20.0-08251-ga0f2c0318a15-dirty #87
> Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform
> pstate: 40000005 (nZcv daif -PAN -UAO)
> pc : scmi_cpufreq_get_rate+0x34/0xb0
> lr : scmi_cpufreq_get_rate+0x34/0xb0
> Call trace:
>  scmi_cpufreq_get_rate+0x34/0xb0
>  __cpufreq_get+0x34/0xc0
>  show_cpuinfo_cur_freq+0x24/0x78
>  show+0x40/0x60
>  sysfs_kf_seq_show+0xc0/0x148
>  kernfs_seq_show+0x44/0x50
>  seq_read+0xd4/0x480
>  kernfs_fop_read+0x15c/0x208
>  __vfs_read+0x60/0x188
>  vfs_read+0x94/0x150
>  ksys_read+0x6c/0xd8
>  __arm64_sys_read+0x24/0x30
>  el0_svc_common+0x78/0x100
>  el0_svc_handler+0x38/0x78
>  el0_svc+0x8/0xc
> ---[ end trace 3d1024e58f77f6b2 ]---
> 
> So fix the issue by checking if the policy is invalid early in
> __cpufreq_get before attempting to get the current frequency.
> 
> 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 | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> v1->v2:
> 	- Instead of calling cpufreq_get directly from show_cpuinfo_cur_freq
> 	  fixed __cpufreq_get to handle invalid policy(moved handling
> 	  from cpufreq_get into __cpufreq_get)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 6f23ebb395f1..e35a886e00bc 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1530,17 +1530,16 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
>  {
>  	unsigned int ret_freq = 0;
> 
> -	if (!cpufreq_driver->get)
> +	if (unlikely(policy_is_inactive(policy)) || !cpufreq_driver->get)
>  		return ret_freq;
> 
>  	ret_freq = cpufreq_driver->get(policy->cpu);
> 
>  	/*
> -	 * Updating inactive policies is invalid, so avoid doing that.  Also
> -	 * if fast frequency switching is used with the given policy, the check
> +	 * If fast frequency switching is used with the given policy, the check
>  	 * against policy->cur is pointless, so skip it in that case too.
>  	 */
> -	if (unlikely(policy_is_inactive(policy)) || policy->fast_switch_enabled)
> +	if (policy->fast_switch_enabled)
>  		return ret_freq;
> 
>  	if (ret_freq && policy->cur &&
> @@ -1569,10 +1568,7 @@ unsigned int cpufreq_get(unsigned int cpu)
> 
>  	if (policy) {
>  		down_read(&policy->rwsem);
> -
> -		if (!policy_is_inactive(policy))
> -			ret_freq = __cpufreq_get(policy);
> -
> +		ret_freq = __cpufreq_get(policy);
>  		up_read(&policy->rwsem);
> 
>  		cpufreq_cpu_put(policy);

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Rafael J. Wysocki Jan. 11, 2019, 10:38 a.m. UTC | #2 
On Tuesday, January 8, 2019 6:03:28 AM CET Viresh Kumar wrote:
> On 07-01-19, 18:51, Sudeep Holla wrote:
> > cpuinfo_cur_freq gets current CPU frequency as detected by hardware
> > while scaling_cur_freq last known CPU frequency. Some platforms may not
> > allow checking the CPU frequency of an offline CPU or the associated
> > resources may have been released via cpufreq_exit when the CPU gets
> > offlined, in which case the policy would have been invalidated already.
> > If we attempt to get current frequency from the hardware, it may result
> > in hang or crash.
> > 
> > For example on Juno, I see:
> > 
> > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000188
> > [0000000000000188] pgd=0000000000000000
> > Internal error: Oops: 96000004 [#1] PREEMPT SMP
> > Modules linked in:
> > CPU: 5 PID: 4202 Comm: cat Not tainted 4.20.0-08251-ga0f2c0318a15-dirty #87
> > Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform
> > pstate: 40000005 (nZcv daif -PAN -UAO)
> > pc : scmi_cpufreq_get_rate+0x34/0xb0
> > lr : scmi_cpufreq_get_rate+0x34/0xb0
> > Call trace:
> >  scmi_cpufreq_get_rate+0x34/0xb0
> >  __cpufreq_get+0x34/0xc0
> >  show_cpuinfo_cur_freq+0x24/0x78
> >  show+0x40/0x60
> >  sysfs_kf_seq_show+0xc0/0x148
> >  kernfs_seq_show+0x44/0x50
> >  seq_read+0xd4/0x480
> >  kernfs_fop_read+0x15c/0x208
> >  __vfs_read+0x60/0x188
> >  vfs_read+0x94/0x150
> >  ksys_read+0x6c/0xd8
> >  __arm64_sys_read+0x24/0x30
> >  el0_svc_common+0x78/0x100
> >  el0_svc_handler+0x38/0x78
> >  el0_svc+0x8/0xc
> > ---[ end trace 3d1024e58f77f6b2 ]---
> > 
> > So fix the issue by checking if the policy is invalid early in
> > __cpufreq_get before attempting to get the current frequency.
> > 
> > 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 | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > v1->v2:
> > 	- Instead of calling cpufreq_get directly from show_cpuinfo_cur_freq
> > 	  fixed __cpufreq_get to handle invalid policy(moved handling
> > 	  from cpufreq_get into __cpufreq_get)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 6f23ebb395f1..e35a886e00bc 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1530,17 +1530,16 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
> >  {
> >  	unsigned int ret_freq = 0;
> > 
> > -	if (!cpufreq_driver->get)
> > +	if (unlikely(policy_is_inactive(policy)) || !cpufreq_driver->get)
> >  		return ret_freq;
> > 
> >  	ret_freq = cpufreq_driver->get(policy->cpu);
> > 
> >  	/*
> > -	 * Updating inactive policies is invalid, so avoid doing that.  Also
> > -	 * if fast frequency switching is used with the given policy, the check
> > +	 * If fast frequency switching is used with the given policy, the check
> >  	 * against policy->cur is pointless, so skip it in that case too.
> >  	 */
> > -	if (unlikely(policy_is_inactive(policy)) || policy->fast_switch_enabled)
> > +	if (policy->fast_switch_enabled)
> >  		return ret_freq;
> > 
> >  	if (ret_freq && policy->cur &&
> > @@ -1569,10 +1568,7 @@ unsigned int cpufreq_get(unsigned int cpu)
> > 
> >  	if (policy) {
> >  		down_read(&policy->rwsem);
> > -
> > -		if (!policy_is_inactive(policy))
> > -			ret_freq = __cpufreq_get(policy);
> > -
> > +		ret_freq = __cpufreq_get(policy);
> >  		up_read(&policy->rwsem);
> > 
> >  		cpufreq_cpu_put(policy);
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Applied, thanks!
diff mbox series

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 6f23ebb395f1..e35a886e00bc 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1530,17 +1530,16 @@  static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
 {
 	unsigned int ret_freq = 0;

-	if (!cpufreq_driver->get)
+	if (unlikely(policy_is_inactive(policy)) || !cpufreq_driver->get)
 		return ret_freq;

 	ret_freq = cpufreq_driver->get(policy->cpu);

 	/*
-	 * Updating inactive policies is invalid, so avoid doing that.  Also
-	 * if fast frequency switching is used with the given policy, the check
+	 * If fast frequency switching is used with the given policy, the check
 	 * against policy->cur is pointless, so skip it in that case too.
 	 */
-	if (unlikely(policy_is_inactive(policy)) || policy->fast_switch_enabled)
+	if (policy->fast_switch_enabled)
 		return ret_freq;

 	if (ret_freq && policy->cur &&
@@ -1569,10 +1568,7 @@  unsigned int cpufreq_get(unsigned int cpu)

 	if (policy) {
 		down_read(&policy->rwsem);
-
-		if (!policy_is_inactive(policy))
-			ret_freq = __cpufreq_get(policy);
-
+		ret_freq = __cpufreq_get(policy);
 		up_read(&policy->rwsem);

 		cpufreq_cpu_put(policy);