linux-kernel - Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0gswDEdK9-gC1dPr9BFPv3G9rs+JYe-7=1JZ11OMoBb-g@mail.gmail.com>
Date:   Tue, 14 Jun 2022 15:59:01 +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>,
        kernel test robot <oliver.sang@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()

On Fri, May 27, 2022 at 5:53 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> With the new design in place, to avoid potential races show() and
> store() callbacks check if the policy is active or not before proceeding
> any further. And in order to guarantee that cpufreq_policy_free() must
> be called after clearing the policy->cpus mask, i.e. by marking it
> inactive.
>
> Lets make sure we don't get a bug around this later and catch this early
> by putting a BUG_ON() within cpufreq_policy_free().
>
> 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>
> ---
> V2: Update cpufreq_online() and changelog.
>
>  drivers/cpufreq/cpufreq.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e24aa5d4bca5..0f8245731783 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1284,6 +1284,12 @@ 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.
> +        */
> +       BUG_ON(!policy_is_inactive(policy));

I'm not a super-big fan of this change.

First off, crashing the kernel outright here because of possible races
appears a bit excessive to me.

Second, it looks like we are worrying about the code running before
the wait_for_completion() call in cpufreq_policy_put_kobj(), because
after that call no one can be running show() or store().  So why don't
we reorder the wait_for_completion() call with respect to the code in
question instead?

> +
>         /* Remove policy from list */
>         write_lock_irqsave(&cpufreq_driver_lock, flags);
>         list_del(&policy->policy_list);
> @@ -1538,8 +1544,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);
> @@ -1549,6 +1553,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);
> --
> 2.31.1.272.g89b43f80a514
>