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

[Rafael J. Wysocki]

On Mon, Jun 29, 2020 at 4:13 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> 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 ?

Works for me.

> 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