linux-kernel - Re: [PATCH V3] cpufreq: Warn users while freeing active policy

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] [day] [month] [year] [list]

Message-ID: <CAJZ5v0i9+xyqcp+Ohmjh8J45xXqdGvNgFQ6WSx8NLcNMYS_khA@mail.gmail.com>
Date:   Fri, 15 Jul 2022 19:20:55 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3] cpufreq: Warn users while freeing active policy

On Thu, Jul 7, 2022 at 11:58 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> With the new design in place, the show() and store() callbacks check if
> the policy is active or not before proceeding any further to avoid
> potential races. And in order to guarantee that cpufreq_policy_free()
> must be called after clearing the policy->cpus mask, i.e. by marking the
> policy inactive.
>
> In order to avoid introducing a bug around this later, print a warning
> message if we end up freeing an active policy.
>
> Also update cpufreq_online() a bit to make sure we clear the cpus mask
> for each error case before calling cpufreq_policy_free().
>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
> V3: BUG_ON() -> pr_warn()
>
>  drivers/cpufreq/cpufreq.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 73432360e6e9..954eef26685f 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1273,6 +1273,13 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>         unsigned long flags;
>         int cpu;
>
> +       /*
> +        * The callers must ensure the policy is inactive by now, to avoid any
> +        * races with show()/store() callbacks.
> +        */
> +       if (unlikely(!policy_is_inactive(policy)))
> +               pr_warn("%s: Freeing active policy\n", __func__);
> +
>         /* Remove policy from list */
>         write_lock_irqsave(&cpufreq_driver_lock, flags);
>         list_del(&policy->policy_list);
> @@ -1527,8 +1534,6 @@ static int cpufreq_online(unsigned int cpu)
>         for_each_cpu(j, policy->real_cpus)
>                 remove_cpu_dev_symlink(policy, j, get_cpu_device(j));
>
> -       cpumask_clear(policy->cpus);
> -
>  out_offline_policy:
>         if (cpufreq_driver->offline)
>                 cpufreq_driver->offline(policy);
> @@ -1538,6 +1543,7 @@ static int cpufreq_online(unsigned int cpu)
>                 cpufreq_driver->exit(policy);
>
>  out_free_policy:
> +       cpumask_clear(policy->cpus);
>         up_write(&policy->rwsem);
>
>         cpufreq_policy_free(policy);
> --

Applied as 5.20 material, thanks!