| 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: <20220511043515.fn2gz6q3kcpdai5p@vireshk-i7>
Date: Wed, 11 May 2022 10:05:15 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Schspa Shi <schspa@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
rafael@...nel.org
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@...aro.org>
From: Viresh Kumar <viresh.kumar@...aro.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@...il.com>
Signed-off-by: Viresh Kumar <viresh.kumar@...aro.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
Powered by blists - more mailing lists