Re: cpufreq_stats NULL deref on second system suspend

[Viresh Kumar]

On 12 September 2013 01:16, Srivatsa S. Bhat
<srivatsa.bhat@xxxxxxxxxxxxxxxxxx> wrote:
> From: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
> Subject: [PATCH] cpufreq: Restructure if/else block to avoid unintended behavior
>
> In __cpufreq_remove_dev_prepare(), the code which decides whether to remove
> the sysfs link or nominate a new policy cpu, is governed by an if/else block
> with a rather complex set of conditionals. Worse, they harbor a subtlety
> which leads to certain unintended behavior.
>
> The code looks like this:
>
>         if (cpu != policy->cpu && !frozen) {
>                 sysfs_remove_link(&dev->kobj, "cpufreq");
>         } else if (cpus > 1) {
>                 new_cpu = cpufreq_nominate_new_policy_cpu(...);
>                 ...
>                 update_policy_cpu(..., new_cpu);
>         }
>
> The original intention was:
> If the CPU going offline is not policy->cpu, just remove the link.
> On the other hand, if the CPU going offline is the policy->cpu itself,
> handover the policy->cpu job to some other surviving CPU in that policy.
>
> But because the 'if' condition also includes the 'frozen' check, now there
> are *two* possibilities by which we can enter the 'else' block:
>
> 1. cpu == policy->cpu (intended)
> 2. cpu != policy->cpu && frozen (unintended)
>
> Due to the second (unintended) scenario, we end up spuriously nominating
> a CPU as the policy->cpu, even when the existing policy->cpu is alive and
> well. This can cause problems further down the line, especially when we end
> up nominating the same policy->cpu as the new one (ie., old == new),
> because it totally confuses update_policy_cpu().
>
> To avoid this mess, restructure the if/else block to only do what was
> originally intended, and thus prevent any unwelcome surprises.
>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
> ---
>
>  drivers/cpufreq/cpufreq.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 62bdb95..247842b 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1193,8 +1193,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
>                 cpumask_clear_cpu(cpu, policy->cpus);
>         unlock_policy_rwsem_write(cpu);
>
> -       if (cpu != policy->cpu && !frozen) {
> -               sysfs_remove_link(&dev->kobj, "cpufreq");
> +       if (cpu != policy->cpu) {
> +               if (!frozen)
> +                       sysfs_remove_link(&dev->kobj, "cpufreq");
>         } else if (cpus > 1) {
>
>                 new_cpu = cpufreq_nominate_new_policy_cpu(policy, cpu, frozen);

Ahh, I wrote exactly the same crap.. Rafael please take Srivatsa's patch
here :)

> So can you see if patch 1 + this above fix solves your problem as well?
> Then we can retain the original patch 2 as a cleanup, after these 2 patches.

Why do we need 2 now? We should never hit that case I would say.. And If we
do, there is some other bug in our code which we have hidden :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/