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@xxxxxxx>
>> ---
>>  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.