Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220512135231.10076-1-schspa@gmail.com> <20220512135231.10076-2-schspa@gmail.com>
 <CAJZ5v0htGfZ3G+BWgZSxvcEAhNd_LONm8rNMetdPts3uZ=Tcxw@mail.gmail.com>
In-Reply-To: <CAJZ5v0htGfZ3G+BWgZSxvcEAhNd_LONm8rNMetdPts3uZ=Tcxw@mail.gmail.com>
From:   Schspa Shi <schspa@gmail.com>
Date:   Fri, 13 May 2022 00:01:16 +0800
Message-ID: <CAMA88TodZJYmd2GnWty=qCw7T=LG9jihEAmT+RPK8tSBqdiubA@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] cpufreq: make interface functions and lock holding
 state clear
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Viresh Kumar <viresh.kumar@linaro.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Thu, May 12, 2022 at 3:52 PM Schspa Shi <schspa@gmail.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@gmail.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().

No, we can't split this into two different patches. which will cause a
uninitialized unlock for policy rwsem.
This will make the git bitsec unusable.

Which Dan Carpenter reported, and cause the patch of the v1 version to
be reverted.

Link: https://lore.kernel.org/all/YnKZCGaig+EXSowf@kili/

---
BRs


Schspa Shi