linux-kernel - Re: [RFC] memcg v1: provide read access to memory.pressure

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: <6f4420d0-2f86-7294-01e1-6806ef7e4686@nutanix.com>
Date:   Mon, 27 Mar 2023 14:59:37 +0100
From:   Florian Schmidt <flosch@...anix.com>
To:     Michal Koutný <mkoutny@...e.com>
Cc:     Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Shakeel Butt <shakeelb@...gle.com>,
        Muchun Song <muchun.song@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        cgroups@...r.kernel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC] memcg v1: provide read access to memory.pressure_level

Hi Michal,

On 24/03/2023 15:03, Michal Koutný wrote:
> On Wed, Mar 22, 2023 at 02:25:25PM +0000, Florian Schmidt <flosch@...anix.com> wrote:
>> cgroups v1 has a unique way of setting up memory pressure notifications:
> ...
>> There are several ways around this issue, but adding a dummy read
>> handler seems like the least invasive to me. I'd be interested to hear:
>> (a) do you think there is a less invasive way? Alternatively, we could
>>      add a flag in cftype in include/linux/cgroup-defs.h, but that seems
>>      more invasive for what is a legacy interface.
> 
> You can (as privileged user) modify file perms in userspace first (e.g.
> chmod o+r memory.pressure_level) and then it can used by non-privileged
> users. (Or do LSM prevent you from that too?)

That's true, we can work around this in userspace (though it means you 
need to give the process additional permissions, to change file 
permissions on top of just reading and writing).

Though considering that the memcg_write_event_control() explicitly 
checks whether the caller has read permissions on pressure_level, it 
felt sensible to me that the file would be created with read permissions 
in the first place, just like all the other files are created with 
permissions that are suitable for their immediate use without having to 
manually change permissions. The current implementation feels 
inconsistent in that way.

>> (b) would you be interested to take this patch, or is it too niche a fix
>>      for a legacy subsystem?
> 
> I'd rather not extend this "unique way" with additionally unique dummy
> helpers.

I understand that this is all code that has no modern user any more, 
which is why I tried to keep the fix as self-contained as possible.
Another option would be to have a special handler in cgroup_file_mode(), 
but that feels a lot klunkier to me, and leaks a v1-specific behaviour 
into the shared cgroup code.

Cheers,
Florian