linux-kernel - Re: [PATCH v3] cpufreq: fix race on cpufreq online

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: <20220512065623.q4aa6y52pst3zpxu@vireshk-i7>
Date:   Thu, 12 May 2022 12:26:23 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Schspa Shi <schspa@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v3] cpufreq: fix race on cpufreq online

On 11-05-22, 15:19, Rafael J. Wysocki wrote:
> On Wed, May 11, 2022 at 2:59 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > Hmm, I don't think depending on the values of policy->cpus is a good idea to be
> > > honest. This design is inviting bugs to come in at another place. We need a
> > > clear flag for this, a new flag or something like policy_list.
> 
> Why?

Because it doesn't mean anything unless we have code elsewhere which checks this
specifically. It should be fine though after using policy_is_inactive() in
show/store as you suggested, which I too tried to do in a patch :)

> > > Also I see the same bug happening while the policy is removed. The kobject is
> > > put after the rwsem is dropped.
> 
> This shouldn't be a problem because of the wait_for_completion() in
> cpufreq_policy_put_kobj().  It is known that cpufreq_sysfs_release()
> has run when cpufreq_policy_put_kobj() returns, so it is safe to free
> the policy then.

I agree to that, but the destruction of stuff happens right in
cpufreq_policy_free() where it starts removing the policy from the list and
clears cpufreq_cpu_data. I don't know if it will break anything or not, but we
should disallow any further sysfs operations once we have reached
cpufreq_policy_free().

> TBH, I'm not sure why show() doesn't check policy_is_inactive() under the rwsem.

I agree, both show/store should have it.

> Moreover, I'm not sure why the locking dance in store() is necessary.

commit fdd320da84c6 ("cpufreq: Lock CPU online/offline in cpufreq_register_driver()")

-- 
viresh