Re: [PATCH v2] netfilter: Make xt_cgroup independent from net_cls

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Michal,

I have one question.

On Wed, Mar 05, 2025 at 06:09:35PM +0100, Michal Koutný wrote:
> diff --git a/net/netfilter/xt_cgroup.c b/net/netfilter/xt_cgroup.c
> index c0f5e9a4f3c65..c3055e74aa0ea 100644
> --- a/net/netfilter/xt_cgroup.c
> +++ b/net/netfilter/xt_cgroup.c
> @@ -23,6 +23,13 @@ MODULE_DESCRIPTION("Xtables: process control group matching");
>  MODULE_ALIAS("ipt_cgroup");
>  MODULE_ALIAS("ip6t_cgroup");
>  
> +#define NET_CLS_CLASSID_INVALID_MSG "xt_cgroup: classid invalid without net_cls cgroups\n"
> +
> +static bool possible_classid(u32 classid)
> +{
> +	return IS_ENABLED(CONFIG_CGROUP_NET_CLASSID) || classid == 0;
> +}
> +
>  static int cgroup_mt_check_v0(const struct xt_mtchk_param *par)
>  {
>  	struct xt_cgroup_info_v0 *info = par->matchinfo;
> @@ -30,6 +37,11 @@ static int cgroup_mt_check_v0(const struct xt_mtchk_param *par)
>  	if (info->invert & ~1)
>  		return -EINVAL;
>  
> +	if (!possible_classid(info->id)) {

why classid != 0 is accepted for cgroup_mt_check_v0()?

cgroup_mt_check_v0 represents revision 0 of this match, and this match
only supports for clsid (groupsv1).

History of revisions of cgroupsv2:

- cgroup_mt_check_v0 added to match on clsid (initial version of this match)
- cgroup_mt_check_v1 is added to support cgroupsv2 matching 
- cgroup_mt_check_v2 is added to make cgroupsv2 matching more flexible

I mean, if !IS_ENABLED(CONFIG_CGROUP_NET_CLASSID) then xt_cgroup
should fail for cgroup_mt_check_v0.

But a more general question: why this check for classid == 0 in
cgroup_mt_check_v1 and cgroup_mt_check_v2?

> +		pr_info(NET_CLS_CLASSID_INVALID_MSG);
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -51,6 +63,11 @@ static int cgroup_mt_check_v1(const struct xt_mtchk_param *par)
>  		return -EINVAL;
>  	}
>  
> +	if (!possible_classid(info->classid)) {
> +		pr_info(NET_CLS_CLASSID_INVALID_MSG);
> +		return -EINVAL;
> +	}
> +
>  	info->priv = NULL;
>  	if (info->has_path) {
>  		cgrp = cgroup_get_from_path(info->path);
> @@ -83,6 +100,11 @@ static int cgroup_mt_check_v2(const struct xt_mtchk_param *par)
>  		return -EINVAL;
>  	}
>  
> +	if (info->has_classid && !possible_classid(info->classid)) {
> +		pr_info(NET_CLS_CLASSID_INVALID_MSG);
> +		return -EINVAL;
> +	}
> +
>  	info->priv = NULL;
>  	if (info->has_path) {
>  		cgrp = cgroup_get_from_path(info->path);
> 
> base-commit: dd83757f6e686a2188997cb58b5975f744bb7786
> -- 
> 2.48.1
>