All of lore.kernel.org
 help / color / mirror / Atom feed
* [nf-next RFC] netfilter: nf_tables: Introduce NFTA_DEVICE_WILDCARD
@ 2025-07-24 22:00 Phil Sutter
  2025-07-25 15:12 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Phil Sutter @ 2025-07-24 22:00 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel

On netlink receive side, this attribute is just another name for
NFTA_DEVICE_NAME and handled equally. It enables user space to detect
lack of wildcard interface spec support as older kernels will reject it.

On netlink send side, it is used for wildcard interface specs to avoid
confusing or even crashing old user space with non NUL-terminated
strings in attributes which are expected to be NUL-terminated.

Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
While this works, I wonder if it should be named NFTA_DEVICE_PREFIX
instead and contain NUL-terminated strings just like NFTA_DEVICE_NAME.
Kernel-internally I would continue using strncmp() and hook->ifnamelen,
but handling in user space might be simpler.

A downside of this approach is that we mix NFTA_DEVICE_NAME and
NFTA_DEVICE_WILDCARD attributes in NFTA_FLOWTABLE_HOOK_DEVS and
NFTA_HOOK_DEVS nested attributes, even though old user space will reject
the whole thing and not just take the known attributes and ignore the
rest.
---
 include/uapi/linux/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c            | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 2beb30be2c5f..ed85b61afcd8 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -1788,6 +1788,7 @@ enum nft_synproxy_attributes {
 enum nft_devices_attributes {
 	NFTA_DEVICE_UNSPEC,
 	NFTA_DEVICE_NAME,
+	NFTA_DEVICE_WILDCARD,
 	__NFTA_DEVICE_MAX
 };
 #define NFTA_DEVICE_MAX		(__NFTA_DEVICE_MAX - 1)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 04795af6e586..f65b4fba5225 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1959,6 +1959,14 @@ static int nft_dump_stats(struct sk_buff *skb, struct nft_stats __percpu *stats)
 	return -ENOSPC;
 }
 
+static int hook_device_attr(struct nft_hook *hook)
+{
+	if (hook->ifname[hook->ifnamelen - 1] == '\0')
+		return NFTA_DEVICE_NAME;
+
+	return NFTA_DEVICE_WILDCARD;
+}
+
 static int nft_dump_basechain_hook(struct sk_buff *skb,
 				   const struct net *net, int family,
 				   const struct nft_base_chain *basechain,
@@ -1990,7 +1998,7 @@ static int nft_dump_basechain_hook(struct sk_buff *skb,
 			if (!first)
 				first = hook;
 
-			if (nla_put(skb, NFTA_DEVICE_NAME,
+			if (nla_put(skb, hook_device_attr(hook),
 				    hook->ifnamelen, hook->ifname))
 				goto nla_put_failure;
 			n++;
@@ -1998,6 +2006,7 @@ static int nft_dump_basechain_hook(struct sk_buff *skb,
 		nla_nest_end(skb, nest_devs);
 
 		if (n == 1 &&
+		    hook_device_attr(first) != NFTA_DEVICE_WILDCARD &&
 		    nla_put(skb, NFTA_HOOK_DEV,
 			    first->ifnamelen, first->ifname))
 			goto nla_put_failure;
@@ -2376,7 +2385,8 @@ static int nf_tables_parse_netdev_hooks(struct net *net,
 	int rem, n = 0, err;
 
 	nla_for_each_nested(tmp, attr, rem) {
-		if (nla_type(tmp) != NFTA_DEVICE_NAME) {
+		if (nla_type(tmp) != NFTA_DEVICE_NAME &&
+		    nla_type(tmp) != NFTA_DEVICE_WILDCARD) {
 			err = -EINVAL;
 			goto err_hook;
 		}
@@ -9427,7 +9437,7 @@ static int nf_tables_fill_flowtable_info(struct sk_buff *skb, struct net *net,
 
 	list_for_each_entry_rcu(hook, hook_list, list,
 				lockdep_commit_lock_is_held(net)) {
-		if (nla_put(skb, NFTA_DEVICE_NAME,
+		if (nla_put(skb, hook_device_attr(hook),
 			    hook->ifnamelen, hook->ifname))
 			goto nla_put_failure;
 	}
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [nf-next RFC] netfilter: nf_tables: Introduce NFTA_DEVICE_WILDCARD
  2025-07-24 22:00 [nf-next RFC] netfilter: nf_tables: Introduce NFTA_DEVICE_WILDCARD Phil Sutter
@ 2025-07-25 15:12 ` Pablo Neira Ayuso
  2025-07-25 21:51   ` Phil Sutter
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2025-07-25 15:12 UTC (permalink / raw)
  To: Phil Sutter; +Cc: Florian Westphal, netfilter-devel

Hi Phil,

On Fri, Jul 25, 2025 at 12:00:31AM +0200, Phil Sutter wrote:
> On netlink receive side, this attribute is just another name for
> NFTA_DEVICE_NAME and handled equally. It enables user space to detect
> lack of wildcard interface spec support as older kernels will reject it.
> 
> On netlink send side, it is used for wildcard interface specs to avoid
> confusing or even crashing old user space with non NUL-terminated
> strings in attributes which are expected to be NUL-terminated.

This looks good to me.

> Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> While this works, I wonder if it should be named NFTA_DEVICE_PREFIX
> instead and contain NUL-terminated strings just like NFTA_DEVICE_NAME.
> Kernel-internally I would continue using strncmp() and hook->ifnamelen,
> but handling in user space might be simpler.

Pick the name you like.

> A downside of this approach is that we mix NFTA_DEVICE_NAME and
> NFTA_DEVICE_WILDCARD attributes in NFTA_FLOWTABLE_HOOK_DEVS and
> NFTA_HOOK_DEVS nested attributes, even though old user space will reject
> the whole thing and not just take the known attributes and ignore the
> rest.

Old userspace is just ignoring the unknown attribute?

I think upside is good enough to follow this approach: new userspace
version with old kernel bails out with EINVAL, so it is easy to see
that feature is unsupported.

As for netlink attributes coming from the kernel, we can just review
the existing userspace parsing side and see what we can do better in
that regard.

> ---
>  include/uapi/linux/netfilter/nf_tables.h |  1 +
>  net/netfilter/nf_tables_api.c            | 16 +++++++++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index 2beb30be2c5f..ed85b61afcd8 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -1788,6 +1788,7 @@ enum nft_synproxy_attributes {
>  enum nft_devices_attributes {
>  	NFTA_DEVICE_UNSPEC,
>  	NFTA_DEVICE_NAME,
> +	NFTA_DEVICE_WILDCARD,
>  	__NFTA_DEVICE_MAX
>  };
>  #define NFTA_DEVICE_MAX		(__NFTA_DEVICE_MAX - 1)
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 04795af6e586..f65b4fba5225 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -1959,6 +1959,14 @@ static int nft_dump_stats(struct sk_buff *skb, struct nft_stats __percpu *stats)
>  	return -ENOSPC;
>  }
>  
> +static int hook_device_attr(struct nft_hook *hook)
> +{
> +	if (hook->ifname[hook->ifnamelen - 1] == '\0')
> +		return NFTA_DEVICE_NAME;
> +
> +	return NFTA_DEVICE_WILDCARD;
> +}
> +
>  static int nft_dump_basechain_hook(struct sk_buff *skb,
>  				   const struct net *net, int family,
>  				   const struct nft_base_chain *basechain,
> @@ -1990,7 +1998,7 @@ static int nft_dump_basechain_hook(struct sk_buff *skb,
>  			if (!first)
>  				first = hook;
>  
> -			if (nla_put(skb, NFTA_DEVICE_NAME,
> +			if (nla_put(skb, hook_device_attr(hook),
>  				    hook->ifnamelen, hook->ifname))
>  				goto nla_put_failure;
>  			n++;
> @@ -1998,6 +2006,7 @@ static int nft_dump_basechain_hook(struct sk_buff *skb,
>  		nla_nest_end(skb, nest_devs);
>  
>  		if (n == 1 &&
> +		    hook_device_attr(first) != NFTA_DEVICE_WILDCARD &&
>  		    nla_put(skb, NFTA_HOOK_DEV,
>  			    first->ifnamelen, first->ifname))
>  			goto nla_put_failure;
> @@ -2376,7 +2385,8 @@ static int nf_tables_parse_netdev_hooks(struct net *net,
>  	int rem, n = 0, err;
>  
>  	nla_for_each_nested(tmp, attr, rem) {
> -		if (nla_type(tmp) != NFTA_DEVICE_NAME) {
> +		if (nla_type(tmp) != NFTA_DEVICE_NAME &&
> +		    nla_type(tmp) != NFTA_DEVICE_WILDCARD) {
>  			err = -EINVAL;
>  			goto err_hook;
>  		}
> @@ -9427,7 +9437,7 @@ static int nf_tables_fill_flowtable_info(struct sk_buff *skb, struct net *net,
>  
>  	list_for_each_entry_rcu(hook, hook_list, list,
>  				lockdep_commit_lock_is_held(net)) {
> -		if (nla_put(skb, NFTA_DEVICE_NAME,
> +		if (nla_put(skb, hook_device_attr(hook),
>  			    hook->ifnamelen, hook->ifname))
>  			goto nla_put_failure;
>  	}
> -- 
> 2.49.0
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [nf-next RFC] netfilter: nf_tables: Introduce NFTA_DEVICE_WILDCARD
  2025-07-25 15:12 ` Pablo Neira Ayuso
@ 2025-07-25 21:51   ` Phil Sutter
  2025-07-29  0:30     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Phil Sutter @ 2025-07-25 21:51 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel

Hi Pablo,

On Fri, Jul 25, 2025 at 05:12:42PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Jul 25, 2025 at 12:00:31AM +0200, Phil Sutter wrote:
> > On netlink receive side, this attribute is just another name for
> > NFTA_DEVICE_NAME and handled equally. It enables user space to detect
> > lack of wildcard interface spec support as older kernels will reject it.
> > 
> > On netlink send side, it is used for wildcard interface specs to avoid
> > confusing or even crashing old user space with non NUL-terminated
> > strings in attributes which are expected to be NUL-terminated.
> 
> This looks good to me.
> 
> > Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > While this works, I wonder if it should be named NFTA_DEVICE_PREFIX
> > instead and contain NUL-terminated strings just like NFTA_DEVICE_NAME.
> > Kernel-internally I would continue using strncmp() and hook->ifnamelen,
> > but handling in user space might be simpler.
> 
> Pick the name you like.

Ah, it's not just about the name. The initial version using
NFTA_DEVICE_NAME for both, distinction of wildcards from regular
names came from missing '\0' terminator. With distinct attribute types,
this is not needed anymore. I guess it's more user (space) friendly to
include the NUL-char in wildcards as well, right?

> > A downside of this approach is that we mix NFTA_DEVICE_NAME and
> > NFTA_DEVICE_WILDCARD attributes in NFTA_FLOWTABLE_HOOK_DEVS and
> > NFTA_HOOK_DEVS nested attributes, even though old user space will reject
> > the whole thing and not just take the known attributes and ignore the
> > rest.
> 
> Old userspace is just ignoring the unknown attribute?

Attribute parser in libnftnl will abort if it finds an attribute with
type other than NFTA_DEVICE_NAME nested in NFTA_HOOK_DEVS (or the
flowtable equivalent). So old userspace will refuse to parse the data,
but not crash at least.

> I think upside is good enough to follow this approach: new userspace
> version with old kernel bails out with EINVAL, so it is easy to see
> that feature is unsupported.

ACK, it is definitely much more sane than before!

> As for netlink attributes coming from the kernel, we can just review
> the existing userspace parsing side and see what we can do better in
> that regard.

We could introduce a "NFTA_DEVICE_NAME_NEW" which may hold wildcards or
a regular name (thereby keeping the NUL-char distinction mentioned
above) and at some point drop NFTA_DEVICE_NAME. Basically a merge
strategy to upgrade NFTA_DEVICE_NAME to support also wildcards, but I'm
not sure how long this transition period will take. At least it would
never crash old user space, but "merely" become incompatible to it at
some point.

Cheers, Phil

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [nf-next RFC] netfilter: nf_tables: Introduce NFTA_DEVICE_WILDCARD
  2025-07-25 21:51   ` Phil Sutter
@ 2025-07-29  0:30     ` Pablo Neira Ayuso
  2025-07-31 22:54       ` Phil Sutter
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2025-07-29  0:30 UTC (permalink / raw)
  To: Phil Sutter, Florian Westphal, netfilter-devel

Hi Phil,

On Fri, Jul 25, 2025 at 11:51:12PM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Fri, Jul 25, 2025 at 05:12:42PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Jul 25, 2025 at 12:00:31AM +0200, Phil Sutter wrote:
> > > On netlink receive side, this attribute is just another name for
> > > NFTA_DEVICE_NAME and handled equally. It enables user space to detect
> > > lack of wildcard interface spec support as older kernels will reject it.
> > > 
> > > On netlink send side, it is used for wildcard interface specs to avoid
> > > confusing or even crashing old user space with non NUL-terminated
> > > strings in attributes which are expected to be NUL-terminated.
> > 
> > This looks good to me.
> > 
> > > Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > > While this works, I wonder if it should be named NFTA_DEVICE_PREFIX
> > > instead and contain NUL-terminated strings just like NFTA_DEVICE_NAME.
> > > Kernel-internally I would continue using strncmp() and hook->ifnamelen,
> > > but handling in user space might be simpler.
> > 
> > Pick the name you like.
> 
> Ah, it's not just about the name. The initial version using
> NFTA_DEVICE_NAME for both, distinction of wildcards from regular
> names came from missing '\0' terminator. With distinct attribute types,
> this is not needed anymore. I guess it's more user (space) friendly to
> include the NUL-char in wildcards as well, right?

Yes. In practise, you can put anything over netlink (someone decided
to sending strings, not even TLVs)...

But two different types provides clear semantics, no need to peek on
the value to know what to do.

> > > A downside of this approach is that we mix NFTA_DEVICE_NAME and
> > > NFTA_DEVICE_WILDCARD attributes in NFTA_FLOWTABLE_HOOK_DEVS and
> > > NFTA_HOOK_DEVS nested attributes, even though old user space will reject
> > > the whole thing and not just take the known attributes and ignore the
> > > rest.
> > 
> > Old userspace is just ignoring the unknown attribute?
> 
> Attribute parser in libnftnl will abort if it finds an attribute with
> type other than NFTA_DEVICE_NAME nested in NFTA_HOOK_DEVS (or the
> flowtable equivalent). So old userspace will refuse to parse the data,
> but not crash at least.

Please, fix it so we can do better in the future.

> > I think upside is good enough to follow this approach: new userspace
> > version with old kernel bails out with EINVAL, so it is easy to see
> > that feature is unsupported.
> 
> ACK, it is definitely much more sane than before!

OK.

I suggest you formally submit this for nf.git including userspace
patches? Then, request it to be included in -stable. We probably have
to skip including this userspace code in the next 1.1.4 release.
Unless anyone have a better proposal to handle this. I'm sorry I did
not bring up this issue sooner.

> > As for netlink attributes coming from the kernel, we can just review
> > the existing userspace parsing side and see what we can do better in
> > that regard.
> 
> We could introduce a "NFTA_DEVICE_NAME_NEW" which may hold wildcards or
> a regular name (thereby keeping the NUL-char distinction mentioned
> above) and at some point drop NFTA_DEVICE_NAME. Basically a merge
> strategy to upgrade NFTA_DEVICE_NAME to support also wildcards, but I'm
> not sure how long this transition period will take. At least it would
> never crash old user space, but "merely" become incompatible to it at
> some point.

I don't think it is worth, as for old user space, IMO the only
reasonable thing we can do is:

- do not crash.
- highlight that old user space is skipping unknown stuff.

Other than that, we would have to explore a generic way to print raw
attributes, then extend the parser to allow this, which I am not
convinced yet it is worth the effort.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [nf-next RFC] netfilter: nf_tables: Introduce NFTA_DEVICE_WILDCARD
  2025-07-29  0:30     ` Pablo Neira Ayuso
@ 2025-07-31 22:54       ` Phil Sutter
  2025-08-07 12:28         ` Pablo Neira Ayuso
  0 siblings, 1 reply; 7+ messages in thread
From: Phil Sutter @ 2025-07-31 22:54 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel

On Tue, Jul 29, 2025 at 02:30:54AM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Fri, Jul 25, 2025 at 11:51:12PM +0200, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Fri, Jul 25, 2025 at 05:12:42PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Jul 25, 2025 at 12:00:31AM +0200, Phil Sutter wrote:
> > > > On netlink receive side, this attribute is just another name for
> > > > NFTA_DEVICE_NAME and handled equally. It enables user space to detect
> > > > lack of wildcard interface spec support as older kernels will reject it.
> > > > 
> > > > On netlink send side, it is used for wildcard interface specs to avoid
> > > > confusing or even crashing old user space with non NUL-terminated
> > > > strings in attributes which are expected to be NUL-terminated.
> > > 
> > > This looks good to me.
> > > 
> > > > Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
> > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > ---
> > > > While this works, I wonder if it should be named NFTA_DEVICE_PREFIX
> > > > instead and contain NUL-terminated strings just like NFTA_DEVICE_NAME.
> > > > Kernel-internally I would continue using strncmp() and hook->ifnamelen,
> > > > but handling in user space might be simpler.
> > > 
> > > Pick the name you like.
> > 
> > Ah, it's not just about the name. The initial version using
> > NFTA_DEVICE_NAME for both, distinction of wildcards from regular
> > names came from missing '\0' terminator. With distinct attribute types,
> > this is not needed anymore. I guess it's more user (space) friendly to
> > include the NUL-char in wildcards as well, right?
> 
> Yes. In practise, you can put anything over netlink (someone decided
> to sending strings, not even TLVs)...
> 
> But two different types provides clear semantics, no need to peek on
> the value to know what to do.

ACK.

> > > > A downside of this approach is that we mix NFTA_DEVICE_NAME and
> > > > NFTA_DEVICE_WILDCARD attributes in NFTA_FLOWTABLE_HOOK_DEVS and
> > > > NFTA_HOOK_DEVS nested attributes, even though old user space will reject
> > > > the whole thing and not just take the known attributes and ignore the
> > > > rest.
> > > 
> > > Old userspace is just ignoring the unknown attribute?
> > 
> > Attribute parser in libnftnl will abort if it finds an attribute with
> > type other than NFTA_DEVICE_NAME nested in NFTA_HOOK_DEVS (or the
> > flowtable equivalent). So old userspace will refuse to parse the data,
> > but not crash at least.
> 
> Please, fix it so we can do better in the future.
> 
> > > I think upside is good enough to follow this approach: new userspace
> > > version with old kernel bails out with EINVAL, so it is easy to see
> > > that feature is unsupported.
> > 
> > ACK, it is definitely much more sane than before!
> 
> OK.
> 
> I suggest you formally submit this for nf.git including userspace
> patches? Then, request it to be included in -stable. We probably have
> to skip including this userspace code in the next 1.1.4 release.
> Unless anyone have a better proposal to handle this. I'm sorry I did
> not bring up this issue sooner.

The Fixes: tag will suffice for -stable, correct?

I don't see why we have to hold back user space. There was no support
for these wildcards in nft tool yet, so probably nothing relies upon the
old (i.e., non-NUL-terminated strings in NFTA_DEVICE_NAME) behaviour
yet. Even if something does, the kernel (with my patch applied) will
treat it sanely as non-wildcard.

Updated user space facing a kernel prior to my patch will detect lack of
wildcard support, even if the kernel would support it (via
non-NUL-terminated string in NFTA_DEVICE_NAME).

Am I missing your point?

> > > As for netlink attributes coming from the kernel, we can just review
> > > the existing userspace parsing side and see what we can do better in
> > > that regard.
> > 
> > We could introduce a "NFTA_DEVICE_NAME_NEW" which may hold wildcards or
> > a regular name (thereby keeping the NUL-char distinction mentioned
> > above) and at some point drop NFTA_DEVICE_NAME. Basically a merge
> > strategy to upgrade NFTA_DEVICE_NAME to support also wildcards, but I'm
> > not sure how long this transition period will take. At least it would
> > never crash old user space, but "merely" become incompatible to it at
> > some point.
> 
> I don't think it is worth, as for old user space, IMO the only
> reasonable thing we can do is:
> 
> - do not crash.

With NFTA_DEVICE_PREFIX, old user space will stop parsing the netlink
message (nftnl_{chain,flowtable}_nlmsg_parse() will return -1). Given
the limited control, this is almost ideal behaviour.

> - highlight that old user space is skipping unknown stuff.

I sent an RFC implementing Florian's suggested solution to detect
potential problems a few months ago but didn't get a reply:

Subject: [nft RFC] table: Embed creating nft version into userdata
Message-ID: <20250512210321.29032-1-phil@nwl.cc>

Maybe putting "PATCH" in the prefix would have provoked feedback? :D
Could you please have a look? I think it is quite unintrusive and
probably the most reliable way of letting users know something may be
odd in the output they're seeing.

> Other than that, we would have to explore a generic way to print raw
> attributes, then extend the parser to allow this, which I am not
> convinced yet it is worth the effort.

This may work for error reporting/debug output in libnftnl, but I doubt
exposing the opaque data in nft output to make it survive a
save'n'restore is doable.

Maybe we could add versions to netlink messages so user space knows if
it will understand all content or not. This could even allow users to
specify a max version to use on command line and nft would reject any
input requiring a larger version. Of course this means tracking "version
requirements" of expressions and other features.

Cheers, Phil

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [nf-next RFC] netfilter: nf_tables: Introduce NFTA_DEVICE_WILDCARD
  2025-07-31 22:54       ` Phil Sutter
@ 2025-08-07 12:28         ` Pablo Neira Ayuso
  2025-08-07 13:58           ` Phil Sutter
  0 siblings, 1 reply; 7+ messages in thread
From: Pablo Neira Ayuso @ 2025-08-07 12:28 UTC (permalink / raw)
  To: Phil Sutter, Florian Westphal, netfilter-devel

Hi Phil,

On Fri, Aug 01, 2025 at 12:54:33AM +0200, Phil Sutter wrote:
> On Tue, Jul 29, 2025 at 02:30:54AM +0200, Pablo Neira Ayuso wrote:
> > Hi Phil,
> > 
> > On Fri, Jul 25, 2025 at 11:51:12PM +0200, Phil Sutter wrote:
> > > Hi Pablo,
> > > 
> > > On Fri, Jul 25, 2025 at 05:12:42PM +0200, Pablo Neira Ayuso wrote:
> > > > On Fri, Jul 25, 2025 at 12:00:31AM +0200, Phil Sutter wrote:
> > > > > On netlink receive side, this attribute is just another name for
> > > > > NFTA_DEVICE_NAME and handled equally. It enables user space to detect
> > > > > lack of wildcard interface spec support as older kernels will reject it.
> > > > > 
> > > > > On netlink send side, it is used for wildcard interface specs to avoid
> > > > > confusing or even crashing old user space with non NUL-terminated
> > > > > strings in attributes which are expected to be NUL-terminated.
> > > > 
> > > > This looks good to me.
> > > > 
> > > > > Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
> > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > ---
> > > > > While this works, I wonder if it should be named NFTA_DEVICE_PREFIX
> > > > > instead and contain NUL-terminated strings just like NFTA_DEVICE_NAME.
> > > > > Kernel-internally I would continue using strncmp() and hook->ifnamelen,
> > > > > but handling in user space might be simpler.
> > > > 
> > > > Pick the name you like.
> > > 
> > > Ah, it's not just about the name. The initial version using
> > > NFTA_DEVICE_NAME for both, distinction of wildcards from regular
> > > names came from missing '\0' terminator. With distinct attribute types,
> > > this is not needed anymore. I guess it's more user (space) friendly to
> > > include the NUL-char in wildcards as well, right?
> > 
> > Yes. In practise, you can put anything over netlink (someone decided
> > to sending strings, not even TLVs)...
> > 
> > But two different types provides clear semantics, no need to peek on
> > the value to know what to do.
> 
> ACK.
> 
> > > > > A downside of this approach is that we mix NFTA_DEVICE_NAME and
> > > > > NFTA_DEVICE_WILDCARD attributes in NFTA_FLOWTABLE_HOOK_DEVS and
> > > > > NFTA_HOOK_DEVS nested attributes, even though old user space will reject
> > > > > the whole thing and not just take the known attributes and ignore the
> > > > > rest.
> > > > 
> > > > Old userspace is just ignoring the unknown attribute?
> > > 
> > > Attribute parser in libnftnl will abort if it finds an attribute with
> > > type other than NFTA_DEVICE_NAME nested in NFTA_HOOK_DEVS (or the
> > > flowtable equivalent). So old userspace will refuse to parse the data,
> > > but not crash at least.
> > 
> > Please, fix it so we can do better in the future.
> > 
> > > > I think upside is good enough to follow this approach: new userspace
> > > > version with old kernel bails out with EINVAL, so it is easy to see
> > > > that feature is unsupported.
> > > 
> > > ACK, it is definitely much more sane than before!
> > 
> > OK.
> > 
> > I suggest you formally submit this for nf.git including userspace
> > patches? Then, request it to be included in -stable. We probably have
> > to skip including this userspace code in the next 1.1.4 release.
> > Unless anyone have a better proposal to handle this. I'm sorry I did
> > not bring up this issue sooner.
> 
> The Fixes: tag will suffice for -stable, correct?

I think so, we can also request inclusion in -stable explicitly.

> I don't see why we have to hold back user space. There was no support
> for these wildcards in nft tool yet, so probably nothing relies upon the
> old (i.e., non-NUL-terminated strings in NFTA_DEVICE_NAME) behaviour
> yet. Even if something does, the kernel (with my patch applied) will
> treat it sanely as non-wildcard.

Yes.

> Updated user space facing a kernel prior to my patch will detect lack of
> wildcard support, even if the kernel would support it (via
> non-NUL-terminated string in NFTA_DEVICE_NAME).
> 
> Am I missing your point?

At this time, I am more concerned about new userspace with old kernel.
EINVAL should be returned if user requests wildcard and kernel does
not support.

Will you formally post this patch then? Or you prefer different
approach?

Anyway, coming back to the (forward) compatibility issues in
containers...

> > > > As for netlink attributes coming from the kernel, we can just review
> > > > the existing userspace parsing side and see what we can do better in
> > > > that regard.
> > > 
> > > We could introduce a "NFTA_DEVICE_NAME_NEW" which may hold wildcards or
> > > a regular name (thereby keeping the NUL-char distinction mentioned
> > > above) and at some point drop NFTA_DEVICE_NAME. Basically a merge
> > > strategy to upgrade NFTA_DEVICE_NAME to support also wildcards, but I'm
> > > not sure how long this transition period will take. At least it would
> > > never crash old user space, but "merely" become incompatible to it at
> > > some point.
> > 
> > I don't think it is worth, as for old user space, IMO the only
> > reasonable thing we can do is:
> > 
> > - do not crash.
> 
> With NFTA_DEVICE_PREFIX, old user space will stop parsing the netlink
> message (nftnl_{chain,flowtable}_nlmsg_parse() will return -1). Given
> the limited control, this is almost ideal behaviour.
>
> > - highlight that old user space is skipping unknown stuff.
> 
> I sent an RFC implementing Florian's suggested solution to detect
> potential problems a few months ago but didn't get a reply:
> 
> Subject: [nft RFC] table: Embed creating nft version into userdata
> Message-ID: <20250512210321.29032-1-phil@nwl.cc>
> 
> Maybe putting "PATCH" in the prefix would have provoked feedback? :D
> Could you please have a look? I think it is quite unintrusive and
> probably the most reliable way of letting users know something may be
> odd in the output they're seeing.
>
> > Other than that, we would have to explore a generic way to print raw
> > attributes, then extend the parser to allow this, which I am not
> > convinced yet it is worth the effort.
> 
> This may work for error reporting/debug output in libnftnl, but I doubt
> exposing the opaque data in nft output to make it survive a
> save'n'restore is doable.
> 
> Maybe we could add versions to netlink messages so user space knows if
> it will understand all content or not. This could even allow users to
> specify a max version to use on command line and nft would reject any
> input requiring a larger version. Of course this means tracking "version
> requirements" of expressions and other features.

Maybe exposing a description of the netlink bus capabilities can help?
I sent a patchset a long time ago, but this never received any
feedback. I think version numbers are tricky.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [nf-next RFC] netfilter: nf_tables: Introduce NFTA_DEVICE_WILDCARD
  2025-08-07 12:28         ` Pablo Neira Ayuso
@ 2025-08-07 13:58           ` Phil Sutter
  0 siblings, 0 replies; 7+ messages in thread
From: Phil Sutter @ 2025-08-07 13:58 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel

On Thu, Aug 07, 2025 at 02:28:16PM +0200, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Fri, Aug 01, 2025 at 12:54:33AM +0200, Phil Sutter wrote:
> > On Tue, Jul 29, 2025 at 02:30:54AM +0200, Pablo Neira Ayuso wrote:
> > > Hi Phil,
> > > 
> > > On Fri, Jul 25, 2025 at 11:51:12PM +0200, Phil Sutter wrote:
> > > > Hi Pablo,
> > > > 
> > > > On Fri, Jul 25, 2025 at 05:12:42PM +0200, Pablo Neira Ayuso wrote:
> > > > > On Fri, Jul 25, 2025 at 12:00:31AM +0200, Phil Sutter wrote:
> > > > > > On netlink receive side, this attribute is just another name for
> > > > > > NFTA_DEVICE_NAME and handled equally. It enables user space to detect
> > > > > > lack of wildcard interface spec support as older kernels will reject it.
> > > > > > 
> > > > > > On netlink send side, it is used for wildcard interface specs to avoid
> > > > > > confusing or even crashing old user space with non NUL-terminated
> > > > > > strings in attributes which are expected to be NUL-terminated.
> > > > > 
> > > > > This looks good to me.
> > > > > 
> > > > > > Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
> > > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > > ---
> > > > > > While this works, I wonder if it should be named NFTA_DEVICE_PREFIX
> > > > > > instead and contain NUL-terminated strings just like NFTA_DEVICE_NAME.
> > > > > > Kernel-internally I would continue using strncmp() and hook->ifnamelen,
> > > > > > but handling in user space might be simpler.
> > > > > 
> > > > > Pick the name you like.
> > > > 
> > > > Ah, it's not just about the name. The initial version using
> > > > NFTA_DEVICE_NAME for both, distinction of wildcards from regular
> > > > names came from missing '\0' terminator. With distinct attribute types,
> > > > this is not needed anymore. I guess it's more user (space) friendly to
> > > > include the NUL-char in wildcards as well, right?
> > > 
> > > Yes. In practise, you can put anything over netlink (someone decided
> > > to sending strings, not even TLVs)...
> > > 
> > > But two different types provides clear semantics, no need to peek on
> > > the value to know what to do.
> > 
> > ACK.
> > 
> > > > > > A downside of this approach is that we mix NFTA_DEVICE_NAME and
> > > > > > NFTA_DEVICE_WILDCARD attributes in NFTA_FLOWTABLE_HOOK_DEVS and
> > > > > > NFTA_HOOK_DEVS nested attributes, even though old user space will reject
> > > > > > the whole thing and not just take the known attributes and ignore the
> > > > > > rest.
> > > > > 
> > > > > Old userspace is just ignoring the unknown attribute?
> > > > 
> > > > Attribute parser in libnftnl will abort if it finds an attribute with
> > > > type other than NFTA_DEVICE_NAME nested in NFTA_HOOK_DEVS (or the
> > > > flowtable equivalent). So old userspace will refuse to parse the data,
> > > > but not crash at least.
> > > 
> > > Please, fix it so we can do better in the future.
> > > 
> > > > > I think upside is good enough to follow this approach: new userspace
> > > > > version with old kernel bails out with EINVAL, so it is easy to see
> > > > > that feature is unsupported.
> > > > 
> > > > ACK, it is definitely much more sane than before!
> > > 
> > > OK.
> > > 
> > > I suggest you formally submit this for nf.git including userspace
> > > patches? Then, request it to be included in -stable. We probably have
> > > to skip including this userspace code in the next 1.1.4 release.
> > > Unless anyone have a better proposal to handle this. I'm sorry I did
> > > not bring up this issue sooner.
> > 
> > The Fixes: tag will suffice for -stable, correct?
> 
> I think so, we can also request inclusion in -stable explicitly.
> 
> > I don't see why we have to hold back user space. There was no support
> > for these wildcards in nft tool yet, so probably nothing relies upon the
> > old (i.e., non-NUL-terminated strings in NFTA_DEVICE_NAME) behaviour
> > yet. Even if something does, the kernel (with my patch applied) will
> > treat it sanely as non-wildcard.
> 
> Yes.
> 
> > Updated user space facing a kernel prior to my patch will detect lack of
> > wildcard support, even if the kernel would support it (via
> > non-NUL-terminated string in NFTA_DEVICE_NAME).
> > 
> > Am I missing your point?
> 
> At this time, I am more concerned about new userspace with old kernel.
> EINVAL should be returned if user requests wildcard and kernel does
> not support.
> 
> Will you formally post this patch then? Or you prefer different
> approach?

Uhm ... I thought to have submitted this already, but can't find it on
the list. The updated user space patches are there, no idea what went
wrong. Just (re-)sent:

| Subject: [nf-next PATCH] netfilter: nf_tables: Introduce NFTA_DEVICE_PREFIX
| Date: Thu,  7 Aug 2025 15:49:59 +0200

> Anyway, coming back to the (forward) compatibility issues in
> containers...
> 
> > > > > As for netlink attributes coming from the kernel, we can just review
> > > > > the existing userspace parsing side and see what we can do better in
> > > > > that regard.
> > > > 
> > > > We could introduce a "NFTA_DEVICE_NAME_NEW" which may hold wildcards or
> > > > a regular name (thereby keeping the NUL-char distinction mentioned
> > > > above) and at some point drop NFTA_DEVICE_NAME. Basically a merge
> > > > strategy to upgrade NFTA_DEVICE_NAME to support also wildcards, but I'm
> > > > not sure how long this transition period will take. At least it would
> > > > never crash old user space, but "merely" become incompatible to it at
> > > > some point.
> > > 
> > > I don't think it is worth, as for old user space, IMO the only
> > > reasonable thing we can do is:
> > > 
> > > - do not crash.
> > 
> > With NFTA_DEVICE_PREFIX, old user space will stop parsing the netlink
> > message (nftnl_{chain,flowtable}_nlmsg_parse() will return -1). Given
> > the limited control, this is almost ideal behaviour.
> >
> > > - highlight that old user space is skipping unknown stuff.
> > 
> > I sent an RFC implementing Florian's suggested solution to detect
> > potential problems a few months ago but didn't get a reply:
> > 
> > Subject: [nft RFC] table: Embed creating nft version into userdata
> > Message-ID: <20250512210321.29032-1-phil@nwl.cc>
> > 
> > Maybe putting "PATCH" in the prefix would have provoked feedback? :D
> > Could you please have a look? I think it is quite unintrusive and
> > probably the most reliable way of letting users know something may be
> > odd in the output they're seeing.
> >
> > > Other than that, we would have to explore a generic way to print raw
> > > attributes, then extend the parser to allow this, which I am not
> > > convinced yet it is worth the effort.
> > 
> > This may work for error reporting/debug output in libnftnl, but I doubt
> > exposing the opaque data in nft output to make it survive a
> > save'n'restore is doable.
> > 
> > Maybe we could add versions to netlink messages so user space knows if
> > it will understand all content or not. This could even allow users to
> > specify a max version to use on command line and nft would reject any
> > input requiring a larger version. Of course this means tracking "version
> > requirements" of expressions and other features.
> 
> Maybe exposing a description of the netlink bus capabilities can help?
> I sent a patchset a long time ago, but this never received any
> feedback. I think version numbers are tricky.

I'm not sure such netlink data would help. Consider the typical case of
new kernel with ruleset created by new user space. What should happen if
old user space sends a dump request to kernel with insufficient
capabilities? The kernel could merely reject the request, right? If it
dumps and user space sees the dump having unknown capabilities, it could
merely refuse to parse it (or warn and risk crashing).

I fear any more intelligent approach will end as a can of worms we have
to keep maintaining forever. :(

Cheers, Phil

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-08-07 13:58 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-24 22:00 [nf-next RFC] netfilter: nf_tables: Introduce NFTA_DEVICE_WILDCARD Phil Sutter
2025-07-25 15:12 ` Pablo Neira Ayuso
2025-07-25 21:51   ` Phil Sutter
2025-07-29  0:30     ` Pablo Neira Ayuso
2025-07-31 22:54       ` Phil Sutter
2025-08-07 12:28         ` Pablo Neira Ayuso
2025-08-07 13:58           ` Phil Sutter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.