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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Hello Chen.
On Fri, Dec 18, 2020 at 02:17:55PM +0800, Chen Zhou <[email protected]> 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
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 <[email protected]> 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
On Thu, Jan 14, 2021 at 10:08:19PM +0800, chenzhou <[email protected]> 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
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
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.
>
On 2021/1/15 0:54, Michal Koutn? wrote:
> On Thu, Jan 14, 2021 at 10:08:19PM +0800, chenzhou <[email protected]> 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
On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou <[email protected]> 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? <[email protected]>
> The behavior was changed since commit f5dfb5315d34 ("cgroup: take
> options parsing into ->parse_monolithic()"), will add this as Fixes.
Thanks.
Michal
Hi Michal,
On 2021/1/15 18:08, Michal Koutn? wrote:
> On Fri, Jan 15, 2021 at 09:55:43AM +0800, chenzhou <[email protected]> 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? <[email protected]>
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