linux-kernel - Re: [PATCH v4 2/2] cpufreq: make interface functions and lock holding state clear

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0htGfZ3G+BWgZSxvcEAhNd_LONm8rNMetdPts3uZ=Tcxw@mail.gmail.com>
Date:   Thu, 12 May 2022 16:44:30 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Schspa Shi <schspa@...il.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] cpufreq: make interface functions and lock holding
 state clear

On Thu, May 12, 2022 at 3:52 PM Schspa Shi <schspa@...il.com> wrote:
>
> cpufreq_offline() calls offline() and exit() under the policy rwsem
> But they are called outside the rwsem in cpufreq_online().
>
> This patch move the offline(), exit(), online(), init() to be inside
> of policy rwsem to achieve a clear lock relationship.
>
> All the init() online() implement only initialize policy object without
> holding this lock and won't call cpufreq APIs need to hold this lock.
>
> Signed-off-by: Schspa Shi <schspa@...il.com>

IMV this still addresses 2 different issues and so it should be split
into 2 different patches.

> ---
>  drivers/cpufreq/cpufreq.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 35dffd738580..f242d5488364 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c

Patch 1:

> @@ -1343,12 +1343,12 @@ static int cpufreq_online(unsigned int cpu)
>                 down_write(&policy->rwsem);
>                 policy->cpu = cpu;
>                 policy->governor = NULL;
> -               up_write(&policy->rwsem);
>         } else {
>                 new_policy = true;
>                 policy = cpufreq_policy_alloc(cpu);
>                 if (!policy)
>                         return -ENOMEM;
> +               down_write(&policy->rwsem);
>         }
>
>         if (!new_policy && cpufreq_driver->online) {
> @@ -1388,7 +1388,6 @@ static int cpufreq_online(unsigned int cpu)
>                 cpumask_copy(policy->related_cpus, policy->cpus);
>         }
>
> -       down_write(&policy->rwsem);
>         /*
>          * affected cpus must always be the one, which are online. We aren't
>          * managing offline cpus here.

which addresses the problem that cpufreq_online() updates the
policy->cpus and related_cpus masks without holding the policy rwsem
(since the policy kobject has been registered already at this point,
this is generally unsafe).

A side-effect of it is that ->online() and ->init() will be called
under the policy rwsem now, but that should be fine and is more
consistent than the current code too.

Patch 2:

> @@ -1540,7 +1539,6 @@ static int cpufreq_online(unsigned int cpu)
>                 remove_cpu_dev_symlink(policy, get_cpu_device(j));
>
>         cpumask_clear(policy->cpus);
> -       up_write(&policy->rwsem);
>
>  out_offline_policy:
>         if (cpufreq_driver->offline)
> @@ -1549,6 +1547,7 @@ static int cpufreq_online(unsigned int cpu)
>  out_exit_policy:
>         if (cpufreq_driver->exit)
>                 cpufreq_driver->exit(policy);
> +       up_write(&policy->rwsem);
>
>  out_free_policy:
>         cpufreq_policy_free(policy);
> --

which addressed the issue of calling ->offline() and ->exit() without
holding the policy rwsem that is at best inconsistent with
cpufreq_offline().