Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220420191541.99528-1-schspa@gmail.com> <CAJZ5v0guPiGx-ZnC0RcqVgDEp0bh4DcKC7QYjjO0PF_3kHdVGg@mail.gmail.com>
In-Reply-To: <CAJZ5v0guPiGx-ZnC0RcqVgDEp0bh4DcKC7QYjjO0PF_3kHdVGg@mail.gmail.com>
From:   Schspa Shi <schspa@gmail.com>
Date:   Fri, 22 Apr 2022 23:10:39 +0800
Message-ID: <CAMA88TpEHTEYU3Z_sXFcCXmW+9q=Ks+OSkGNY3N+uAisb+G7Ow@mail.gmail.com>
Subject: Re: [PATCH] cpufreq: fix race on cpufreq online
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Viresh Kumar <viresh.kumar@linaro.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Wed, Apr 20, 2022 at 9:16 PM Schspa Shi <schspa@gmail.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@gmail.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