lkml.org 
[lkml]   [2022]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v3] cpufreq: fix race on cpufreq online
    Don't use in-reply-to for single patches. It is mostly used when you are
    updating a single patch in a patchset and it makes sense to continue the
    discussion in the same thread. In this case, we have a fresh patchset and it
    makes the same thread messy.

    On 10-05-22, 23:42, Schspa Shi wrote:
    > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
    > index 80f535cc8a75..d93958dbdab8 100644
    > --- a/drivers/cpufreq/cpufreq.c
    > +++ b/drivers/cpufreq/cpufreq.c
    > @@ -1337,12 +1337,12 @@ static int cpufreq_online(unsigned int cpu)
    > down_write(&policy->rwsem);
    > policy->cpu = cpu;
    > policy->governor = NULL;
    > - up_write(&policy->rwsem);
    > } else {
    > new_policy = true;
    > policy = cpufreq_policy_alloc(cpu);
    > if (!policy)
    > return -ENOMEM;
    > + down_write(&policy->rwsem);
    > }

    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.

    > if (!new_policy && cpufreq_driver->online) {
    > @@ -1382,7 +1382,6 @@ static int cpufreq_online(unsigned int cpu)
    > cpumask_copy(policy->related_cpus, policy->cpus);
    > }
    >
    > - down_write(&policy->rwsem);
    > /*
    > * affected cpus must always be the one, which are online. We aren't
    > * managing offline cpus here.
    > @@ -1533,7 +1532,7 @@ 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);
    > + cpumask_clear(policy->cpus);

    I don't think you can do that safely. offline() or exit() may depend on
    policy->cpus being set to all CPUs.

    >
    > out_offline_policy:
    > if (cpufreq_driver->offline)
    > @@ -1542,6 +1541,7 @@ static int cpufreq_online(unsigned int cpu)
    > out_exit_policy:
    > if (cpufreq_driver->exit)
    > cpufreq_driver->exit(policy);
    > + up_write(&policy->rwsem);
    >
    > out_free_policy:
    > cpufreq_policy_free(policy);

    Apart from the issues highlighted about, I think we are trying to fix an issue
    locally which can happen at other places as well. Does something like this fix
    your problem at hand ?

    Untested.

    --
    viresh

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

    From e379921d3efa58a40d9565a4506a2580318a437d Mon Sep 17 00:00:00 2001
    Message-Id: <e379921d3efa58a40d9565a4506a2580318a437d.1652243573.git.viresh.kumar@linaro.org>
    From: Viresh Kumar <viresh.kumar@linaro.org>
    Date: Wed, 11 May 2022 09:13:26 +0530
    Subject: [PATCH] cpufreq: Allow sysfs access only for active policies

    It is currently possible, in a corner case, to access the sysfs files
    and reach show_cpuinfo_cur_freq(), etc, for a partly initialized policy.

    This can happen for example if cpufreq_online() fails after adding the
    sysfs files, which are immediately accessed by another process. There
    can easily be other such cases, which aren't identified yet.

    Process A: Process B

    cpufreq_online()
    down_write(&policy->rwsem);
    if (new_policy) {
    ret = cpufreq_add_dev_interface(policy);
    /* This fails after adding few files */
    if (ret)
    goto out_destroy_policy;

    ...
    }

    ...

    out_destroy_policy:
    ...
    up_write(&policy->rwsem);
    /*
    * This will end up accessing the policy
    * which isn't fully initialized.
    */
    show_cpuinfo_cur_freq()

    if (cpufreq_driver->offline)
    cpufreq_driver->offline(policy);

    if (cpufreq_driver->exit)
    cpufreq_driver->exit(policy);

    cpufreq_policy_free(policy);

    Fix these by checking in show/store if the policy is active or not and
    update policy_is_inactive() to also check if the policy is added to the
    global list or not.

    Reported-by: Schspa Shi <schspa@gmail.com>
    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
    ---
    drivers/cpufreq/cpufreq.c | 10 ++++++----
    include/linux/cpufreq.h | 3 ++-
    2 files changed, 8 insertions(+), 5 deletions(-)

    diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
    index fbaa8e6c7d23..036314d05ded 100644
    --- a/drivers/cpufreq/cpufreq.c
    +++ b/drivers/cpufreq/cpufreq.c
    @@ -948,13 +948,14 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
    {
    struct cpufreq_policy *policy = to_policy(kobj);
    struct freq_attr *fattr = to_attr(attr);
    - ssize_t ret;
    + ssize_t ret = -EBUSY;

    if (!fattr->show)
    return -EIO;

    down_read(&policy->rwsem);
    - ret = fattr->show(policy, buf);
    + if (!policy_is_inactive(policy))
    + ret = fattr->show(policy, buf);
    up_read(&policy->rwsem);

    return ret;
    @@ -965,7 +966,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
    {
    struct cpufreq_policy *policy = to_policy(kobj);
    struct freq_attr *fattr = to_attr(attr);
    - ssize_t ret = -EINVAL;
    + ssize_t ret = -EBUSY;

    if (!fattr->store)
    return -EIO;
    @@ -979,7 +980,8 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,

    if (cpu_online(policy->cpu)) {
    down_write(&policy->rwsem);
    - ret = fattr->store(policy, buf, count);
    + if (!policy_is_inactive(policy))
    + ret = fattr->store(policy, buf, count);
    up_write(&policy->rwsem);
    }

    diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
    index 35c7d6db4139..03e5e114c996 100644
    --- a/include/linux/cpufreq.h
    +++ b/include/linux/cpufreq.h
    @@ -209,7 +209,8 @@ static inline void cpufreq_cpu_put(struct cpufreq_policy *policy) { }

    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));
    }

    static inline bool policy_is_shared(struct cpufreq_policy *policy)
    --
    2.31.1.272.g89b43f80a514
    \
     
     \ /
      Last update: 2022-05-11 06:36    [W:7.739 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site