linux-kernel - Re: [PATCH] opp: Invalidate current opp when draining the opp list

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: <20210308115053.ua2gfo6kfnfjslyd@vireshk-i7>
Date:   Mon, 8 Mar 2021 17:20:53 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Beata Michalska <beata.michalska@....com>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        vireshk@...nel.org, nm@...com, sboyd@...nel.org
Subject: Re: [PATCH] opp: Invalidate current opp when draining the opp list

On 05-03-21, 13:55, Beata Michalska wrote:
> Actually, that one might be problematic: by the time the
> _opp_table_kref_release is being reached, the opp pointed to
> by current_opp may no longer be valid.
> _opp_remove_all_static and/or dev_pm_opp_remove_all_dynamic
> will release all the opps by going through opp_table->opp_list.
> It will drop the reference for each opp on the list, until
> the list gets empty(for given opp type), which means,
> all the opps will actually get released
> (only upon _opp_kref_release the opp will get removed
> from the list).

Sorry for missing the context completely, I get it now.

This is what I have applied instead, please see if it breaks anything
or works as expected.

-------------------------8<-------------------------

From: Beata Michalska <beata.michalska@....com>
Date: Thu, 4 Mar 2021 15:07:34 +0000
Subject: [PATCH] opp: Invalidate current opp when draining the opp list

The current_opp when set, grabs additional reference on the opp,
which is then supposed to be dropped upon releasing the opp table.

Still both dev_pm_opp_remove_table and dev_pm_opp_remove_all_dynamic
will completely drain the OPPs list, including dropping the additional
reference on current_opp because they run until the time list gets
empty.

This will lead releasing the current_opp one more time when the OPP
table gets removed and so will raise ref counting issues.

Fix that by making sure we don't release the extra reference to the
current_opp.

Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
Signed-off-by: Beata Michalska <beata.michalska@....com>
[ Viresh: Rewrite _opp_drain_list() to not drop the extra count instead
	  of depending on reference counting. Update commit log and
	  other minor changes. ]
Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
---
 drivers/opp/core.c | 52 +++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c2689386a906..3cc0a1b82adc 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1502,10 +1502,38 @@ static struct dev_pm_opp *_opp_get_next(struct opp_table *opp_table,
 	return opp;
 }
 
-bool _opp_remove_all_static(struct opp_table *opp_table)
+/*
+ * Can't remove the OPP from under the lock, debugfs removal needs to happen
+ * lock less to avoid circular dependency issues. This must be called without
+ * the opp_table->lock held.
+ */
+static int _opp_drain_list(struct opp_table *opp_table, bool dynamic)
 {
-	struct dev_pm_opp *opp;
+	struct dev_pm_opp *opp, *current_opp = NULL;
+	int count = 0;
+
+	while ((opp = _opp_get_next(opp_table, dynamic))) {
+		if (opp_table->current_opp == opp) {
+			/*
+			 * Reached at current OPP twice, no other OPPs left. The
+			 * last reference to current_opp is dropped from
+			 * _opp_table_kref_release().
+			 */
+			if (current_opp)
+				break;
+
+			current_opp = opp;
+		}
+
+		dev_pm_opp_put(opp);
+		count++;
+	}
+
+	return count;
+}
 
+bool _opp_remove_all_static(struct opp_table *opp_table)
+{
 	mutex_lock(&opp_table->lock);
 
 	if (!opp_table->parsed_static_opps) {
@@ -1520,13 +1548,7 @@ bool _opp_remove_all_static(struct opp_table *opp_table)
 
 	mutex_unlock(&opp_table->lock);
 
-	/*
-	 * Can't remove the OPP from under the lock, debugfs removal needs to
-	 * happen lock less to avoid circular dependency issues.
-	 */
-	while ((opp = _opp_get_next(opp_table, false)))
-		dev_pm_opp_put(opp);
-
+	_opp_drain_list(opp_table, false);
 	return true;
 }
 
@@ -1539,21 +1561,13 @@ bool _opp_remove_all_static(struct opp_table *opp_table)
 void dev_pm_opp_remove_all_dynamic(struct device *dev)
 {
 	struct opp_table *opp_table;
-	struct dev_pm_opp *opp;
-	int count = 0;
+	int count;
 
 	opp_table = _find_opp_table(dev);
 	if (IS_ERR(opp_table))
 		return;
 
-	/*
-	 * Can't remove the OPP from under the lock, debugfs removal needs to
-	 * happen lock less to avoid circular dependency issues.
-	 */
-	while ((opp = _opp_get_next(opp_table, true))) {
-		dev_pm_opp_put(opp);
-		count++;
-	}
+	count = _opp_drain_list(opp_table, true);
 
 	/* Drop the references taken by dev_pm_opp_add() */
 	while (count--)