linux-kernel - Re: [Update][PATCH] cpufreq: Do not hold driver module references for additional policy CPUs

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: <CAKohponru8Z1fod43dVtAODmYuMTGX70+73b8wk4wuGN=Vuh-w@mail.gmail.com>
Date:	Fri, 2 Aug 2013 10:07:23 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	Linux PM list <linux-pm@...r.kernel.org>, cpufreq@...r.kernel.org,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Lists linaro-kernel <linaro-kernel@...ts.linaro.org>
Subject: Re: [Update][PATCH] cpufreq: Do not hold driver module references for
 additional policy CPUs

Wow!! Lot of stuff happened while I was asleep..

@Srivatsa: Thanks for answering what I would have answered to Rafael :)
And you should really get some sleep, I would suggest :)

On 2 August 2013 02:23, Rafael J. Wysocki <rjw@...k.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> Subject: cpufreq: Do not hold driver module references for additional policy CPUs

I still have issues with this subject. Why don't we get rid of .owner
field completely? And stop using a mix of cpufreq_cpu_get() and
kobject_get()?

> The cpufreq core is a little inconsistent in the way it uses the
> driver module refcount.
>
> Namely, if __cpufreq_add_dev() is called for a CPU that doesn't
> share the policy object with any other CPUs, the driver module
> refcount it grabs to start with will be dropped by it before
> returning and will be equal to 0 afterward.

It wouldn't be zero but 1, this is what it is initialized with probably.
That's what I can see in my tests.

> However, if the given CPU does share the policy object with other
> CPUs, either cpufreq_add_policy_cpu() is called to link the new CPU
> to the existing policy, or cpufreq_add_dev_symlink() is used to link
> the other CPUs sharing the policy with it to the just created policy
> object.  In that case, because both cpufreq_add_policy_cpu() and
> cpufreq_add_dev_symlink() call cpufreq_cpu_get() for the given
> policy (the latter possibly many times) without the balancing
> cpufreq_cpu_put() (unless there is an error), the driver module
> refcount will be left by __cpufreq_add_dev() with a nonzero value.
>
> To remove that inconsistency make cpufreq_add_policy_cpu() execute
> cpufreq_cpu_put() for the given policy before returning, which
> decrements the driver module refcount so that it will be 0 after
> __cpufreq_add_dev() returns.  Moreover, remove the cpufreq_cpu_get()
> call from cpufreq_add_dev_symlink(), since both the policy refcount
> and the driver module refcount are nonzero when it is called and they
> don't need to be bumped up by it.
>
> Accordingly, drop the cpufreq_cpu_put() from __cpufreq_remove_dev(),
> since it is only necessary to balance the cpufreq_cpu_get() called
> by cpufreq_add_policy_cpu() or cpufreq_add_dev_symlink().
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@...ux.vnet.ibm.com>
> ---
>  drivers/cpufreq/cpufreq.c |   28 +++++++---------------------
>  1 file changed, 7 insertions(+), 21 deletions(-)

So, we can't rmmod the module as soon as it is inserted and so the
problem stays as is. :(

> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -818,14 +818,11 @@ static int cpufreq_add_dev_symlink(struc
>                         continue;
>
>                 pr_debug("Adding link for CPU: %u\n", j);
> -               cpufreq_cpu_get(policy->cpu);
>                 cpu_dev = get_cpu_device(j);
>                 ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
>                                         "cpufreq");
> -               if (ret) {
> -                       cpufreq_cpu_put(policy);
> -                       return ret;
> -               }
> +               if (ret)
> +                       break;
>         }
>         return ret;
>  }
> @@ -908,7 +905,8 @@ static int cpufreq_add_policy_cpu(unsign
>         unsigned long flags;
>
>         policy = cpufreq_cpu_get(sibling);

This can be skipped completely at this place. Caller of
cpufreq_add_policy_cpu() has got the policy pointer with it and so
can be passed. I haven't done it earlier as the impression was we need
to call cpufreq_cpu_get()..

> -       WARN_ON(!policy);
> +       if (WARN_ON_ONCE(!policy))
> +               return -ENODATA;
>
>         if (has_target)
>                 __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> @@ -930,16 +928,10 @@ static int cpufreq_add_policy_cpu(unsign
>         }
>
>         /* Don't touch sysfs links during light-weight init */
> -       if (frozen) {
> -               /* Drop the extra refcount that we took above */
> -               cpufreq_cpu_put(policy);
> -               return 0;
> -       }
> -
> -       ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
> -       if (ret)
> -               cpufreq_cpu_put(policy);
> +       if (!frozen)
> +               ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
>
> +       cpufreq_cpu_put(policy);

And so this will go away.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/