lkml.org 
[lkml]   [2022]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v4 2/2] cpufreq: make interface functions and lock holding state clear
On Thu, May 12, 2022 at 6:01 PM Schspa Shi <schspa@gmail.com> wrote:
>
> "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/

Ah, OK. Never mind.

\
 
 \ /
  Last update: 2022-05-12 18:25    [W:0.179 / U:0.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site