Re: [PATCH V3 1/3] cpufreq: Fix locking issues with governors

[Viresh Kumar]

On 26-06-20, 09:24, Quentin Perret wrote:
> On Friday 26 Jun 2020 at 09:21:42 (+0530), Viresh Kumar wrote:
> > The locking around governors handling isn't adequate currently. The list
> > of governors should never be traversed without locking in place. Also we
> > must make sure the governor isn't removed while it is still referenced
> > by code.
> > 
> > Reported-by: Quentin Perret <qperret@xxxxxxxxxx>
> > Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > ---
> >  drivers/cpufreq/cpufreq.c | 59 ++++++++++++++++++++++++---------------
> >  1 file changed, 36 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 0128de3603df..e798a1193bdf 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -621,6 +621,24 @@ static struct cpufreq_governor *find_governor(const char *str_governor)
> >  	return NULL;
> >  }
> >  
> > +static struct cpufreq_governor *get_governor(const char *str_governor)
> > +{
> > +	struct cpufreq_governor *t;
> > +
> > +	mutex_lock(&cpufreq_governor_mutex);
> > +	t = find_governor(str_governor);
> > +	if (!t)
> > +		goto unlock;
> > +
> > +	if (!try_module_get(t->owner))
> > +		t = NULL;
> > +
> > +unlock:
> > +	mutex_unlock(&cpufreq_governor_mutex);
> > +
> > +	return t;
> > +}
> > +
> >  static unsigned int cpufreq_parse_policy(char *str_governor)
> >  {
> >  	if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN))
> > @@ -640,28 +658,14 @@ static struct cpufreq_governor *cpufreq_parse_governor(char *str_governor)
> >  {
> >  	struct cpufreq_governor *t;
> >  
> > -	mutex_lock(&cpufreq_governor_mutex);
> > -
> > -	t = find_governor(str_governor);
> > -	if (!t) {
> > -		int ret;
> > -
> > -		mutex_unlock(&cpufreq_governor_mutex);
> > -
> > -		ret = request_module("cpufreq_%s", str_governor);
> > -		if (ret)
> > -			return NULL;
> > -
> > -		mutex_lock(&cpufreq_governor_mutex);
> > +	t = get_governor(str_governor);
> > +	if (t)
> > +		return t;
> >  
> > -		t = find_governor(str_governor);
> > -	}
> > -	if (t && !try_module_get(t->owner))
> > -		t = NULL;
> > -
> > -	mutex_unlock(&cpufreq_governor_mutex);
> > +	if (request_module("cpufreq_%s", str_governor))
> > +		return NULL;
> >  
> > -	return t;
> > +	return get_governor(str_governor);
> >  }
> >  
> >  /**
> > @@ -815,12 +819,14 @@ static ssize_t show_scaling_available_governors(struct cpufreq_policy *policy,
> >  		goto out;
> >  	}
> >  
> > +	mutex_lock(&cpufreq_governor_mutex);
> >  	for_each_governor(t) {
> >  		if (i >= (ssize_t) ((PAGE_SIZE / sizeof(char))
> >  		    - (CPUFREQ_NAME_LEN + 2)))
> > -			goto out;
> > +			break;
> >  		i += scnprintf(&buf[i], CPUFREQ_NAME_PLEN, "%s ", t->name);
> >  	}
> > +	mutex_unlock(&cpufreq_governor_mutex);
> >  out:
> >  	i += sprintf(&buf[i], "\n");
> >  	return i;
> > @@ -1058,11 +1064,14 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
> >  	struct cpufreq_governor *def_gov = cpufreq_default_governor();
> >  	struct cpufreq_governor *gov = NULL;
> >  	unsigned int pol = CPUFREQ_POLICY_UNKNOWN;
> > +	bool put_governor = false;
> > +	int ret;
> >  
> >  	if (has_target()) {
> >  		/* Update policy governor to the one used before hotplug. */
> > -		gov = find_governor(policy->last_governor);
> > +		gov = get_governor(policy->last_governor);
> >  		if (gov) {
> > +			put_governor = true;
> >  			pr_debug("Restoring governor %s for cpu %d\n",
> >  				 policy->governor->name, policy->cpu);
> >  		} else if (def_gov) {
> > @@ -1089,7 +1098,11 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
> >  			return -ENODATA;
> >  	}
> >  
> > -	return cpufreq_set_policy(policy, gov, pol);
> > +	ret = cpufreq_set_policy(policy, gov, pol);
> > +	if (put_governor)
> > +		module_put(gov->owner);
> 
> Nit: I think you could safely do
> 
> 	if (gov)
> 		module_put(gov->owner);
> 
> and get rid of 'put_governor', given that try_module_get() and
> module_put() are nops if owner is NULL (which is guaranteed for
> the result of cpufreq_default_governor() as it is builtin).

I described why I chose to keep it that way in the other email, but I
am all for dropping the variable. And so what about this ?

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e798a1193bdf..d9e9ae7051bb 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1064,18 +1064,17 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
        struct cpufreq_governor *def_gov = cpufreq_default_governor();
        struct cpufreq_governor *gov = NULL;
        unsigned int pol = CPUFREQ_POLICY_UNKNOWN;
-       bool put_governor = false;
        int ret;
 
        if (has_target()) {
                /* Update policy governor to the one used before hotplug. */
                gov = get_governor(policy->last_governor);
                if (gov) {
-                       put_governor = true;
                        pr_debug("Restoring governor %s for cpu %d\n",
                                 policy->governor->name, policy->cpu);
                } else if (def_gov) {
                        gov = def_gov;
+                       module_get(gov->owner);
                } else {
                        return -ENODATA;
                }
@@ -1099,7 +1098,7 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
        }
 
        ret = cpufreq_set_policy(policy, gov, pol);
-       if (put_governor)
+       if (gov)
                module_put(gov->owner);
 
        return ret;

-- 
viresh