linux-kernel - Re: [PATCH v3] cpufreq: fix locking order in store_local

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: <20250502050639.2a4mbdav4mdlhbp2@vireshk-i7>
Date: Fri, 2 May 2025 10:36:39 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Seyediman Seyedarab <imandevel@...il.com>
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 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