| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0j6vRpcJxJnSg_Ph02g_4H2LomPM6Ed9t4gaD7YeNE_pg@mail.gmail.com>
Date: Mon, 29 Jun 2020 15:13:46 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Quentin Perret <qperret@...gle.com>,
Rafael Wysocki <rjw@...ysocki.net>,
Linux PM <linux-pm@...r.kernel.org>,
Vincent Guittot <vincent.guittot@...aro.org>,
"Cc: Android Kernel" <kernel-team@...roid.com>,
Todd Kjos <tkjos@...gle.com>, adharmap@...eaurora.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 1/3] cpufreq: Fix locking issues with governors
On Mon, Jun 29, 2020 at 4:13 AM Viresh Kumar <viresh.kumar@...aro.org> 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@...gle.com>
> > > Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> > > ---
> > > 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
Powered by blists - more mailing lists