lkml.org 
[lkml]   [2022]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3] cpufreq: fix race on cpufreq online
"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Wed, May 11, 2022 at 2:59 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Wed, May 11, 2022 at 2:21 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> >
>> > On 11-05-22, 16:10, Schspa Shi wrote:
>> > > Viresh Kumar <viresh.kumar@linaro.org> writes:
>> > > > 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.
>> > > >
>> > >
>> > > I have checked all the init() implement of the fellowing files, It should be OK.
>> > > Function find command:
>> > > ag "init[\s]+=" drivers/cpufreq
>> > >
>> > > All the init() implement only initialize policy object without holding this lock
>> > > and won't call cpufreq APIs need to hold this lock.
>> >
>> > Okay, we can see if someone complains later then :)
>> >
>> > > > I don't think you can do that safely. offline() or exit() may depend on
>> > > > policy->cpus being set to all CPUs.
>> > > OK, I will move this after exit(). and there will be no effect with those
>> > > two APIs. But policy->cpus must be clear before release policy->rwsem.
>> >
>> > Hmm, I don't think depending on the values of policy->cpus is a good idea to be
>> > honest. This design is inviting bugs to come in at another place. We need a
>> > clear flag for this, a new flag or something like policy_list.
>
> Why?
>
>> > Also I see the same bug happening while the policy is removed. The kobject is
>> > put after the rwsem is dropped.
>
> This shouldn't be a problem because of the wait_for_completion() in
> cpufreq_policy_put_kobj(). It is known that cpufreq_sysfs_release()
> has run when cpufreq_policy_put_kobj() returns, so it is safe to free
> the policy then.
>
>> > > > 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));
>> > > > }
>> > > >
>> > >
>> > > I don't think this fully solves my problem.
>> > > 1. There is some case which cpufreq_online failed after the policy is added to
>> > > cpufreq_policy_list.
>> >
>> > And I missed that :(
>> >
>> > > 2. policy->policy_list is not protected by &policy->rwsem, and we
>> > > can't relay on this to
>> > > indict the policy is fine.
>> >
>> > Ahh..
>> >
>> > > >From this point of view, we can fix this problem through the state of
>> > > this linked list.
>> > > But the above two problems need to be solved first.
>> >
>> > I feel overriding policy_list for this is going to make it complex/messy.
>> >
>> > Maybe something like this then:
>>
>> There are two things.
>>
>> One is the possible race with respect to the sysfs access occurring
>> during failing initialization and the other is that ->offline() or
>> ->exit() can be called with or without holding the policy rwsem
>> depending on the code path.
>>
>> Namely, cpufreq_offline() calls them under the policy rwsem, but
>> cpufreq_remove_dev() calls ->exit() outside the rwsem. Also they are
>> called outside the rwsem in cpufreq_online().
>>
>> Moreover, ->offline() and ->exit() cannot expect policy->cpus to be
>> populated, because they are called when it is empty from
>> cpufreq_offline().
>>
>> So the $subject patch is correct AFAICS even though it doesn't address
>> all of the above.
>
> TBH, I'm not sure why show() doesn't check policy_is_inactive() under the rwsem.
>

There is a exist bugs, and somebody try to fixed, please see commit
Fixes: 2f66196208c9 ("cpufreq: check if policy is inactive early in
__cpufreq_get()")

> Moreover, I'm not sure why the locking dance in store() is necessary.

The store interface hold cpu_hotplug_lock via
cpus_read_trylock();
, cannot run in parallel with cpufreq_online() & cpufreq_offline().

---
BRs

Schspa Shi

\
 
 \ /
  Last update: 2022-05-11 15:43    [W:0.092 / U:1.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site