linux-kernel - Re: [PATCH -next] cgroup: Use descriptor table to unify mount flag management

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <nz6urfhwkgigftrovogbwzeqnrsnrnslmxcvpere7bv2im4uho@mdfhkvmpret4>
Date: Tue, 2 Dec 2025 10:24:11 +0100
From: Michal Koutný <mkoutny@...e.com>
To: Chen Ridong <chenridong@...weicloud.com>
Cc: tj@...nel.org, hannes@...xchg.org, cgroups@...r.kernel.org, 
	linux-kernel@...r.kernel.org, lujialin4@...wei.com, chenridong@...wei.com
Subject: Re: [PATCH -next] cgroup: Use descriptor table to unify mount flag
 management

Hi Ridong.

On Wed, Nov 26, 2025 at 02:08:25AM +0000, Chen Ridong <chenridong@...weicloud.com> wrote:
> From: Chen Ridong <chenridong@...wei.com>
> 
> The cgroup2 mount flags (e.g. nsdelegate, favordynmods) were previously
> handled via scattered switch-case and conditional checks across
> parameter parsing, flag application, and option display paths. This
> leads to redundant code and increased maintenance cost when adding/removing
> flags.
> 
> Introduce a `cgroup_mount_flag_desc` descriptor table to centralize the
> mapping between flag bits, names, and apply functions. Refactor the
> relevant paths to use this table for unified management:
> 1. cgroup2_parse_param: Replace switch-case with table lookup
> 2. apply_cgroup_root_flags: Replace multiple conditionals with table
>    iteration
> 3. cgroup_show_options: Replace hardcoded seq_puts with table-driven output
> 
> No functional change intended, and the mount option output format remains
> compatible with the original implementation.

At first I thought this is worthy but then I ran into the possible
(semantic) overlap with the cgroup2_fs_parameters array (the string
`name`s are duplicated in both :-/), I didn't figure out a way how to
make such an polymorphic array in C (like when cgroup_mount_flag_desc
would be a class that inherits from fs_parameter_spec and you could pass
the array of the formers to consumers (fs_parse()) of latters).

So I'm wondering whether there exists some way to avoid possible
divergence between definitions of the two arrays...

(Below are some notes I had made.)

> 
> Signed-off-by: Chen Ridong <chenridong@...wei.com>
> ---
>  kernel/cgroup/cgroup.c | 107 +++++++++++++++++++----------------------
>  1 file changed, 49 insertions(+), 58 deletions(-)
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index e717208cfb18..1e4033d05c29 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2005,6 +2005,36 @@ static const struct fs_parameter_spec cgroup2_fs_parameters[] = {
>  	{}
>  };
>  
> +struct cgroup_mount_flag_desc {
> +	u64 flag;

This could use an enum for CGROUP_ROOT_* values.
(For readability/convenience.)

> +	const char *name;
> +	void (*apply)(struct cgroup_root *root, u64 bit, bool enable);

I leave up to your discretion whether the cgroup_root should be passed
explicitly (it's always cgrp_dfl_root and existing functions reach to
the symbol already).

> +};
> +
> +static void apply_cgroup_favor_flags(struct cgroup_root *root,
> +					     u64 bit, bool enable)
> +{
> +	return cgroup_favor_dynmods(root, enable);
> +}
> +
> +static void __apply_cgroup_root_flags(struct cgroup_root *root,
> +					      u64 bit, bool enable)

I think double underscore is overkill given `static` and the previous
helper.

> +{
> +	if (enable)
> +		root->flags |= bit;
> +	else
> +		root->flags &= ~bit;
> +}
> +
> +static const struct cgroup_mount_flag_desc mount_flags_desc[nr__cgroup2_params] = {
> +{CGRP_ROOT_NS_DELEGATE, "nsdelegate", __apply_cgroup_root_flags},
> +{CGRP_ROOT_FAVOR_DYNMODS, "favordynmods", apply_cgroup_favor_flags},
> +{CGRP_ROOT_MEMORY_LOCAL_EVENTS, "memory_localevents", __apply_cgroup_root_flags},
> +{CGRP_ROOT_MEMORY_RECURSIVE_PROT, "memory_recursiveprot", __apply_cgroup_root_flags},
> +{CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING, "memory_hugetlb_accounting", __apply_cgroup_root_flags},
> +{CGRP_ROOT_PIDS_LOCAL_EVENTS, "pids_localevents", __apply_cgroup_root_flags}
> +};

It seems indentation is missing here.
This is actually a specialization of the cgroup2_fs_parameters array...


Download attachment "signature.asc" of type "application/pgp-signature" (266 bytes)