linux-kernel - Re: [PATCH] cpufreq: Fix locking issues with governors

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: <20200625111452.GA200288@google.com>
Date:   Thu, 25 Jun 2020 12:14:52 +0100
From:   Quentin Perret <qperret@...gle.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Rafael Wysocki <rjw@...ysocki.net>, linux-pm@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpufreq: Fix locking issues with governors

Hey Viresh

On Thursday 25 Jun 2020 at 16:24:16 (+0530), Viresh Kumar wrote:
> The locking around governors handling isn't adequate currently. The list
> of governors should never be traversed without locking in place. Also we
> must make sure the governor isn't removed while it is still referenced
> by code.

Thanks for having a look at this!

This solves the issue for the reference to policy->last_governor, but
given that your patch is based on top of
20200623142138.209513-3-qperret@...gle.com, 'default_governor' needs a
similar treatment I think.

Perhaps something along the lines of the (completely untested) snippet
below?

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index dad6b85f4c89..9d7cf2ce2768 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1062,6 +1062,17 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void)
 	return NULL;
 }
 
+static bool get_default_governor(void)
+{
+	bool ret;
+
+	mutex_lock(&cpufreq_governor_mutex);
+	ret = default_governor && !try_module_get(default_governor->owner);
+	mutex_unlock(&cpufreq_governor_mutex);
+
+	return ret;
+}
+
 static int cpufreq_init_policy(struct cpufreq_policy *policy)
 {
 	struct cpufreq_governor *gov = NULL;
@@ -1073,20 +1084,21 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
 		/* Update policy governor to the one used before hotplug. */
 		gov = get_governor(policy->last_governor);
 		if (gov) {
-			put_governor = true;
 			pr_debug("Restoring governor %s for cpu %d\n",
 				 policy->governor->name, policy->cpu);
-		} else if (default_governor) {
+		} else if (get_default_governor()) {
 			gov = default_governor;
 		} else {
 			return -ENODATA;
 		}
+		put_governor = true;
 	} else {
 		/* Use the default policy if there is no last_policy. */
 		if (policy->last_policy) {
 			pol = policy->last_policy;
-		} else if (default_governor) {
+		} else if (get_default_governor()) {
 			pol = cpufreq_parse_policy(default_governor->name);
+			module_put(default_governor->owner);
 			/*
 			 * In case the default governor is neiter "performance"
 			 * nor "powersave", fall back to the initial policy