| Message ID | 20220506170035.32115-1-schspa@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: fix double unlock when cpufreq online | expand |
On 07-05-22, 01:00, Schspa Shi wrote: > The patch f346e96267cd: ("cpufreq: Fix possible race in cpufreq online > error path") expand the critical region. But policy->rwsem is not held when > calling cpufreq_driver->online and cpufreq_driver->init calls, which lead to bad > unlock. > > And it's well to hold this lock when calling cpufreq_driver->online, which > provide more protects without bad influence. > > Fixes: f346e96267cd: ("cpufreq: Fix possible race in cpufreq online error path") > Link: https://lore.kernel.org/all/YnKZCGaig+EXSowf@kili/ > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Schspa Shi <schspa@gmail.com> > --- > drivers/cpufreq/cpufreq.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 0d58b0f8f3af..43dfaa8124e2 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); > } > > 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. > @@ -1542,9 +1541,9 @@ static int cpufreq_online(unsigned int cpu) > cpufreq_driver->exit(policy); > > cpumask_clear(policy->cpus); > - up_write(&policy->rwsem); > > out_free_policy: > + up_write(&policy->rwsem); > cpufreq_policy_free(policy); > return ret; > } Since this is a tricky piece of code, I suggest sending a fresh patch to fix the original issue over the revert I have sent. I am not yet sure both patches combined will fix it correctly.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 0d58b0f8f3af..43dfaa8124e2 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); } 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. @@ -1542,9 +1541,9 @@ static int cpufreq_online(unsigned int cpu) cpufreq_driver->exit(policy); cpumask_clear(policy->cpus); - up_write(&policy->rwsem); out_free_policy: + up_write(&policy->rwsem); cpufreq_policy_free(policy); return ret; }
The patch f346e96267cd: ("cpufreq: Fix possible race in cpufreq online error path") expand the critical region. But policy->rwsem is not held when calling cpufreq_driver->online and cpufreq_driver->init calls, which lead to bad unlock. And it's well to hold this lock when calling cpufreq_driver->online, which provide more protects without bad influence. Fixes: f346e96267cd: ("cpufreq: Fix possible race in cpufreq online error path") Link: https://lore.kernel.org/all/YnKZCGaig+EXSowf@kili/ Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Schspa Shi <schspa@gmail.com> --- drivers/cpufreq/cpufreq.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)