linux-kernel - Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators

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: <94f4ed8d-ab54-4180-87f7-4ddafb52074f@gmail.com>
Date:   Wed, 20 Jan 2021 17:50:15 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>, linux-pm@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Rafael Wysocki <rjw@...ysocki.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] opp: Prepare for ->set_opp() helper to work without
 regulators

20.01.2021 10:39, Viresh Kumar пишет:
> On 19-01-21, 20:16, Dmitry Osipenko wrote:
>> 19.01.2021 09:35, Viresh Kumar пишет:
>>> +	mutex_lock(&opp_table->lock);
>>> +	opp_table->set_opp_data = data;
>>> +	if (opp_table->sod_supplies) {
>>> +		data->old_opp.supplies = opp_table->sod_supplies;
>>> +		data->new_opp.supplies = opp_table->sod_supplies +
>>> +					 opp_table->regulator_count;
>>> +	}
>>> +	mutex_unlock(&opp_table->lock);
>>
>> Why do we need all these locks in this patch?
> 
> In case dev_pm_opp_set_regulators() and
> dev_pm_opp_register_set_opp_helper() get called at the same time.
> Which can actually happen, though is a corner case.
> 
>> The OPP API isn't thread-safe, these locks won't make the API
>> thread-safe.
> 
> I am not sure what you mean by that, can you please explain ?
> 
>> At least both sod_supplies and set_opp() pointers should be
>> set and unset under the lock.
> 
> The ->set_opp pointer isn't getting used for a comparison and so
> putting that inside a lock won't get us anything. We are only using
> set_opp_data and sod_supplies for comparison at both the places and so
> they need to be updated within the lock.
> 
If OPP API was meant to be thread-safe, then the
dev_pm_opp_unregister_set_opp_helper() should unset the
opp_table->set_opp_data under the lock since it races with
dev_pm_opp_set_regulators().

Secondly, functions like dev_pm_opp_set_rate() don't have any locks at all.

It should be better not to add "random" locks into the code because it
only creates an illusion for an oblivious API user that OPP API cares
about thread safety, IMO.

Making OPP API thread-safe will take some effort and a careful review of
every lock will be needed.