[PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[Chen Zhou]

When mounting a cgroup hierarchy with disabled controller in cgroup v1,
all available controllers will be attached.

Add disabled controller check in cgroup1_parse_param() and return directly
if the specified controller is disabled.

Signed-off-by: Chen Zhou <chenzhou10-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
Changes in v2:
- Fix line over 80 characters warning.
---
 kernel/cgroup/cgroup-v1.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
index 191c329e482a..5190c42fea8b 100644
--- a/kernel/cgroup/cgroup-v1.c
+++ b/kernel/cgroup/cgroup-v1.c
@@ -915,6 +915,9 @@ int cgroup1_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		for_each_subsys(ss, i) {
 			if (strcmp(param->key, ss->legacy_name))
 				continue;
+			if (!cgroup_ssid_enabled(i) || cgroup1_ssid_disabled(i))
+				return invalfc(fc, "Disabled controller '%s'",
+					       param->key);
 			ctx->subsys_mask |= (1 << i);
 			return 0;
 		}
-- 
2.20.1

Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[Zefan Li]

On 2020/12/18 14:17, Chen Zhou wrote:
> When mounting a cgroup hierarchy with disabled controller in cgroup v1,
> all available controllers will be attached.
> 
> Add disabled controller check in cgroup1_parse_param() and return directly
> if the specified controller is disabled.
> 
> Signed-off-by: Chen Zhou <chenzhou10-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> Changes in v2:
> - Fix line over 80 characters warning.
> ---
>  kernel/cgroup/cgroup-v1.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/cgroup/cgroup-v1.c b/kernel/cgroup/cgroup-v1.c
> index 191c329e482a..5190c42fea8b 100644
> --- a/kernel/cgroup/cgroup-v1.c
> +++ b/kernel/cgroup/cgroup-v1.c
> @@ -915,6 +915,9 @@ int cgroup1_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  		for_each_subsys(ss, i) {
>  			if (strcmp(param->key, ss->legacy_name))
>  				continue;
> +			if (!cgroup_ssid_enabled(i) || cgroup1_ssid_disabled(i))
> +				return invalfc(fc, "Disabled controller '%s'",
> +					       param->key);
>  			ctx->subsys_mask |= (1 << i);
>  			return 0;
>  		}

Reviewed-by: Zefan Li <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

Hello Chen.

On Fri, Dec 18, 2020 at 02:17:55PM +0800, Chen Zhou <chenzhou10@huawei.com> wrote:
> When mounting a cgroup hierarchy with disabled controller in cgroup v1,
> all available controllers will be attached.
Not sure if I understand the situation -- have you observed a v1
controller attached to a hierarchy while specifying cgroup_no_v1= kernel
cmdline arg?

AFAICS, the disabled controllers are honored thanks to
check_cgroupfs_options().

Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[chenzhou]

Hi Michal,

On 2021/1/14 21:12, Michal Koutný wrote:
> Hello Chen.
>
> On Fri, Dec 18, 2020 at 02:17:55PM +0800, Chen Zhou <chenzhou10-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
>> When mounting a cgroup hierarchy with disabled controller in cgroup v1,
>> all available controllers will be attached.
> Not sure if I understand the situation -- have you observed a v1
> controller attached to a hierarchy while specifying cgroup_no_v1= kernel
> cmdline arg?
Yeah, this is the situation.
In this case, at the beginning of function check_cgroupfs_options(), the mask
ctx->subsys_mask will be 0. And if we mount without 'none' and 'name=' options,
then in check_cgroupfs_options(), the flag ctx->all_ss will be set, that is, select all the subsystems.

Thanks,
Chen Zhou
>
> AFAICS, the disabled controllers are honored thanks to
> check_cgroupfs_options().
>
> Michal

Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

On Thu, Jan 14, 2021 at 10:08:19PM +0800, chenzhou <chenzhou10-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
> In this case, at the beginning of function check_cgroupfs_options(), the mask
> ctx->subsys_mask will be 0. And if we mount without 'none' and 'name=' options,
> then in check_cgroupfs_options(), the flag ctx->all_ss will be set, that is, select all the subsystems.
But even then, the line
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/cgroup/cgroup-v1.c?h=v5.11-rc3#n1012
would select only 'enabled' controllers, wouldn't it?

It's possible I missed something but if this means that cgroup_no_v1=
doesn't hold to its expectations, I'd suggest adding proper Fixes: tag
to the patch.

Thanks,
Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[chenzhou]

On 2021/1/15 0:54, Michal Koutný wrote:
> On Thu, Jan 14, 2021 at 10:08:19PM +0800, chenzhou <chenzhou10-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
>> In this case, at the beginning of function check_cgroupfs_options(), the mask
>> ctx->subsys_mask will be 0. And if we mount without 'none' and 'name=' options,
>> then in check_cgroupfs_options(), the flag ctx->all_ss will be set, that is, select all the subsystems.
> But even then, the line
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/cgroup/cgroup-v1.c?h=v5.11-rc3#n1012
> would select only 'enabled' controllers, wouldn't it?
Yeah, this will select all enabled controllers, but which doesn't the behavior we want.
I think the case should return error with information "Disabled controller xx" rather than
attaching all the other enabled controllers.

For example, boot with cgroup_no_v1=cpu, and then mount with
"mount -t cgroup -o cpu cpu /sys/fs/cgroup/cpu", then all enabled controllers will
be attached expect cpu.
>
> It's possible I missed something but if this means that cgroup_no_v1=
> doesn't hold to its expectations, I'd suggest adding proper Fixes: tag
> to the patch.
See above. Just the mount behavior isn't what we what.
The behavior was changed since commit f5dfb5315d34 ("cgroup: take options parsing into ->parse_monolithic()"),
will add this as Fixes.

Thanks,
Chen Zhou
>
> Thanks,
> Michal

Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[Tejun Heo]

Hello,

On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou wrote:
> Yeah, this will select all enabled controllers, but which doesn't the behavior we want.
> I think the case should return error with information "Disabled controller xx" rather than
> attaching all the other enabled controllers.
> 
> For example, boot with cgroup_no_v1=cpu, and then mount with
> "mount -t cgroup -o cpu cpu /sys/fs/cgroup/cpu", then all enabled controllers will
> be attached expect cpu.

Okay, that explanation actually makes sense. Can you please update the
description to include what's broken and how it's being fixed? It really
isn't clear what the patch is trying to achieve from the current
description.

Thanks.

-- 
tejun

Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[chenzhou]

On 2021/1/15 11:17, Tejun Heo wrote:
> Hello,
>
> On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou wrote:
>> Yeah, this will select all enabled controllers, but which doesn't the behavior we want.
>> I think the case should return error with information "Disabled controller xx" rather than
>> attaching all the other enabled controllers.
>>
>> For example, boot with cgroup_no_v1=cpu, and then mount with
>> "mount -t cgroup -o cpu cpu /sys/fs/cgroup/cpu", then all enabled controllers will
>> be attached expect cpu.
> Okay, that explanation actually makes sense. Can you please update the
> description to include what's broken and how it's being fixed? It really
> isn't clear what the patch is trying to achieve from the current
> description.
Ok, will update the description.
>
> Thanks.
>

Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou <chenzhou10-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
> Yeah, this will select all enabled controllers, but which doesn't the behavior we want.
I see what the issue is now.

> See above. Just the mount behavior isn't what we what.
I agree this a bug and your I find your approach correct

Reviewed-by: Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org>

> The behavior was changed since commit f5dfb5315d34 ("cgroup: take
> options parsing into ->parse_monolithic()"), will add this as Fixes.
Thanks.

Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] cgroup-v1: add disabled controller check in cgroup1_parse_param()

[chenzhou]

Hi Michal,

On 2021/1/15 18:08, Michal Koutný wrote:
> On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou <chenzhou10-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
>> Yeah, this will select all enabled controllers, but which doesn't the behavior we want.
> I see what the issue is now.
>
>> See above. Just the mount behavior isn't what we what.
> I agree this a bug and your I find your approach correct
>
> Reviewed-by: Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org>
I have sent the v3, which updates the description and add Fixes. 
[v3]: https://lore.kernel.org/patchwork/patch/1365535/
If it is ok for you to add Reviewed-by there.

Thanks,
Chen Zhou
>
>> The behavior was changed since commit f5dfb5315d34 ("cgroup: take
>> options parsing into ->parse_monolithic()"), will add this as Fixes.
> Thanks.
>
> Michal