[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMA88TpEHTEYU3Z_sXFcCXmW+9q=Ks+OSkGNY3N+uAisb+G7Ow@mail.gmail.com>
Date: Fri, 22 Apr 2022 23:10:39 +0800
From: Schspa Shi <schspa@...il.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Viresh Kumar <viresh.kumar@...aro.org>,
Linux PM <linux-pm@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cpufreq: fix race on cpufreq online
"Rafael J. Wysocki" <rafael@...nel.org> writes:
> On Wed, Apr 20, 2022 at 9:16 PM Schspa Shi <schspa@...il.com> wrote:
>>
>> When cpufreq online failed, policy->cpus are not empty while
>> cpufreq sysfs file available, we may access some data freed.
>>
>> Take policy->clk as an example:
>>
>> static int cpufreq_online(unsigned int cpu)
>> {
>> ...
>> // policy->cpus != 0 at this time
>> down_write(&policy->rwsem);
>> ret = cpufreq_add_dev_interface(policy);
>> up_write(&policy->rwsem);
>>
>> return 0;
>>
>> out_destroy_policy:
>> for_each_cpu(j, policy->real_cpus)
>> remove_cpu_dev_symlink(policy, get_cpu_device(j));
>> up_write(&policy->rwsem);
>> ...
>> out_exit_policy:
>> if (cpufreq_driver->exit)
>> cpufreq_driver->exit(policy);
>> clk_put(policy->clk);
>> // policy->clk is a wild pointer
>> ...
>> ^
>> |
>> Another process access
>> __cpufreq_get
>> cpufreq_verify_current_freq
>> cpufreq_generic_get
>> // acces wild pointer of policy->clk;
>> |
>> |
>> out_offline_policy: |
>> cpufreq_policy_free(policy); |
>> // deleted here, and will wait for no body reference
>> cpufreq_policy_put_kobj(policy);
>> }
>>
>> Signed-off-by: Schspa Shi <schspa@...il.com>
>> ---
>> drivers/cpufreq/cpufreq.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 80f535cc8a75..0d58b0f8f3af 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1533,8 +1533,6 @@ 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);
>> -
>> out_offline_policy:
>> if (cpufreq_driver->offline)
>> cpufreq_driver->offline(policy);
>> @@ -1543,6 +1541,9 @@ static int cpufreq_online(unsigned int cpu)
>> if (cpufreq_driver->exit)
>> cpufreq_driver->exit(policy);
>>
>> + cpumask_clear(policy->cpus);
>> + up_write(&policy->rwsem);
>
> This change is correct AFAICS, but it doesn't really fix the race,
> because show_cpuinfo_cur_freq() uses __cpufreq_get() directly without
> locking.
There is a lock outside of show_cpuinfo_cur_freq(). Please check about
static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
{
......
down_read(&policy->rwsem);
ret = fattr->show(policy, buf);
up_read(&policy->rwsem);
......
}
>
> It should use cpufreq_get() instead.
>
>> +
>> out_free_policy:
>> cpufreq_policy_free(policy);
>> return ret;
>> --
--
Schspa Shi
BRs
Powered by blists - more mailing lists