[02/10] cpufreq: conservative: Avoid races with transition notifier

Message ID 41ef05ed3b93677b4519e4c6c758753a7e63d432.1434959517.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar June 22, 2015, 8:02 a.m.
It is possible that cpufreq transition notifier is called while the
governor is performing its EXIT operation. If this happens, 'ccdbs'
may get updated to NULL, while it is being accessed from the notifier
callback. And that will result in NULL pointer dereference.

ccdbs is used here just to get cpufreq policy, which can be obtained
from cpufreq_cpu_get() as well. And so the reference to ccdbs can be
avoided.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Viresh Kumar June 24, 2015, 1:11 a.m. | #1
On 23-06-15, 21:23, Preeti U Murthy wrote:
> On 06/22/2015 01:32 PM, Viresh Kumar wrote:
> > It is possible that cpufreq transition notifier is called while the
> > governor is performing its EXIT operation. If this happens, 'ccdbs'
> 
> When does this happen ? As far as I can see, cpufreq transition notifier
> gets called from the cpufreq kworker or when we set the cpufreq limits.

Which kworker are you talking about here ? The work-handlers of
ondemand/conservative governors ?

Conservative governor has registered for transition notifier and that
will be called every time frequency of a CPU is updated. And that has
nothing to do with the governor callbacks. These notifiers are called
from the ->target() routines of the drivers.

> And from your previous patches, an exit operation only proceeds after
> ensuring that no kworker is running (check on ccdbs->policy). And LIMIT
> operation does not run in parallel too.

Does it look better from the earlier description?
Viresh Kumar June 25, 2015, 7:59 a.m. | #2
On 24-06-15, 06:41, Viresh Kumar wrote:
> On 23-06-15, 21:23, Preeti U Murthy wrote:
> > On 06/22/2015 01:32 PM, Viresh Kumar wrote:
> > > It is possible that cpufreq transition notifier is called while the
> > > governor is performing its EXIT operation. If this happens, 'ccdbs'
> > 
> > When does this happen ? As far as I can see, cpufreq transition notifier
> > gets called from the cpufreq kworker or when we set the cpufreq limits.
> 
> Which kworker are you talking about here ? The work-handlers of
> ondemand/conservative governors ?
> 
> Conservative governor has registered for transition notifier and that
> will be called every time frequency of a CPU is updated. And that has
> nothing to do with the governor callbacks. These notifiers are called
> from the ->target() routines of the drivers.
> 
> > And from your previous patches, an exit operation only proceeds after
> > ensuring that no kworker is running (check on ccdbs->policy). And LIMIT
> > operation does not run in parallel too.

Hmm, you were right. I Nack my own patch. :)

Patch

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 0e4154e584bf..1e3cabfb2b57 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -119,12 +119,13 @@  static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
 	struct cpufreq_freqs *freq = data;
 	struct cs_cpu_dbs_info_s *dbs_info =
 					&per_cpu(cs_cpu_dbs_info, freq->cpu);
-	struct cpufreq_policy *policy;
+	struct cpufreq_policy *policy = cpufreq_cpu_get(freq->cpu);
 
-	if (!dbs_info->enable)
+	if (!policy)
 		return 0;
 
-	policy = dbs_info->cdbs.ccdbs->policy;
+	if (!dbs_info->enable)
+		goto policy_put;
 
 	/*
 	 * we only care if our internally tracked freq moves outside the 'valid'
@@ -134,6 +135,9 @@  static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
 			|| dbs_info->requested_freq < policy->min)
 		dbs_info->requested_freq = freq->new;
 
+policy_put:
+	cpufreq_cpu_put(policy);
+
 	return 0;
 }