linux-kernel - Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy

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]

Date:   Wed, 6 Jul 2022 15:49:09 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        kernel test robot <oliver.sang@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()

On Wed, Jun 15, 2022 at 7:00 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 14-06-22, 15:59, Rafael J. Wysocki wrote:
> > On Fri, May 27, 2022 at 5:53 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
> > >
> > > With the new design in place, to avoid potential races show() and
> > > store() callbacks check if the policy is active or not before proceeding
> > > any further. And in order to guarantee that cpufreq_policy_free() must
> > > be called after clearing the policy->cpus mask, i.e. by marking it
> > > inactive.
> > >
> > > Lets make sure we don't get a bug around this later and catch this early
> > > by putting a BUG_ON() within cpufreq_policy_free().
> > >
> > > Also update cpufreq_online() a bit to make sure we clear the cpus mask
> > > for each error case before calling cpufreq_policy_free().
> > >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> > > ---
> > > V2: Update cpufreq_online() and changelog.
> > >
> > >  drivers/cpufreq/cpufreq.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index e24aa5d4bca5..0f8245731783 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -1284,6 +1284,12 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > >         unsigned long flags;
> > >         int cpu;
> > >
> > > +       /*
> > > +        * The callers must ensure the policy is inactive by now, to avoid any
> > > +        * races with show()/store() callbacks.
> > > +        */
> > > +       BUG_ON(!policy_is_inactive(policy));
> >
> > I'm not a super-big fan of this change.
> >
> > First off, crashing the kernel outright here because of possible races
> > appears a bit excessive to me.
> >
> > Second, it looks like we are worrying about the code running before
> > the wait_for_completion() call in cpufreq_policy_put_kobj(), because
> > after that call no one can be running show() or store().  So why don't
> > we reorder the wait_for_completion() call with respect to the code in
> > question instead?
>
> No, I am not worrying about that race. I am just trying to make sure some change
> in future doesn't break this assumption (that policy should be inactive by this
> point). That's all. It all looks good for now.
>
> May be a WARN instead of BUG if we don't want to crash.

WARN_ON() would be somewhat better, but then I'm not sure if having a
full call trace in this case is really useful, because we know when
cpufreq_policy_free() can be called anyway.

Maybe just print a warning message.