[PATCH] opp: Invalidate current opp when draining the opp list

[PATCH] opp: Invalidate current opp when draining the opp list

[Beata Michalska]

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. This may lead to an attempt to access
memory that has already been released. Make sure that while draining
the list (in both dynamic and static cases) the current_opp gets
actually invalidated.

Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")

Signed-off-by: Beata Michalska <beata.michalska@arm.com>
---
 drivers/opp/core.c | 49 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c268938..10e65c4 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1502,10 +1502,39 @@ 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)
+static int __opp_drain_list(struct opp_table *opp_table, bool dynamic)
 {
 	struct dev_pm_opp *opp;
+	int count = 0;
+
+	/*
+	 * 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, dynamic))) {
+		/*
+		 * The current_opp has extra hold on the ref count,
+		 * still the draining here will result in all of them
+		 * being dropped completely, so make
+		 * sure no one will try to access the current_opp
+		 * afterwords
+		 */
+		if (opp_table->current_opp == opp &&
+		    !(kref_read(&opp->kref) - 1))
+			opp_table->current_opp = NULL;
+
+		dev_pm_opp_put(opp);
+		/*
+		 * Note: the count here will reflect number of references
+		 * dropped, not the number of opps in the list
+		 */
+		++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 +1549,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 +1562,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;
 
 	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--)
-- 
2.7.4

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

[Lukasz Luba]

Hi Beata,

On 3/4/21 3:07 PM, Beata Michalska wrote:
> 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. This may lead to an attempt to access
> memory that has already been released. Make sure that while draining
> the list (in both dynamic and static cases) the current_opp gets
> actually invalidated.
> 
> Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
> 
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> ---
>   drivers/opp/core.c | 49 ++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 32 insertions(+), 17 deletions(-)

The change looks good.

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Regards,
Lukasz

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

[Viresh Kumar]

On 04-03-21, 15:07, Beata Michalska wrote:
> 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. This may lead to an attempt to access
> memory that has already been released. Make sure that while draining
> the list (in both dynamic and static cases) the current_opp gets
> actually invalidated.
> 
> Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
> 
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> ---
>  drivers/opp/core.c | 49 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index c268938..10e65c4 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1502,10 +1502,39 @@ 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)
> +static int __opp_drain_list(struct opp_table *opp_table, bool dynamic)
>  {
>  	struct dev_pm_opp *opp;
> +	int count = 0;
> +
> +	/*
> +	 * 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, dynamic))) {
> +		/*
> +		 * The current_opp has extra hold on the ref count,
> +		 * still the draining here will result in all of them
> +		 * being dropped completely, so make
> +		 * sure no one will try to access the current_opp
> +		 * afterwords
> +		 */
> +		if (opp_table->current_opp == opp &&
> +		    !(kref_read(&opp->kref) - 1))
> +			opp_table->current_opp = NULL;

Did you miss looking at:

static void _opp_table_kref_release(struct kref *kref)
{
        ...

	if (opp_table->current_opp)
		dev_pm_opp_put(opp_table->current_opp);

        ...
}

?

This is the place where the last reference to the current_opp is released and so
we shouldn't have any invalid access to it anywhere else.

Or am I missing some context here ?

-- 
viresh

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

[Beata Michalska]

On 3/5/21 4:24 AM, Viresh Kumar wrote:
> On 04-03-21, 15:07, Beata Michalska wrote:
>> 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. This may lead to an attempt to access
>> memory that has already been released. Make sure that while draining
>> the list (in both dynamic and static cases) the current_opp gets
>> actually invalidated.
>>
>> Fixes: 81c4d8a3c414 ("opp: Keep track of currently programmed OPP")
>>
>> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
>> ---
>>  drivers/opp/core.c | 49 ++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 32 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index c268938..10e65c4 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1502,10 +1502,39 @@ 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)
>> +static int __opp_drain_list(struct opp_table *opp_table, bool dynamic)
>>  {
>>      struct dev_pm_opp *opp;
>> +    int count = 0;
>> +
>> +    /*
>> +     * 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, dynamic))) {
>> +            /*
>> +             * The current_opp has extra hold on the ref count,
>> +             * still the draining here will result in all of them
>> +             * being dropped completely, so make
>> +             * sure no one will try to access the current_opp
>> +             * afterwords
>> +             */
>> +            if (opp_table->current_opp == opp &&
>> +                !(kref_read(&opp->kref) - 1))
>> +                    opp_table->current_opp = NULL;
>
> Did you miss looking at:
>
> static void _opp_table_kref_release(struct kref *kref)
> {
>         ...
>
>       if (opp_table->current_opp)
>               dev_pm_opp_put(opp_table->current_opp);
>
>         ...
> }
>
> ?
>
> This is the place where the last reference to the current_opp is released and so
> we shouldn't have any invalid access to it anywhere else.
>
> Or am I missing some context here ?
>

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).

so assuming simplified case where current_opp is the only
opp on the opp_list:

-> dev_pm_opp_add :  kref : 1
-> set current_opp : kref : 2
...
-> dev_pm_opp_remove_table:
  -> _opp_remove_all_static:
       /*
        * Here the dev_pm_opp_put will be called
        * as many times as the current object's kref
        * count (2)
        * as only then the object will be removed
        * from the list:
        */
       wile ((opp = _opp_get_next(opp_table, false)))
               dev_pm_opp_put(opp);
       ...
  -> dev_pm_opp_put_opp_table
    -> _opp_table_kref_release:
         /*
          * Here the opp_table->current_opp points to object
          * that has been released in _opp_remove_all_static
          * (or dev_pm_opp_remove_all_dynamic )
          * the opp_list might get emptied by that time
          */


Logging the ref counter for current_opp:

[  311.203910] core: _opp_remove_all_static: current opp  [2]
[  311.203943] core: _opp_remove_all_static: current opp  [1]
[  311.218904] core: _opp_table_kref_release: current opp: [0]


The other question is if that was the intention instead of
going through that list once, though
(so instead of list_for_each_entry using
list_for_each_entry_continue i.e.)


Hope I didn't miss anything on the way.

-----
BR
B.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

[Viresh Kumar]

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@arm.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@arm.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@linaro.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--)

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

[Beata Michalska]

On Mon, Mar 08, 2021 at 05:20:53PM +0530, Viresh Kumar wrote:
> 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@arm.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@arm.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@linaro.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;
> +		}
Having a quick look at the code ...
Shouldn't the current_opp be moved at the end of the list ?
Otherwise there is a risk of leaving unreferenced opps (and opp_table).
Might be also worth adding warning (?)

    WARN_ONCE(!list_is_singular())


---
BR
B.
> +
> +		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--)

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

[Viresh Kumar]

On 08-03-21, 18:14, Beata Michalska wrote:
> > -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;
> > +		}
> Having a quick look at the code ...
> Shouldn't the current_opp be moved at the end of the list ?
> Otherwise there is a risk of leaving unreferenced opps (and opp_table).

How exactly ? Note that it is expected that the OPP table isn't being
used by anyone anymore at this point and all the users went away.

> Might be also worth adding warning (?)
> 
>     WARN_ONCE(!list_is_singular())

It is allowed for the list to contain both static and dynamic OPPs,
and so the list may have more OPPs here.

-- 
viresh

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

[Beata Michalska]

On Tue, Mar 09, 2021 at 10:01:21AM +0530, Viresh Kumar wrote:
> On 08-03-21, 18:14, Beata Michalska wrote:
> > > -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;
> > > +		}
> > Having a quick look at the code ...
> > Shouldn't the current_opp be moved at the end of the list ?
> > Otherwise there is a risk of leaving unreferenced opps (and opp_table).
> 
> How exactly ? Note that it is expected that the OPP table isn't being
> used by anyone anymore at this point and all the users went away.
>
With the current version, once the _opp_get_next returns opp
that is the current_opp, the while loop will break, leaving all
the opps that are on the list after current_opp: _opp_get_next
is not actually getting the *next* entry from the list. If it
reaches the current_opp, the _next_ one will still be the same opp
as it will not be removed from the list at this point
( _opp_get_next is a simple for_each_entry and until the first entry
gets removed from the list it will always return the same one).
If the opp_table->current_opp gets pushed at the end of the list,
it should be safe to assume that if it gets reached for the second time,
it's the last one on the list).

The dev_pm_opp_remove_all_dynamic depends on the number of dynamic
opps removed from the list to drop the according number of references
on the opp_table. If the count does not correspond to number of calls
to dev_pm_opp_add, the final refcount on opp_table might not drop to
the point when the table will actually get released. And if the while
loop above drops earlier - this might be the case.

Also, I am not sure how probable is the case, but if the current_opp
has more references that the initial one and one from current_opp,
it might not get released properly either.

Or am I missing smth ?

> > Might be also worth adding warning (?)
> > 
> >     WARN_ONCE(!list_is_singular())
> 
> It is allowed for the list to contain both static and dynamic OPPs,
> and so the list may have more OPPs here.
> 

True, jumped to the idea too early and the _opp_table_kref_release
already warns on list not being empty.

---
BR.
B.
> -- 
> viresh

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

[Viresh Kumar]

On 09-03-21, 12:14, Beata Michalska wrote:
> With the current version, once the _opp_get_next returns opp
> that is the current_opp, the while loop will break, leaving all
> the opps that are on the list after current_opp

Thanks for your excellent review Beata. I have been missing a lot
lately :(

Though now I have spent more time on it and the bug seems to be more
severe. This is what I have applied now :)

I was able to test that the OPP table gets freed without any reference
counts issues now.

-- 
viresh

-------------------------8<-------------------------
From: Beata Michalska <beata.michalska@arm.com>
Date: Thu, 4 Mar 2021 15:07:34 +0000
Subject: [PATCH] opp: Don't drop extra references to OPPs accidentally

We are required to call dev_pm_opp_put() from outside of the
opp_table->lock as debugfs removal needs to happen lock-less to avoid
circular dependency issues.

commit cf1fac943c63 ("opp: Reduce the size of critical section in
_opp_kref_release()") tried to fix that introducing a new routine
_opp_get_next() which keeps returning OPPs that can be freed by the
callers and this routine shall be called without holding the
opp_table->lock.

Though the commit overlooked the fact that the OPPs can be referenced by
other users as well and this routine will end up dropping references
which were taken by other users and hence freeing the OPPs prematurely.

In effect, other users of the OPPs will end up having invalid pointers
at hand. We didn't see any crash reports earlier as the exact situation
never happened, though it is certainly possible.

We need a way to mark which OPPs are no longer referenced by the OPP
core, so we don't drop extra references to them accidentally.

This commit adds another OPP flag, "removed", which is used to track
this. And now we should never end up dropping extra references to the
OPPs.

Cc: v5.11+ <stable@vger.kernel.org> # v5.11+
Fixes: cf1fac943c63 ("opp: Reduce the size of critical section in _opp_kref_release()")
Signed-off-by: Beata Michalska <beata.michalska@arm.com>
[ Viresh: Almost rewrote entire patch, added new "removed" field,
	  rewrote commit log and added the correct Fixes tag. ]
Co-developed-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 48 ++++++++++++++++++++++++----------------------
 drivers/opp/opp.h  |  2 ++
 2 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index c2689386a906..150be4c28c99 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1492,7 +1492,11 @@ static struct dev_pm_opp *_opp_get_next(struct opp_table *opp_table,
 
 	mutex_lock(&opp_table->lock);
 	list_for_each_entry(temp, &opp_table->opp_list, node) {
-		if (dynamic == temp->dynamic) {
+		/*
+		 * Refcount must be dropped only once for each OPP by OPP core,
+		 * do that with help of "removed" flag.
+		 */
+		if (!temp->removed && dynamic == temp->dynamic) {
 			opp = temp;
 			break;
 		}
@@ -1502,10 +1506,27 @@ 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 call dev_pm_opp_put() from under the lock as debugfs removal needs to
+ * happen lock less to avoid circular dependency issues. This routine must be
+ * called without the opp_table->lock held.
+ */
+static void _opp_drain_list(struct opp_table *opp_table, bool dynamic)
 {
 	struct dev_pm_opp *opp;
 
+	while ((opp = _opp_get_next(opp_table, dynamic))) {
+		opp->removed = true;
+		dev_pm_opp_put(opp);
+
+		/* Drop the references taken by dev_pm_opp_add() */
+		if (dynamic)
+			dev_pm_opp_put_opp_table(opp_table);
+	}
+}
+
+bool _opp_remove_all_static(struct opp_table *opp_table)
+{
 	mutex_lock(&opp_table->lock);
 
 	if (!opp_table->parsed_static_opps) {
@@ -1520,13 +1541,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,25 +1554,12 @@ 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;
 
 	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++;
-	}
-
-	/* Drop the references taken by dev_pm_opp_add() */
-	while (count--)
-		dev_pm_opp_put_opp_table(opp_table);
+	_opp_drain_list(opp_table, true);
 
 	/* Drop the reference taken by _find_opp_table() */
 	dev_pm_opp_put_opp_table(opp_table);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 50fb9dced3c5..407c3bfe51d9 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -56,6 +56,7 @@ extern struct list_head opp_tables, lazy_opp_tables;
  * @dynamic:	not-created from static DT entries.
  * @turbo:	true if turbo (boost) OPP
  * @suspend:	true if suspend OPP
+ * @removed:	flag indicating that OPP's reference is dropped by OPP core.
  * @pstate: Device's power domain's performance state.
  * @rate:	Frequency in hertz
  * @level:	Performance level
@@ -78,6 +79,7 @@ struct dev_pm_opp {
 	bool dynamic;
 	bool turbo;
 	bool suspend;
+	bool removed;
 	unsigned int pstate;
 	unsigned long rate;
 	unsigned int level;

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

[Beata Michalska]

On Wed, Mar 10, 2021 at 02:17:38PM +0530, Viresh Kumar wrote:
> On 09-03-21, 12:14, Beata Michalska wrote:
> > With the current version, once the _opp_get_next returns opp
> > that is the current_opp, the while loop will break, leaving all
> > the opps that are on the list after current_opp
> 
> Thanks for your excellent review Beata. I have been missing a lot
> lately :(
> 
> Though now I have spent more time on it and the bug seems to be more
> severe. This is what I have applied now :)
> 
> I was able to test that the OPP table gets freed without any reference
> counts issues now.
> 
> -- 
> viresh
> 
> -------------------------8<-------------------------
> From: Beata Michalska <beata.michalska@arm.com>
> Date: Thu, 4 Mar 2021 15:07:34 +0000
> Subject: [PATCH] opp: Don't drop extra references to OPPs accidentally
> 
> We are required to call dev_pm_opp_put() from outside of the
> opp_table->lock as debugfs removal needs to happen lock-less to avoid
> circular dependency issues.
> 
> commit cf1fac943c63 ("opp: Reduce the size of critical section in
> _opp_kref_release()") tried to fix that introducing a new routine
> _opp_get_next() which keeps returning OPPs that can be freed by the
> callers and this routine shall be called without holding the
> opp_table->lock.
> 
> Though the commit overlooked the fact that the OPPs can be referenced by
> other users as well and this routine will end up dropping references
> which were taken by other users and hence freeing the OPPs prematurely.
> 
> In effect, other users of the OPPs will end up having invalid pointers
> at hand. We didn't see any crash reports earlier as the exact situation
> never happened, though it is certainly possible.
> 
> We need a way to mark which OPPs are no longer referenced by the OPP
> core, so we don't drop extra references to them accidentally.
> 
> This commit adds another OPP flag, "removed", which is used to track
> this. And now we should never end up dropping extra references to the
> OPPs.
> 
> Cc: v5.11+ <stable@vger.kernel.org> # v5.11+
> Fixes: cf1fac943c63 ("opp: Reduce the size of critical section in _opp_kref_release()")
> Signed-off-by: Beata Michalska <beata.michalska@arm.com>
> [ Viresh: Almost rewrote entire patch, added new "removed" field,
> 	  rewrote commit log and added the correct Fixes tag. ]
> Co-developed-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/opp/core.c | 48 ++++++++++++++++++++++++----------------------
>  drivers/opp/opp.h  |  2 ++
>  2 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index c2689386a906..150be4c28c99 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1492,7 +1492,11 @@ static struct dev_pm_opp *_opp_get_next(struct opp_table *opp_table,
>  
>  	mutex_lock(&opp_table->lock);
>  	list_for_each_entry(temp, &opp_table->opp_list, node) {
> -		if (dynamic == temp->dynamic) {
> +		/*
> +		 * Refcount must be dropped only once for each OPP by OPP core,
> +		 * do that with help of "removed" flag.
> +		 */
> +		if (!temp->removed && dynamic == temp->dynamic) {
>  			opp = temp;
>  			break;
>  		}
How about tweaking the _opp_get_next to use list_for_each_entry_continue
instead ? It would eliminate the need of tracking the 'removed' status
and could save few cycles as it wouldn't have to go through the list
starting from it's beginning each time it is being called.
Happy to draft another version.

Otherwise it looks good.

> @@ -1502,10 +1506,27 @@ 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 call dev_pm_opp_put() from under the lock as debugfs removal needs to
> + * happen lock less to avoid circular dependency issues. This routine must be
> + * called without the opp_table->lock held.
> + */
> +static void _opp_drain_list(struct opp_table *opp_table, bool dynamic)
>  {
>  	struct dev_pm_opp *opp;
>  
> +	while ((opp = _opp_get_next(opp_table, dynamic))) {
> +		opp->removed = true;
> +		dev_pm_opp_put(opp);
> +
> +		/* Drop the references taken by dev_pm_opp_add() */
> +		if (dynamic)
> +			dev_pm_opp_put_opp_table(opp_table);
> +	}
> +}
> +
Neat: The 'drain' in the function name is not entirely describing
what the function actually does anymore.

---
BR
B.
> +bool _opp_remove_all_static(struct opp_table *opp_table)
> +{
>  	mutex_lock(&opp_table->lock);
>  
>  	if (!opp_table->parsed_static_opps) {
> @@ -1520,13 +1541,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,25 +1554,12 @@ 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;
>  
>  	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++;
> -	}
> -
> -	/* Drop the references taken by dev_pm_opp_add() */
> -	while (count--)
> -		dev_pm_opp_put_opp_table(opp_table);
> +	_opp_drain_list(opp_table, true);
>  
>  	/* Drop the reference taken by _find_opp_table() */
>  	dev_pm_opp_put_opp_table(opp_table);
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index 50fb9dced3c5..407c3bfe51d9 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -56,6 +56,7 @@ extern struct list_head opp_tables, lazy_opp_tables;
>   * @dynamic:	not-created from static DT entries.
>   * @turbo:	true if turbo (boost) OPP
>   * @suspend:	true if suspend OPP
> + * @removed:	flag indicating that OPP's reference is dropped by OPP core.
>   * @pstate: Device's power domain's performance state.
>   * @rate:	Frequency in hertz
>   * @level:	Performance level
> @@ -78,6 +79,7 @@ struct dev_pm_opp {
>  	bool dynamic;
>  	bool turbo;
>  	bool suspend;
> +	bool removed;
>  	unsigned int pstate;
>  	unsigned long rate;
>  	unsigned int level;

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

[Viresh Kumar]

On 10-03-21, 23:03, Beata Michalska wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index c2689386a906..150be4c28c99 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1492,7 +1492,11 @@ static struct dev_pm_opp *_opp_get_next(struct opp_table *opp_table,
> >  
> >  	mutex_lock(&opp_table->lock);
> >  	list_for_each_entry(temp, &opp_table->opp_list, node) {
> > -		if (dynamic == temp->dynamic) {
> > +		/*
> > +		 * Refcount must be dropped only once for each OPP by OPP core,
> > +		 * do that with help of "removed" flag.
> > +		 */
> > +		if (!temp->removed && dynamic == temp->dynamic) {
> >  			opp = temp;
> >  			break;
> >  		}
> How about tweaking the _opp_get_next to use list_for_each_entry_continue
> instead ? It would eliminate the need of tracking the 'removed' status
> and could save few cycles as it wouldn't have to go through the list
> starting from it's beginning each time it is being called.
> Happy to draft another version.

I tried that as well, but the problem is that we need to drop locks in
between and if the next OPP somehow gets freed or another one gets
added there, we can be in trouble. To make this work without any side
effects, the new field was kind of required.

-- 
viresh