Re: [PATCH 09/12] cpufreq: governor: Avoid invalid states with additional checks

[Preeti U Murthy <preeti@linux.vnet.ibm.com>]

On 06/11/2015 04:21 PM, Viresh Kumar wrote:
> There can be races where the request has come to a wrong state. For
> example INIT followed by STOP (instead of START) or START followed by
> EXIT (instead of STOP).
> 
> Also make sure, once we have started canceling queued works, we don't
> queue any new works. That can lead to the case where the work-handler
> finds many data structures are freed and so NULL pointer exceptions.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_governor.c | 56 ++++++++++++++++++++++++++++++--------
>  drivers/cpufreq/cpufreq_governor.h |  1 +
>  2 files changed, 45 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index aa24aa9a9eb3..ee2e19a1218a 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -169,8 +169,12 @@ static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
>  void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
>  		unsigned int delay, bool all_cpus)
>  {
> +	struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(policy->cpu);
>  	int i;
> 
> +	if (!cdbs->ccdbs->enabled)
> +		return;

policy->governor_enabled is already doing this job. Why this additional
check ?

> +
>  	mutex_lock(&cpufreq_governor_lock);
>  	if (!policy->governor_enabled)
>  		goto out_unlock;
> @@ -234,6 +238,8 @@ static void dbs_timer(struct work_struct *work)
>  	bool modify_all = true;
> 
>  	mutex_lock(&dbs_data->cdata->mutex);
> +	if (!cdbs->ccdbs->enabled)
> +		goto unlock;

This should not trigger at all if we get the entries into
cpufreq_governor_dbs() fixed. I don't like the idea of adding
checks/locks in places where it can be avoided.

> 
>  	if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
>  		struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
> @@ -251,6 +257,7 @@ static void dbs_timer(struct work_struct *work)
>  	delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, modify_all);
>  	gov_queue_work(dbs_data, policy, delay, modify_all);
> 
> +unlock:
>  	mutex_unlock(&dbs_data->cdata->mutex);
>  }
> 
> @@ -376,10 +383,15 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy,
>  	return ret;
>  }
> 
> -static void cpufreq_governor_exit(struct cpufreq_policy *policy,
> -				  struct dbs_data *dbs_data)
> +static int cpufreq_governor_exit(struct cpufreq_policy *policy,
> +				 struct dbs_data *dbs_data)
>  {
>  	struct common_dbs_data *cdata = dbs_data->cdata;
> +	struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(policy->cpu);
> +
> +	/* STOP should have been called by now */

This is not true, atleast in the races that I have seen. The problem is
not about STOP not being called before an EXIT. It is about a START
being called after a STOP and before an EXIT. The comment should ideally
be "The policy is active, stop it before exit" or similar.

> +	if (cdbs->ccdbs->enabled)
> +		return -EBUSY;

And.. in such a scenario, we must not be aborting EXIT; rather it must
cancel the queued work and successfully exit the policy. An EXIT is a
more urgent operation than START, given its call sites. Also an EXIT
will not leave the cpufreq governors in a limbo state, it is bound to
restart a new policy or quit a policy if the last cpu goes down. A
racing START operation however is typically from a call site referencing
an older policy. Its better to abort this than the EXIT operation.

It may mean a user is trying to switch governors, and the exit operation
is quitting the old governor as a result. A START from a
cpufreq_remove_dev_finish() racing in just before this is no reason to
prevent switching governors.

> 
>  	policy->governor_data = NULL;
>  	if (!--dbs_data->usage_count) {
> @@ -395,6 +407,8 @@ static void cpufreq_governor_exit(struct cpufreq_policy *policy,
>  		free_ccdbs(policy, cdata);
>  		kfree(dbs_data);
>  	}
> +
> +	return 0;
>  }
> 
>  static int cpufreq_governor_start(struct cpufreq_policy *policy,
> @@ -409,6 +423,10 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,
>  	if (!policy->cur)
>  		return -EINVAL;
> 
> +	/* START shouldn't be already called */
> +	if (ccdbs->enabled)
> +		return -EBUSY;

Why not reuse policy->governor_enabled in each of these places ?

Regards
Preeti U Murthy