The comment in cgroup_file_write is missing some interfaces, such as
'cgroup.threads'. All delegatable files are listed in
'/sys/kernel/cgroup/delegate', so update the comment in cgroup_file_write.
Besides, add a statement that files outside the namespace shouldn't be
visible from inside the delegated namespace.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 Documentation/admin-guide/cgroup-v2.rst | 10 ++++++----
 kernel/cgroup/cgroup.c                  |  4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst
index f2d1ec7d6aba..2665d08159fb 100644
--- a/Documentation/admin-guide/cgroup-v2.rst
+++ b/Documentation/admin-guide/cgroup-v2.rst
@@ -533,10 +533,12 @@ cgroup namespace on namespace creation.
 Because the resource control interface files in a given directory
 control the distribution of the parent's resources, the delegatee
 shouldn't be allowed to write to them.  For the first method, this is
-achieved by not granting access to these files.  For the second, the
-kernel rejects writes to all files other than "cgroup.procs" and
-"cgroup.subtree_control" on a namespace root from inside the
-namespace.
+achieved by not granting access to these files.  For the second, files
+outside the namespace shouldn't be visible from within the delegated
+namespace, and the kernel rejects writes to all files on a namespace
+root from inside the namespace, except for those files listed in
+"/sys/kernel/cgroup/delegate" (including "cgroup.procs", "cgroup.threads",
+"cgroup.subtree_control", etc.).
 
 The end results are equivalent for both delegation types.  Once
 delegated, the user can build sub-hierarchy under the directory,
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index a3fa645f8433..9758686f0332 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4130,8 +4130,8 @@ static ssize_t cgroup_file_write(struct kernfs_open_file *of, char *buf,
 	/*
 	 * If namespaces are delegation boundaries, disallow writes to
 	 * files in an non-init namespace root from inside the namespace
-	 * except for the files explicitly marked delegatable -
-	 * cgroup.procs and cgroup.subtree_control.
+	 * except for the set delegatable files shown in /sys/kernel/cgroup/delegate,
+	 * including cgroup.procs, cgroup.threads and cgroup.subtree_control, etc.
 	 */
 	if ((cgrp->root->flags & CGRP_ROOT_NS_DELEGATE) &&
 	    !(cft->flags & CFTYPE_NS_DELEGATABLE) &&
-- 
2.34.1

Hi,
thanks for writing up on the care needed when you only use namespacing
(and not de-privilgation) for delegation.

On Thu, Aug 15, 2024 at 02:41:18AM GMT, Chen Ridong <chenridong@huawei.com> wrote:
...

What about some more clarifications to prevent other confusions?

> --- a/Documentation/admin-guide/cgroup-v2.rst
> +++ b/Documentation/admin-guide/cgroup-v2.rst
> @@ -533,10 +533,12 @@ cgroup namespace on namespace creation.
>  Because the resource control interface files in a given directory
>  control the distribution of the parent's resources, the delegatee
>  shouldn't be allowed to write to them.  For the first method, this is
> -achieved by not granting access to these files.  For the second, the
> -kernel rejects writes to all files other than "cgroup.procs" and
> -"cgroup.subtree_control" on a namespace root from inside the
> -namespace.
> +achieved by not granting access to these files.  For the second, files
> +outside the namespace shouldn't be visible from within the delegated
                         should be hidden from the delegatee by the
means of at least mount namespacing, and the kernel...

> +namespace, and the kernel rejects writes to all files on a namespace
> +root from inside the namespace, except for those files listed in
          inside the cgroup namespace

> +"/sys/kernel/cgroup/delegate" (including "cgroup.procs", "cgroup.threads",
> +"cgroup.subtree_control", etc.).
  
...
> -	 * except for the files explicitly marked delegatable -
> -	 * cgroup.procs and cgroup.subtree_control.
> +	 * except for the set delegatable files shown in /sys/kernel/cgroup/delegate,
> +	 * including cgroup.procs, cgroup.threads and cgroup.subtree_control, etc.

"Marked delegatable" (meaning CFTYPE_NS_DELEGATABLE) is appropriate
comment in the code, a reference to the sysfs file is only consequential
to this marking. A minimal change would be like:

-	 * cgroup.procs and cgroup.subtree_control.
+	 * e.g. cgroup.procs and cgroup.subtree_control.

On 2024/8/15 20:12, Michal Koutný wrote:
> Hi,
> thanks for writing up on the care needed when you only use namespacing
> (and not de-privilgation) for delegation.
> 
> On Thu, Aug 15, 2024 at 02:41:18AM GMT, Chen Ridong <chenridong@huawei.com> wrote:
> ...
> 
> What about some more clarifications to prevent other confusions?
> 
>> --- a/Documentation/admin-guide/cgroup-v2.rst
>> +++ b/Documentation/admin-guide/cgroup-v2.rst
>> @@ -533,10 +533,12 @@ cgroup namespace on namespace creation.
>>   Because the resource control interface files in a given directory
>>   control the distribution of the parent's resources, the delegatee
>>   shouldn't be allowed to write to them.  For the first method, this is
>> -achieved by not granting access to these files.  For the second, the
>> -kernel rejects writes to all files other than "cgroup.procs" and
>> -"cgroup.subtree_control" on a namespace root from inside the
>> -namespace.
>> +achieved by not granting access to these files.  For the second, files
>> +outside the namespace shouldn't be visible from within the delegated
>                           should be hidden from the delegatee by the
> means of at least mount namespacing, and the kernel...
> 
>> +namespace, and the kernel rejects writes to all files on a namespace
>> +root from inside the namespace, except for those files listed in
>            inside the cgroup namespace
> 
>> +"/sys/kernel/cgroup/delegate" (including "cgroup.procs", "cgroup.threads",
>> +"cgroup.subtree_control", etc.).
>    
> ...
>> -	 * except for the files explicitly marked delegatable -
>> -	 * cgroup.procs and cgroup.subtree_control.
>> +	 * except for the set delegatable files shown in /sys/kernel/cgroup/delegate,
>> +	 * including cgroup.procs, cgroup.threads and cgroup.subtree_control, etc.
> 
> "Marked delegatable" (meaning CFTYPE_NS_DELEGATABLE) is appropriate
> comment in the code, a reference to the sysfs file is only consequential
> to this marking. A minimal change would be like:
> 
> -	 * cgroup.procs and cgroup.subtree_control.
> +	 * e.g. cgroup.procs and cgroup.subtree_control.
Thank you, Michal, I will send new patch.

Thanks,
Ridong