[PATCH v2] netfilter: Make xt_cgroup independent from net_cls

[PATCH v2] netfilter: Make xt_cgroup independent from net_cls

[Michal Koutný]

The xt_group matching supports the default hierarchy since commit
c38c4597e4bf3 ("netfilter: implement xt_cgroup cgroup2 path match").
The cgroup v1 matching (based on clsid) and cgroup v2 matching (based on
path) are rather independent. Downgrade the Kconfig dependency to
mere CONFIG_SOCK_GROUP_DATA so that xt_group can be built even without
CONFIG_NET_CLS_CGROUP for path matching.
Also add a message for users when they attempt to specify any
non-trivial clsid.

Link: https://lists.opensuse.org/archives/list/kernel@lists.opensuse.org/thread/S23NOILB7MUIRHSKPBOQKJHVSK26GP6X/
Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
 net/netfilter/Kconfig     |  2 +-
 net/netfilter/xt_cgroup.c | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

Changes from v1 (https://lore.kernel.org/r/20250228165216.339407-1-mkoutny@suse.com)
- terser guard (Jan)
- verboser message (Florian)
- better select !CGROUP_BPF (kernel test robot)

diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index df2dc21304efb..346ac2152fa18 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -1180,7 +1180,7 @@ config NETFILTER_XT_MATCH_CGROUP
 	tristate '"control group" match support'
 	depends on NETFILTER_ADVANCED
 	depends on CGROUPS
-	select CGROUP_NET_CLASSID
+	select SOCK_CGROUP_DATA
 	help
 	Socket/process control group matching allows you to match locally
 	generated packets based on which net_cls control group processes
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)) {
+		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

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

[Michal Koutný]

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

Hello.

On Wed, Mar 05, 2025 at 06:09:35PM +0100, Michal Koutný <mkoutny@suse.com> wrote:
...
> Changes from v1 (https://lore.kernel.org/r/20250228165216.339407-1-mkoutny@suse.com)
> - terser guard (Jan)
> - verboser message (Florian)
> - better select !CGROUP_BPF (kernel test robot)

Are there any more remarks or should I resend this?

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Pablo Neira Ayuso]

On Wed, Mar 19, 2025 at 06:35:47PM +0100, Michal Koutný wrote:
> Hello.
> 
> On Wed, Mar 05, 2025 at 06:09:35PM +0100, Michal Koutný <mkoutny@suse.com> wrote:
> ...
> > Changes from v1 (https://lore.kernel.org/r/20250228165216.339407-1-mkoutny@suse.com)
> > - terser guard (Jan)
> > - verboser message (Florian)
> > - better select !CGROUP_BPF (kernel test robot)
> 
> Are there any more remarks or should I resend this?

No need to resend, thanks.

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

[Pablo Neira Ayuso]

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
> 

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

[Michal Koutný]

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

Hello Pablo.

On Sun, Mar 23, 2025 at 10:20:10AM +0100, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> why classid != 0 is accepted for cgroup_mt_check_v0()?

It is opposite, only classid == 0 is accepted (that should be same for
all of v0..v2). (OTOH, there should be no change in validation with
CONFIG_CGROUP_NET_CLASSID.)

> 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.


I considered classid == 0 valid (regardless of CONFIG_*) as counterpart
to implementation of sock_cgroup_classid() that collapses to 0 when
!CONFIG_CGROUP_NET_CLASSID (thus at least rules with classid=0 remain
acceptable).

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

cgroup_mt_check_v1 is for cgroupv2 OR classid matching. Similar with
cgroup_mt_check_v2.

IOW, all three versions accept classid=0 with !CONFIG_CGROUP_NET_CLASSID
equally because that is the value that sockets reported classid falls
back to.

But please correct me if I misunderstood the logic.

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Pablo Neira Ayuso]

On Mon, Mar 24, 2025 at 01:56:07PM +0100, Michal Koutný wrote:
> Hello Pablo.
> 
> On Sun, Mar 23, 2025 at 10:20:10AM +0100, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > why classid != 0 is accepted for cgroup_mt_check_v0()?
> 
> It is opposite, only classid == 0 is accepted (that should be same for
> all of v0..v2). (OTOH, there should be no change in validation with
> CONFIG_CGROUP_NET_CLASSID.)

Thanks for clarifying this, more questions below.

> > 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.
> 
> 
> I considered classid == 0 valid (regardless of CONFIG_*) as counterpart
> to implementation of sock_cgroup_classid() that collapses to 0 when
> !CONFIG_CGROUP_NET_CLASSID (thus at least rules with classid=0 remain
> acceptable).

That is, 0 is the default value when !CONFIG_CGROUP_NET_CLASSID.

> > But a more general question: why this check for classid == 0 in
> > cgroup_mt_check_v1 and cgroup_mt_check_v2?
> 
> cgroup_mt_check_v1 is for cgroupv2 OR classid matching. Similar with
> cgroup_mt_check_v2.

Yes, and cgroup_mt_check_v0 only supports for classid matching.

> IOW, all three versions accept classid=0 with !CONFIG_CGROUP_NET_CLASSID
> equally because that is the value that sockets reported classid falls
> back to.

If !CONFIG_CGROUP_NET_CLASSID, then no classid matching is possible.

So why allow a rule to match on cgroup with classid == 0?

Maybe simply do this instead?

static bool possible_classid(u32 classid)
{
       return IS_ENABLED(CONFIG_CGROUP_NET_CLASSID);
}

Thanks.

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

[Michal Koutný]

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

On Mon, Mar 24, 2025 at 05:49:09PM +0100, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> If !CONFIG_CGROUP_NET_CLASSID, then no classid matching is possible.
> 
> So why allow a rule to match on cgroup with classid == 0?

It is conservative approach to supposed users who may have filtering
rules with classid=0 but never mkdir any net_cls group. Only those who
eventually need to mkdir would realize there's nowhere to mkdir on (with
!CONFIG_CGROUP_NET_CLASSID). Admittedly, I have no idea if this helps to
5% of net_cls users or 0.05% or 0%. Do you have any insights into that?

> Maybe simply do this instead?
> 
> static bool possible_classid(u32 classid)
> {
>        return IS_ENABLED(CONFIG_CGROUP_NET_CLASSID);
> }

Yes, if the above carefulness is unnecessary, I'd like to accompany this
with complete removal of sock_cgroup_classid() function then (to have it
compile-checked that it's really impossible to compare any classids w/o
CONFIG_CGROUP_NET_CLASSID).

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Pablo Neira Ayuso]

On Mon, Mar 24, 2025 at 07:01:14PM +0100, Michal Koutný wrote:
> On Mon, Mar 24, 2025 at 05:49:09PM +0100, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > If !CONFIG_CGROUP_NET_CLASSID, then no classid matching is possible.
> > 
> > So why allow a rule to match on cgroup with classid == 0?
> 
> It is conservative approach to supposed users who may have filtering
> rules with classid=0 but never mkdir any net_cls group. Only those who
> eventually need to mkdir would realize there's nowhere to mkdir on (with
> !CONFIG_CGROUP_NET_CLASSID). Admittedly, I have no idea if this helps to
> 5% of net_cls users or 0.05% or 0%. Do you have any insights into that?

I suspect this partial support will not help anyway, because user will
be most likely matching to classid != 0 in their rulesets, and the
ruleset loads via iptables-restore in an atomic fashion, ie. take it
all or nothing.

> > Maybe simply do this instead?
> > 
> > static bool possible_classid(u32 classid)
> > {
> >        return IS_ENABLED(CONFIG_CGROUP_NET_CLASSID);
> > }
> 
> Yes, if the above carefulness is unnecessary, I'd like to accompany this
> with complete removal of sock_cgroup_classid() function then (to have it
> compile-checked that it's really impossible to compare any classids w/o
> CONFIG_CGROUP_NET_CLASSID).

Go ahead remove this shim function and post v3.

Thanks.