Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <916c1255-26b0-05e8-96af-e5062512cb97@nutanix.com>
Date:   Wed, 22 Mar 2023 16:00:52 +0000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.9.0
Subject: Re: [RFC] memcg v1: provide read access to memory.pressure_level
Content-Language: en-US
To:     Michal Hocko <mhocko@suse.com>
Cc:     Johannes Weiner <hannes@cmpxchg.org>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Shakeel Butt <shakeelb@google.com>,
        Muchun Song <muchun.song@linux.dev>,
        Andrew Morton <akpm@linux-foundation.org>,
        cgroups@vger.kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20230322142525.162469-1-flosch@nutanix.com>
 <ZBslZrtFISfYqV5l@dhcp22.suse.cz>
From:   Florian Schmidt <flosch@nutanix.com>
In-Reply-To: <ZBslZrtFISfYqV5l@dhcp22.suse.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Q0tNbXhLRFlaZ0htNUx3VFRqMGhFdUFSL1FCcEZqN21TZ3lSWGdid2tCcnFB?=
 =?utf-8?B?UXYvMzRuY0lmMm1YbkI2NnJkK1ZLYlZJN2h2aWhsMXk5Y1d6ellUZXBPcko1?=
 =?utf-8?B?b0VMalJWUmhDQVR5V29DZWNWQ3UrUVMrejVGRWZYRlZTSXNqbEE5RGxWNDdO?=
 =?utf-8?B?Uy8vRzdON2RndGNaWnRVcUdvKzY0MjhXaW1CTVBJSU1RdDNKaFZ3aTk0WTJP?=
 =?utf-8?B?dGpxcC9ybkMxK2Nzd3ZibWFrRTMveGNNQ3NCUzRhQ3Jhc3h1OXh6d29RSi84?=
 =?utf-8?B?UmxiSENFSVN5Q3JyYnhST1NVZFZxalZqaUJibVZ0SmNibytsWWlpWjhQRUJm?=
 =?utf-8?B?ZEJoaUZTR3JLQnRpTUFFZHZlcUlYcDRoRlRyNXZEcVlPQURsR3UzUUkybDc2?=
 =?utf-8?B?THVyMnpqbzV5Zmpod3RVSUQxeEZwVDNxOVpyN3pkYUlrVTF2RkQ2dzFXNUxO?=
 =?utf-8?B?em5RWTdUby9jYkt4M3BHamhrS0pPeGJmaW0yMVZBMXY3WVZHQmxIVjc1c2lt?=
 =?utf-8?B?MXB2b3psYlY1T0E3amlvUGRreGJpUE5HQ2RMa2tPTEpaRnNUTTlVZ2ZMbU5z?=
 =?utf-8?B?YWJLd1JWS01JWlgrWUtacWx6clMyUy82MWE2dWRQYlBYcndzTVYyRjEvUy9M?=
 =?utf-8?B?Y1pabW1oZUQrSmRFVTJxZHpsYnZ4SmhlVFVibS9VN00yTE5ycWkzZjNpWWxX?=
 =?utf-8?B?M0pOSmFMb2ZHcjU1R0FpdlROQlYvV05Edy9JcVVaUGlETUdmTTBPRWxMNGZ3?=
 =?utf-8?B?cmV2c2JwTnUySkNVNCtCMTZxZEsrRjhJZFVDR1FjaGRYSm9GSVZRenNwNW5D?=
 =?utf-8?B?aXFZN2hnWUhKUVlLVm45dHJWK3pOK3A3Vms1Z0lObUthQlQwekVDTFpWWWZ5?=
 =?utf-8?B?M05oU0xsaUJ0NDFsWHRaUmgxbjVkK0ozRzF4SGNhczhxNnZPKzFsUGVCNFFz?=
 =?utf-8?B?TGZGcnZTUHJJTkVGTW9MSkIvZklRK1FqTythOFdSUHZzWjdBYjdGZmQvemlX?=
 =?utf-8?B?eGVBdXEyMXZGZHF0UjBZNG1INUloN25GaFNDbTBkc2VJc1kybnB3SjljVVRX?=
 =?utf-8?B?aFlKcWd4endIZWZ6am5nYWhYNjcxVWZCVnNiS1ZiYUl4Yi9qUjBBeFExeW8y?=
 =?utf-8?B?SUlDOFNCY2twbHd1YndzbndQZFhTNyt0OE5WeXppakJNVXdkOWUrZC8xRmNt?=
 =?utf-8?B?TkNmSTJRbUVpUC8zNlRNWVhzR2hlQjFWTW1ZVk50Qi82Ny9OZ0VPWU05Tk1q?=
 =?utf-8?B?VE5ndW1ud21WR0hFVDNMREFhekV6WkhLdDB1cEVSVy96YXpXZTVyaUZ3UWE1?=
 =?utf-8?B?c1Y4ZlZNUk9WLzVGQ0xUQjB5WW9IZUIrd2hHamxlNWxiaStWZytiM3M4eVl2?=
 =?utf-8?B?MmpBUVdZQ2x3MTR1cmYxbk1wcThiaVdIQVZwMUd6a1BYS05ydmY3cDdwbGYz?=
 =?utf-8?B?a1VDNnFPb0ZRLzh4U05GMW4vdzNLRDhSRGhta1VUVzU5bkl0LzlkOUtRYTRX?=
 =?utf-8?B?V084bW13ZmJ0R1pOZUlBTTRnSEdiQmgyQ040YWVjLzlydjd5UFREMThaUUpW?=
 =?utf-8?B?dFUvVWRqWFBYVEpVdjBQVmJXOXU4NkNIeGl4UVc5REhsN0daVXBZOHZxb1o4?=
 =?utf-8?B?TWNNRmRqaHU2b04yTStqdklGRXFCQkNvbEc1aFFBUTNlS3laMG1TcU1pbzh1?=
 =?utf-8?B?WjNsNm01bmhhL1g1Zk1GY1hoZVFpS1pmeTNjYVFWWEVHaFR0dlJab2NyY2g1?=
 =?utf-8?B?cVFSaG1HK2x1MUptOG9WQ29nMmJhYnI2QWNkQkI5dExISmdQbnZTNmFtb1dh?=
 =?utf-8?B?Y1I4dFVoaXQ4U1FwTjRWRm0rdjJwcEVHN0Y0TGhRUm52V2psd2RSMlZvRGht?=
 =?utf-8?B?MEg3ZUJ1c0czeE9raXAzWk9zQnkydnpZVHdjRm5HbFh4M1FMRnlxSGp3U3B3?=
 =?utf-8?B?R2Q5cGhDMWpLS1Q0MWJIS2dEbUxRRXpieXVGcXhsMEVXYUNlcUp3aTEzSlZW?=
 =?utf-8?B?cEhPcVZ1a1JoVi83WmNhRSt0QTd3MFZIQlBrY1JjOWtMY1dzeDZHakYvWGM2?=
 =?utf-8?B?S3JjcGNaT2l1VVd0Nnh1RUV0MVRNRjl6Z0x5Nm9OVloxcmtBbnUvSkQzeEcw?=
 =?utf-8?B?WjI4bER3NmxaRDI2OWh3RWVIT1A4amNvZWZZbHdwOEFSQ3hBS2RHSms4anJT?=
 =?utf-8?B?ZUE9PQ==?=
X-OriginatorOrg: nutanix.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 906c7ba0-3ddd-4251-2f34-08db2aee9fed
X-MS-Exchange-CrossTenant-AuthSource: BL3PR02MB8217.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Mar 2023 16:00:57.9261
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bb047546-786f-4de1-bd75-24e5b6f79043
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: HCRnM1jabCx7SJNsBSAPkSdH7amKVpPprvtNRIhMM03jhEB0X1yBCI0s26cFajRuxHWLfPqc265VLR8WpF14GueRi2UAfOOrh1qAC1dRSsM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR02MB7796
X-Proofpoint-GUID: XEmmhip7L68O94e9NF5pO-DCe8F441Mh
X-Proofpoint-ORIG-GUID: XEmmhip7L68O94e9NF5pO-DCe8F441Mh
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-03-22_13,2023-03-22_01,2023-02-09_01
X-Proofpoint-Spam-Reason: safe
X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_NONE,
        SPF_NONE,URIBL_BLOCKED autolearn=unavailable autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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
>