Re: [RFC] memcg v1: provide read access to memory.pressure_level

Florian Schmidt
2023-03-22T16:01:13Z

On 22/03/2023 15:57, Michal Hocko wrote:
> On Wed 22-03-23 14:25:25, Florian Schmidt wrote:
>> cgroups v1 has a unique way of setting up memory pressure notifications:
>> the user opens "memory.pressure_level" of the cgroup they want to
>> monitor for pressure, then open "cgroup.event_control" and write the fd
>> (among other things) to that file. memory.pressure_level has no other
>> use, specifically it does not support any read or write operations.
>> Consequently, no handlers are provided, and the file ends up with
>> permissions 000. However, to actually use the mechanism, the subscribing
>> user must have read access to the file and open the fd for reading, see
>> memcg_write_event_control().
>>
>> This is all fine as long as the subscribing process runs as root and is
>> otherwise unconfined by further restrictions. However, if you add strict
>> access controls such as selinux, the permission bits will be enforced,
>> and opening memory.pressure_level for reading will fail, preventing the
>> process from subscribing, even as root.
>>
>>
>> There are several ways around this issue, but adding a dummy read
>> handler seems like the least invasive to me.
>
> I was struggling to see how that addresses the problem because all you
> need is a read permission. But then I've looked into cgroup code and
> learned that permissions are constructed based on available callbacks
> (cgroup_file_mode). This would have made the review easier ;)

Oh, sorry, I forgot to mention that salient detail!
I didn't check whether that was a common pattern or not...

>
> I have no issue with the patch. It would be great to hear from cgroup
> maintainers whether a concept of default permissions is something that
> would be useful also for other files.
>
>> 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.
>> (b) would you be interested to take this patch, or is it too niche a fix
>> for a legacy subsystem?
>
> After you add your s-o-b, feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>
>
> If cgroup people find a concept of default permissions for a cgroup file
> sound then this could be replaced by that approach but this is really an
> easy workaround.

Will do, once I know the path forward and construct a proper commit
message, I'll add the s-o-b and ack.

>> ---
>> mm/memcontrol.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 5abffe6f8389..e48c749d9724 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3734,6 +3734,16 @@ static u64 mem_cgroup_read_u64(struct cgroup_subsys_state *css,
>> }
>> }
>>
>> +/*
>> + * This function doesn't do anything useful. Its only job is to provide a read
>> + * handler so that the file gets read permissions when it's created.
>
> I would just reference cgroup_file_mode() in the comment to make our
> lifes easier and comment more helpful.

Ack.

>
>> + */
>> +static int mem_cgroup_dummy_seq_show(__always_unused struct seq_file *m,
>> + __always_unused void *v)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> #ifdef CONFIG_MEMCG_KMEM
>> static int memcg_online_kmem(struct mem_cgroup *memcg)
>> {
>> @@ -5064,6 +5074,7 @@ static struct cftype mem_cgroup_legacy_files[] = {
>> },
>> {
>> .name = "pressure_level",
>> + .seq_show = mem_cgroup_dummy_seq_show,
>> },
>> #ifdef CONFIG_NUMA
>> {
>> --
>> 2.32.0
>