Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220510035259.5ep52sgahd2a6rie@vireshk-i7> <20220510154236.88753-1-schspa@gmail.com>
 <20220511043515.fn2gz6q3kcpdai5p@vireshk-i7> <CAMA88TpefB=rnqea2u1zEvNUJNE_kdj4mYito7SGCuMj-o071Q@mail.gmail.com>
 <20220511122114.wccgyur6g3qs6fps@vireshk-i7> <CAJZ5v0gN_yDFpvCXRXv8rN-i3TugCi-HKpBKK2z4eWU0Zm1GUg@mail.gmail.com>
 <CAJZ5v0id+7vkqMQEyVRe29oF_dRtzZ0EhoYUn8=yzeENDeABJw@mail.gmail.com> <CAMA88TpJuJY7oxPFW8xKzch60+n_2qRp7KO2r-YwZxiM7GXF3w@mail.gmail.com>
In-Reply-To: <CAMA88TpJuJY7oxPFW8xKzch60+n_2qRp7KO2r-YwZxiM7GXF3w@mail.gmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 11 May 2022 15:50:11 +0200
Message-ID: <CAJZ5v0hkdr2N65p84X0HdVCkqr621=rE4cFmqFMMXTvn6=BCAw@mail.gmail.com>
Subject: Re: [PATCH v3] cpufreq: fix race on cpufreq online
To:     Schspa Shi <schspa@gmail.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, May 11, 2022 at 3:42 PM Schspa Shi <schspa@gmail.com> wrote:
>
> "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()")

Well, exactly.

This only addressed one bug out of a category.

> > 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().

So the reason why is to prevent store() from running in parallel with
the two functions above.  Which generally  is because the policy
configuration is in-flight then.  However, I'm wondering about what
exactly would break then.