cpufreq: Fix race between sysfs writes and hotplug/policy update
Subsystems (2)
PATCH 1/1 cpufreq: Fix race between sysfs writes and hotplug/policy update
The sysfs store ops need to grab the policy write semaphore to avoid race with hotplug and cpufreq_update_policy() calls. Without this, we could end up with simultaneous calls to cpufreq_driver->target()
---
There still seem to be race conditions between cpufreq_update_policy() and the cpufreq hotplug path. But that seems more complicated to fix. So, leaving that for later.
drivers/cpufreq/cpufreq.c | 10 ++++++++++ drivers/cpufreq/cpufreq_userspace.c | 11 +---------- 2 files changed, 11 insertions(+), 10 deletions(-)
drivers/cpufreq/cpufreq.c +10 -0
@@ -420,9 +420,13 @@ static ssize_t store_##file_name \if (ret !=1 ) \return -EINVAL ; \ \ +lock_policy_rwsem_write (policy->cpu); \ + \ ret =__cpufreq_set_policy (policy, &new_policy); \ policy->user_policy.object = policy->object; \ \ +unlock_policy_rwsem_write (policy->cpu); \ + \return ret ? ret : count; \ }
@@ -480,6 +484,8 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, &new_policy.governor))return -EINVAL ; +lock_policy_rwsem_write (policy->cpu); +/* Do not use cpufreq_set_policy here or the user_policy.max will be wrongly overridden */ ret =__cpufreq_set_policy (policy, &new_policy);
@@ -487,6 +493,8 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, policy->user_policy.policy = policy->policy; policy->user_policy.governor = policy->governor; +unlock_policy_rwsem_write (policy->cpu); +if (ret)return ret;else
@@ -572,7 +580,9 @@ static ssize_t store_scaling_setspeed(struct cpufreq_policy *policy,if (ret !=1 )return -EINVAL ; +lock_policy_rwsem_write (policy->cpu); policy->governor->store_setspeed (policy, freq); +unlock_policy_rwsem_write (policy->cpu);return count; }
drivers/cpufreq/cpufreq_userspace.c +1 -10
@@ -87,16 +87,7 @@ static int cpufreq_set(struct cpufreq_policy *policy, unsigned int freq)if (freq >per_cpu (cpu_max_freq, policy->cpu)) freq =per_cpu (cpu_max_freq, policy->cpu); -/* -* We're safe from concurrent calls to ->target() here -* as we hold the userspace_mutex lock. If we were calling -* cpufreq_driver_target, a deadlock situation might occur: -* A: cpufreq_set (lock userspace_mutex) -> -* cpufreq_driver_target(lock policy->lock) -* B: cpufreq_set_policy(lock policy->lock) -> -* __cpufreq_governor -> -* cpufreq_governor_userspace (lock userspace_mutex) -*/ +/* The cpufreq framework holds the lock before calling this op. */ ret =__cpufreq_driver_target (policy, freq,CPUFREQ_RELATION_L ); err:
Signature · 4 lines
1.7.8.3 The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Viresh Kumar
maintainerHi Saravana,
Quoted 3 lines · Saravana Kannan
The sysfs store ops need to grab the policy write semaphore to avoid race with hotplug and
cpufreq_update_policy()calls. Without this, we could end up with simultaneous calls tocpufreq_driver->target()
Interesting.
Quoted 7 lines
drivers/cpufreq/cpufreq.c +0
-0@@ -420,9 +420,13 @@ static ssize_t store_##file_name \if (ret !=1 ) \return -EINVAL ; \
drivers/cpufreq/cpufreq.c static ssize_t store_##file_name \ · line 431
Quoted 10 lines
drivers/cpufreq/cpufreq.c +4
-0\ +lock_policy_rwsem_write (policy->cpu); \ + \ ret =__cpufreq_set_policy (policy, &new_policy); \ policy->user_policy.object = policy->object; \ \ +unlock_policy_rwsem_write (policy->cpu); \ + \return ret ? ret : count; \ }
As far as I know, this protection already exists. Check this function, which eventually calls all **store() related to struct freq_attr
static ssize_t store(struct kobject *kobj, struct attribute *attr,
const char *buf, size_t count)
{
struct cpufreq_policy *policy = to_policy(kobj);
struct freq_attr *fattr = to_attr(attr);
ssize_t ret = -EINVAL;
policy = cpufreq_cpu_get_sysfs(policy->cpu);
if (!policy)
goto no_policy;
if (lock_policy_rwsem_write(policy->cpu) < 0)
goto fail;
if (fattr->store)
ret = fattr->store(policy, buf, count);
else
ret = -EIO;
unlock_policy_rwsem_write(policy->cpu);
fail:
cpufreq_cpu_put_sysfs(policy);
no_policy:
return ret;
}Quoted 47 lines · Viresh Kumar
Hi Saravana,
Quoted 3 lines · Saravana Kannan
The sysfs store ops need to grab the policy write semaphore to avoid race with hotplug and
cpufreq_update_policy()calls. Without this, we could end up with simultaneous calls tocpufreq_driver->target()Interesting.
Quoted 7 lines
drivers/cpufreq/cpufreq.c +0
-0@@ -420,9 +420,13 @@ static ssize_t store_##file_name \if (ret !=1 ) \return -EINVAL ; \drivers/cpufreq/cpufreq.c static ssize_t store_##file_name \ · line 431
Quoted 10 lines
drivers/cpufreq/cpufreq.c +4
-0\ +lock_policy_rwsem_write (policy->cpu); \ + \ ret =__cpufreq_set_policy (policy, &new_policy); \ policy->user_policy.object = policy->object; \ \ +unlock_policy_rwsem_write (policy->cpu); \ + \return ret ? ret : count; \ }As far as I know, this protection already exists. Check this function, which eventually calls all **store() related to struct
freq_attrstatic
ssize_tstore(struct kobject *kobj, struct attribute *attr,
const char *buf,size_tcount){ struct cpufreq_policy *policy = to_policy(kobj); struct freq_attr *fattr = to_attr(attr); ssize_t ret = -EINVAL; policy = cpufreq_cpu_get_sysfs(policy->cpu); if (!policy) goto no_policy; if (lock_policy_rwsem_write(policy->cpu) < 0) goto fail; if (fattr->store) ret = fattr->store(policy, buf, count); else
Quoted 9 lines
ret = -EIO; unlock_policy_rwsem_write(policy->cpu); fail: cpufreq_cpu_put_sysfs(policy); no_policy: return ret; }
You are right. I did look at this, but looks like I skimmed some code too fast. But the race is certainly happening. I'll have to dig deeper I guess. I do see some patches you have for serializing driver->target() calls. Haven't looked at them yet, but maybe they'll help.
I'll dig deeper.
Thanks,
Saravana
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation