linux-kernel - Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE

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]

Date:   Mon, 26 Nov 2018 09:27:02 +0100
From:   Juri Lelli <juri.lelli@...hat.com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     Sudeep Holla <sudeep.holla@....com>, rjw@...ysocki.net,
        vincent.guittot@...aro.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH 2/4] base/drivers/arch_topology: Replace mutex with
 READ_ONCE / WRITE_ONCE

Hi,

On 23/11/18 17:54, Daniel Lezcano wrote:
> On 23/11/2018 17:20, Sudeep Holla wrote:
> > On Fri, Nov 23, 2018 at 05:04:18PM +0100, Daniel Lezcano wrote:
> >> On 23/11/2018 14:58, Sudeep Holla wrote:
> >>> On Mon, Oct 29, 2018 at 05:23:18PM +0100, Daniel Lezcano wrote:
> >>>> The mutex protects a per_cpu variable access. The potential race can
> >>>> happen only when the cpufreq governor module is loaded and at the same
> >>>> time the cpu capacity is changed in the sysfs.
> >>>>
> >>>
> >>> I wonder if we really need that sysfs entry to be writable. For some
> >>> reason, I had assumed it's read only, obviously it's not. I prefer to
> >>> make it RO if there's no strong reason other than debug purposes.
> >>
> >> Are you suggesting to remove the READ_ONCE/WRITE_ONCE patch and set the
> >> sysfs file read-only ?
> >>
> > 
> > Just to be sure, if we retain RW capability we still need to fix the
> > race you are pointing out.
> > 
> > However I just don't see the need for RW cpu_capacity sysfs and hence
> > asking the reason here. IIRC I had pointed this out long back(not sure
> > internally or externally) but seemed to have missed the version that got
> > merged. So I am just asking if we really need write capability given that
> > it has known issues.
> > 
> > If user-space starts writing the value to influence the scheduler, then
> > it makes it difficult for kernel to change the way it uses the
> > cpu_capacity in future.
> > 
> > Sorry if there's valid usecase and I am just making noise here.
> 
> It's ok [added Juri Lelli]
> 
> I've been through the history:
> 
> commit be8f185d8af4dbd450023a42a48c6faa8cbcdfe6
> Author: Juri Lelli <juri.lelli@....com>
> Date:   Thu Nov 3 05:40:18 2016 +0000
> 
>     arm64: add sysfs cpu_capacity attribute
> 
>     Add a sysfs cpu_capacity attribute with which it is possible to read and
>     write (thus over-writing default values) CPUs capacity. This might be
>     useful in situations where values needs changing after boot.
> 
>     The new attribute shows up as:
> 
>      /sys/devices/system/cpu/cpu*/cpu_capacity
> 
>     Cc: Will Deacon <will.deacon@....com>
>     Cc: Mark Brown <broonie@...nel.org>
>     Cc: Sudeep Holla <sudeep.holla@....com>
>     Signed-off-by: Juri Lelli <juri.lelli@....com>
>     Signed-off-by: Catalin Marinas <catalin.marinas@....com>
> 
> Juri do you have a use case where we want to override the capacity?
> 
> Shall we switch the sysfs attribute to read-only?

So, I spent a bit of time researching patchset history and IIRC the
point of having a RW cpu_capacity was to help in situations where one
wants to change values after boot, because she/he now has "better"
numbers (remember we advocate to use Dhrystone to populate DTs, but that
is highly debatable). I also seem to remember that there might also be
cases where DT values cannot be changed at all for a (new?) platform
that happens to be using DTs shipped with an old revision; something
along these lines was mentioned (by Mark?) during the review process,
but exact details escape my mind ATM, apologies.

Best,

- Juri