Messages in this thread Patch in this message | | | Date | Wed, 11 May 2022 10:05:15 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH v3] cpufreq: fix race on cpufreq online |
| |
Don't use in-reply-to for single patches. It is mostly used when you are updating a single patch in a patchset and it makes sense to continue the discussion in the same thread. In this case, we have a fresh patchset and it makes the same thread messy.
On 10-05-22, 23:42, Schspa Shi wrote: > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 80f535cc8a75..d93958dbdab8 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1337,12 +1337,12 @@ static int cpufreq_online(unsigned int cpu) > down_write(&policy->rwsem); > policy->cpu = cpu; > policy->governor = NULL; > - up_write(&policy->rwsem); > } else { > new_policy = true; > policy = cpufreq_policy_alloc(cpu); > if (!policy) > return -ENOMEM; > + down_write(&policy->rwsem); > }
I am not sure, but maybe there were issues in calling init() with rwsem held, as it may want to call some API from there.
> if (!new_policy && cpufreq_driver->online) { > @@ -1382,7 +1382,6 @@ static int cpufreq_online(unsigned int cpu) > cpumask_copy(policy->related_cpus, policy->cpus); > } > > - down_write(&policy->rwsem); > /* > * affected cpus must always be the one, which are online. We aren't > * managing offline cpus here. > @@ -1533,7 +1532,7 @@ static int cpufreq_online(unsigned int cpu) > for_each_cpu(j, policy->real_cpus) > remove_cpu_dev_symlink(policy, get_cpu_device(j)); > > - up_write(&policy->rwsem); > + cpumask_clear(policy->cpus);
I don't think you can do that safely. offline() or exit() may depend on policy->cpus being set to all CPUs.
> > out_offline_policy: > if (cpufreq_driver->offline) > @@ -1542,6 +1541,7 @@ static int cpufreq_online(unsigned int cpu) > out_exit_policy: > if (cpufreq_driver->exit) > cpufreq_driver->exit(policy); > + up_write(&policy->rwsem); > > out_free_policy: > cpufreq_policy_free(policy);
Apart from the issues highlighted about, I think we are trying to fix an issue locally which can happen at other places as well. Does something like this fix your problem at hand ?
Untested.
-- viresh
-------------------------8<-------------------------
From e379921d3efa58a40d9565a4506a2580318a437d Mon Sep 17 00:00:00 2001 Message-Id: <e379921d3efa58a40d9565a4506a2580318a437d.1652243573.git.viresh.kumar@linaro.org> From: Viresh Kumar <viresh.kumar@linaro.org> Date: Wed, 11 May 2022 09:13:26 +0530 Subject: [PATCH] cpufreq: Allow sysfs access only for active policies
It is currently possible, in a corner case, to access the sysfs files and reach show_cpuinfo_cur_freq(), etc, for a partly initialized policy.
This can happen for example if cpufreq_online() fails after adding the sysfs files, which are immediately accessed by another process. There can easily be other such cases, which aren't identified yet.
Process A: Process B
cpufreq_online() down_write(&policy->rwsem); if (new_policy) { ret = cpufreq_add_dev_interface(policy); /* This fails after adding few files */ if (ret) goto out_destroy_policy;
... }
...
out_destroy_policy: ... up_write(&policy->rwsem); /* * This will end up accessing the policy * which isn't fully initialized. */ show_cpuinfo_cur_freq()
if (cpufreq_driver->offline) cpufreq_driver->offline(policy);
if (cpufreq_driver->exit) cpufreq_driver->exit(policy);
cpufreq_policy_free(policy);
Fix these by checking in show/store if the policy is active or not and update policy_is_inactive() to also check if the policy is added to the global list or not.
Reported-by: Schspa Shi <schspa@gmail.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 10 ++++++---- include/linux/cpufreq.h | 3 ++- 2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index fbaa8e6c7d23..036314d05ded 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -948,13 +948,14 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf) { struct cpufreq_policy *policy = to_policy(kobj); struct freq_attr *fattr = to_attr(attr); - ssize_t ret; + ssize_t ret = -EBUSY; if (!fattr->show) return -EIO; down_read(&policy->rwsem); - ret = fattr->show(policy, buf); + if (!policy_is_inactive(policy)) + ret = fattr->show(policy, buf); up_read(&policy->rwsem); return ret; @@ -965,7 +966,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr, { struct cpufreq_policy *policy = to_policy(kobj); struct freq_attr *fattr = to_attr(attr); - ssize_t ret = -EINVAL; + ssize_t ret = -EBUSY; if (!fattr->store) return -EIO; @@ -979,7 +980,8 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr, if (cpu_online(policy->cpu)) { down_write(&policy->rwsem); - ret = fattr->store(policy, buf, count); + if (!policy_is_inactive(policy)) + ret = fattr->store(policy, buf, count); up_write(&policy->rwsem); } diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 35c7d6db4139..03e5e114c996 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -209,7 +209,8 @@ static inline void cpufreq_cpu_put(struct cpufreq_policy *policy) { } static inline bool policy_is_inactive(struct cpufreq_policy *policy) { - return cpumask_empty(policy->cpus); + return unlikely(cpumask_empty(policy->cpus) || + list_empty(&policy->policy_list)); } static inline bool policy_is_shared(struct cpufreq_policy *policy) -- 2.31.1.272.g89b43f80a514
| |