linux-kernel - Re: [PATCH 1/1] cpufreq: Fix show()/store() issue for hotplugging offline CPU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0h=tPw8K30_yTh6rq3Pbnvx2MzQH5+kiMq3UAeUFXGWRA@mail.gmail.com>
Date:   Wed, 19 Oct 2022 13:47:39 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Guanjun <guanjun@...ux.alibaba.com>
Cc:     schspa@...il.com, rafael@...nel.org, viresh.kumar@...aro.org,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        zelin.deng@...ux.alibaba.com
Subject: Re: [PATCH 1/1] cpufreq: Fix show()/store() issue for hotplugging
 offline CPU

On Wed, Oct 19, 2022 at 10:40 AM Guanjun <guanjun@...ux.alibaba.com> wrote:
>
> From: Zelin Deng <zelin.deng@...ux.alibaba.com>
>
> After brought one CPU offline, lscpu returned failure:
>
> lscpu: cannot read /sys/devices/system/cpu/cpu64/cpufreq/cpuinfo_max_freq: Device or resource busy
>
> which had blocked all outputs of lscpu.

OK, so the policy->cpus mask is empty and -EBUSY is returned.

What's wrong?

> This is not the case mentioned in commit d4627a287e251, as the policy
> had been created successfully but is inactive due to CPU gets offline.

Yes, that's when policy_is_inactive(policy) returns "true" IIUC.

> To fix this issue, just add an addtional check whether CPU is online or
> not.

Which is racy.

Please explain the problem in the first place.

> Fixes: d4627a287e251 ("cpufreq: Abort show()/store() for half-initialized policies")
> Signed-off-by: Zelin Deng <zelin.deng@...ux.alibaba.com>
> Signed-off-by: Guanjun <guanjun@...ux.alibaba.com>
> ---
>  drivers/cpufreq/cpufreq.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 69b3d61852ac..aa238ba7d2fe 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -956,8 +956,12 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
>                 return -EIO;
>
>         down_read(&policy->rwsem);
> -       if (likely(!policy_is_inactive(policy)))
> -               ret = fattr->show(policy, buf);
> +       if (unlikely(policy_is_inactive(policy) && cpu_online(policy->cpu)))
> +               goto err;
> +
> +       ret = fattr->show(policy, buf);
> +
> +err:
>         up_read(&policy->rwsem);
>
>         return ret;
> @@ -974,8 +978,12 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>                 return -EIO;
>
>         down_write(&policy->rwsem);
> -       if (likely(!policy_is_inactive(policy)))
> -               ret = fattr->store(policy, buf, count);
> +       if (unlikely(policy_is_inactive(policy) && cpu_online(policy->cpu)))
> +               goto err;
> +
> +       ret = fattr->store(policy, buf, count);
> +
> +err:
>         up_write(&policy->rwsem);
>
>         return ret;
> --
> 2.32.0.GIT
>