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
| ||
|
Message-ID: <CAJZ5v0hFBAi7dNhQC1DJG1Fg44LaR-j-vsxO5=G8VbDj15Kr5Q@mail.gmail.com> Date: Sat, 10 May 2025 13:41:29 +0200 From: "Rafael J. Wysocki" <rafael@...nel.org> To: Seyediman Seyedarab <imandevel@...il.com>, Viresh Kumar <viresh.kumar@...aro.org> Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev Subject: Re: [PATCH v3] cpufreq: fix locking order in store_local_boost to prevent deadlock On Fri, May 9, 2025 at 7:03 PM Rafael J. Wysocki <rafael@...nel.org> wrote: > > On Fri, May 9, 2025 at 7:29 AM Seyediman Seyedarab <imandevel@...il.com> wrote: > > > > On 25/05/02 10:36AM, Viresh Kumar wrote: > > > On 30-04-25, 12:09, Seyediman Seyedarab wrote: > > > > Lockdep reports a possible circular locking dependency[1] when > > > > writing to /sys/devices/system/cpu/cpufreq/policyN/boost, > > > > triggered by power-profiles-daemon at boot. > > > > > > > > store_local_boost() used to acquire cpu_hotplug_lock *after* > > > > the policy lock had already been taken by the store() handler. > > > > However, the expected locking hierarchy is to acquire > > > > cpu_hotplug_lock before the policy guard. This inverted lock order > > > > creates a *theoretical* deadlock possibility. > > > > > > > > Acquire cpu_hotplug_lock in the store() handler *only* for the > > > > local_boost attribute, before entering the policy guard block, > > > > and remove the cpus_read_lock/unlock() calls from store_local_boost(). > > > > Also switch from guard() to scoped_guard() to allow explicitly wrapping > > > > the policy guard inside the cpu_hotplug_lock critical section. > > > > > > > > [1] > > > > ====================================================== > > > > WARNING: possible circular locking dependency detected > > > > 6.15.0-rc4-debug #28 Not tainted > > > > ------------------------------------------------------ > > > > power-profiles-/596 is trying to acquire lock: > > > > ffffffffb147e910 (cpu_hotplug_lock){++++}-{0:0}, at: store_local_boost+0x6a/0xd0 > > > > > > > > but task is already holding lock: > > > > ffff9eaa48377b80 (&policy->rwsem){++++}-{4:4}, at: store+0x37/0x90 > > > > > > > > which lock already depends on the new lock. > > > > > > > > the existing dependency chain (in reverse order) is: > > > > > > > > -> #2 (&policy->rwsem){++++}-{4:4}: > > > > down_write+0x29/0xb0 > > > > cpufreq_online+0x841/0xa00 > > > > cpufreq_add_dev+0x71/0x80 > > > > subsys_interface_register+0x14b/0x170 > > > > cpufreq_register_driver+0x154/0x250 > > > > amd_pstate_register_driver+0x36/0x70 > > > > amd_pstate_init+0x1e7/0x270 > > > > do_one_initcall+0x67/0x2c0 > > > > kernel_init_freeable+0x230/0x270 > > > > kernel_init+0x15/0x130 > > > > ret_from_fork+0x2c/0x50 > > > > ret_from_fork_asm+0x11/0x20 > > > > > > > > -> #1 (subsys mutex#3){+.+.}-{4:4}: > > > > __mutex_lock+0xc2/0x930 > > > > subsys_interface_register+0x83/0x170 > > > > cpufreq_register_driver+0x154/0x250 > > > > amd_pstate_register_driver+0x36/0x70 > > > > amd_pstate_init+0x1e7/0x270 > > > > do_one_initcall+0x67/0x2c0 > > > > kernel_init_freeable+0x230/0x270 > > > > kernel_init+0x15/0x130 > > > > ret_from_fork+0x2c/0x50 > > > > ret_from_fork_asm+0x11/0x20 > > > > > > > > -> #0 (cpu_hotplug_lock){++++}-{0:0}: > > > > __lock_acquire+0x1087/0x17e0 > > > > lock_acquire.part.0+0x66/0x1b0 > > > > cpus_read_lock+0x2a/0xc0 > > > > store_local_boost+0x6a/0xd0 > > > > store+0x50/0x90 > > > > kernfs_fop_write_iter+0x135/0x200 > > > > vfs_write+0x2ab/0x540 > > > > ksys_write+0x6c/0xe0 > > > > do_syscall_64+0xbb/0x1d0 > > > > entry_SYSCALL_64_after_hwframe+0x56/0x5e > > > > > > > > Signed-off-by: Seyediman Seyedarab <ImanDevel@...il.com> > > > > --- > > > > Changes in v3: > > > > - Rebased over PM tree's linux-next branch > > > > - Added a comment to explain why this piece of code is required > > > > - Switched from guard() to scoped_guard() to allow explicitly wrapping > > > > the policy guard inside the cpu_hotplug_lock critical section. > > > > > > > > Changes in v2: > > > > - Restrict cpu_hotplug_lock acquisition to only > > > > the local_boost attribute in store() handler. > > > > > > > > Regards, > > > > Seyediman > > > > > > > > drivers/cpufreq/cpufreq.c | 23 ++++++++++++++++------- > > > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > > > index 21fa733a2..b349adbeb 100644 > > > > --- a/drivers/cpufreq/cpufreq.c > > > > +++ b/drivers/cpufreq/cpufreq.c > > > > @@ -622,10 +622,7 @@ static ssize_t store_local_boost(struct cpufreq_policy *policy, > > > > if (!policy->boost_supported) > > > > return -EINVAL; > > > > > > > > - cpus_read_lock(); > > > > ret = policy_set_boost(policy, enable); > > > > - cpus_read_unlock(); > > > > - > > > > if (!ret) > > > > return count; > > > > > > > > @@ -1006,16 +1003,28 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr, > > > > { > > > > struct cpufreq_policy *policy = to_policy(kobj); > > > > struct freq_attr *fattr = to_attr(attr); > > > > + int ret = -EBUSY; > > > > > > > > if (!fattr->store) > > > > return -EIO; > > > > > > > > - guard(cpufreq_policy_write)(policy); > > > > + /* > > > > + * store_local_boost() requires cpu_hotplug_lock to be held, and must be > > > > + * called with that lock acquired *before* taking policy->rwsem to avoid > > > > + * lock ordering violations. > > > > + */ > > > > + if (fattr == &local_boost) > > > > + cpus_read_lock(); > > > > > > > > - if (likely(!policy_is_inactive(policy))) > > > > - return fattr->store(policy, buf, count); > > > > + scoped_guard(cpufreq_policy_write, policy) { > > > > + if (likely(!policy_is_inactive(policy))) > > > > + ret = fattr->store(policy, buf, count); > > > > + } > > > > > > > > - return -EBUSY; > > > > + if (fattr == &local_boost) > > > > + cpus_read_unlock(); > > > > + > > > > + return ret; > > > > } > > > > > > Acked-by: Viresh Kumar <viresh.kumar@...aro.org> > > > > > > -- > > > viresh > > > > Hi there, > > > > Just following up to see if there's anything you'd like me to > > change or address in the patch before it can move forward. > > Please let me know if any updates are needed. > > I'm kind of wondering why local_boost needs cpus_read_lock() at all. > Holding the policy rwsem blocks CPU online/offline already for this > policy. > > Is that because ->set_boost() may need to synchronize with the other policies? IOW, what can go wrong if the cpus_read_lock() locking is dropped from there altogether?
Powered by blists - more mailing lists