linux-kernel - Re: [PATCH] opp: Drop unnecessary check frmo dev_pm_opp_attach

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] [day] [month] [year] [list]

Message-ID: <20200831055107.fuyqszsir5ibue2q@vireshk-i7>
Date:   Mon, 31 Aug 2020 11:21:07 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Stephan Gerhold <stephan@...hold.net>
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: Drop unnecessary check frmo
 dev_pm_opp_attach_genpd()

On 27-08-20, 14:14, Stephan Gerhold wrote:
> Only partially related to this patch, but actually I noticed that
> dev_pm_opp_attach_genpd() does not work correctly if it is called
> multiple times.
> 
> For example, qcom-cpufreq-nvmem calls this for every CPU because it is
> not aware that the OPP table is shared between the CPUs.

It could have called it from cpufreq_driver->init, but yeah I see the
problem here.

> dev_pm_opp_attach_genpd() does not check if opp_table->genpd_virt_devs
> is already set, so when it is called again for other CPUs we will:
> 
>   - Cause a memory leak (opp_table->genpd_virt_devs is just replaced
>     with new memory)
>   - Attach the power domains multiple times
>   - Never detach the power domains from earlier calls
>   - Crash when dev_pm_opp_detach_genpd() is called the second time
> 
> Oh well. :)
> 
> I think the function should just return and do nothing if the power
> domains were already attached, just like dev_pm_opp_set_supported_hw()
> etc. But this is a bit complicated to implement with the "virt_devs"
> parameter, since callers will probably assume that to be valid if we
> return success.

Or maybe at least make it work by returning the OPP table and not
setting the virt_devs.

> Another advantage of my proposal to remove the virt_devs parameter [1] :)

Yes, I do see the advantage there, lets see where that discussion
goes.

-- 
viresh