Re: [PATCH v3] cpufreq: fix race on cpufreq online

[Viresh Kumar]

On 11-05-22, 16:10, Schspa Shi wrote:
> Viresh Kumar <viresh.kumar@xxxxxxxxxx> 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.

Also I see the same bug happening while the policy is removed. The kobject is
put after the rwsem is dropped.

> >  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:

-------------------------8<-------------------------