| 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: <6nfpd4jvx7dmpl5hqk5nokxprxbcccah66x2mptgwxtyxmb74r@qiosxhaw3tew>
Date: Fri, 9 May 2025 01:32:39 -0400
From: Seyediman Seyedarab <imandevel@...il.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: rafael@...nel.org, 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 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.
Regards,
Seyediman
Powered by blists - more mailing lists