linux-kernel - [RFC][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-next>] [day] [month] [year] [list]

Message-ID: <2362640.pUofnXyzOi@vostro.rjw.lan>
Date:	Thu, 01 Aug 2013 02:08:15 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Viresh Kumar <viresh.kumar@...aro.org>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
Cc:	Linux PM list <linux-pm@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>, cpufreq@...r.kernel.org
Subject: [RFC][PATCH] cpufreq: Do not hold driver module references for additional policy CPUs

From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>

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 without siblings
or generally a CPU for which a new policy object has to be created,
it grabs a reference to the driver module to start with, but drops
that reference before returning.  As a result, the driver module
refcount is then equal to 0 after __cpufreq_add_dev() has returned.

On the other hand, if the given CPU is a sibling of some other
CPU already having a policy, cpufreq_add_policy_cpu() is called
to link the new CPU to the existing policy.  In that case,
cpufreq_cpu_get() is called to obtain that policy and grabs a
reference to the driver module, but that reference is not
released and the module refcount will be different from 0 after
__cpufreq_add_dev() returns (unless there is an error).  That
prevents the driver module from being unloaded until
__cpufreq_remove_dev() is called for all the CPUs that
cpufreq_add_policy_cpu() was called for previously.

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, but also make it take a reference to
the policy itself using kobject_get() and do not release that
reference (unless there's an error or system resume is under way),
which again is consistent with the "raw" __cpufreq_add_dev()
behavior.

Accordingly, modify __cpufreq_remove_dev() to use kobject_put() to
drop policy references taken by cpufreq_add_policy_cpu().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
---

On top of current linux-pm.git/linux-next.

Thanks,
Rafael

---
 drivers/cpufreq/cpufreq.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -908,8 +908,10 @@ static int cpufreq_add_policy_cpu(unsign
 	unsigned long flags;

 	policy = cpufreq_cpu_get(sibling);
-	WARN_ON(!policy);
+	if (WARN_ON_ONCE(!policy))
+		return -ENODATA;

+	kobject_get(&policy->kobj);
 	if (has_target)
 		__cpufreq_governor(policy, CPUFREQ_GOV_STOP);

@@ -932,14 +934,14 @@ 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;
+		kobject_put(&policy->kobj);
+	} else {
+		ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+		if (ret)
+			kobject_put(&policy->kobj);
 	}

-	ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
-	if (ret)
-		cpufreq_cpu_put(policy);
-
+	cpufreq_cpu_put(policy);
 	return ret;
 }
 #endif
@@ -1298,10 +1300,14 @@ static int __cpufreq_remove_dev(struct d
 		if (!frozen)
 			cpufreq_policy_free(data);
 	} else {
-
+		/*
+		 * There are more CPUs using the same policy, so only drop the
+		 * reference taken by cpufreq_add_policy_cpu() (unless the
+		 * system is suspending).
+		 */
 		if (!frozen) {
 			pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
-			cpufreq_cpu_put(data);
+			kobject_put(&data->kobj);
 		}

 		if (cpufreq_driver->target) {

--
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/