Re: [PATCH v3] cpufreq: fix race on cpufreq online
From: Viresh Kumar
Date: Wed May 11 2022 - 08:21:25 EST
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<-------------------------