Re: [PATCH] segtree: bail out on concatenations

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

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

On Thu, Apr 02, 2020 at 11:49:41PM +0200, Pablo Neira Ayuso wrote:
> This patch adds a lazy check to validate that the first element is not a
> concatenation. The segtree code does not support for concatenations,
> bail out with EOPNOTSUPP.
>
>  # nft add element x y { 10.0.0.0/8 . 192.168.1.3-192.168.1.9 . 1024-65535 }
>  Error: Could not process rule: Operation not supported
>  add element x y { 10.0.0.0/8 . 192.168.1.3-192.168.1.9 . 1024-65535 }
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Otherwise, the segtree code barfs with:
>
>  BUG: invalid range expression type concat

Hm.

I'm afraid this patch is not enough, the following ruleset crashes
in old kernels with recent nft:

flush ruleset

table inet filter {
        set test {
                type ipv4_addr . ipv4_addr . inet_service
                flags interval,timeout
                elements = { 1.1.1.1 . 2.2.2.2 . 30 ,
                             2.2.2.2 . 3.3.3.3 . 40 ,
                             3.3.3.3 . 4.4.4.4 . 50 }
        }

        chain output {
                type filter hook output priority 0; policy accept;
                ip saddr . ip daddr . tcp dport @test counter
        }
}

Old kernel obviously does not support for concatenation with intervals.

I think Stefano's NFT_SET_CONCAT flag needs to be restored in the
kernel (see attached patch) to deal with old kernel properly.

On set creation, old kernels bail out since they do not know what to
do with this patch.

Then, the next problem is that the kernel says -EINVAL in this case,
this error code is bad. It should only be used for malformed messages,
if NFT_SET_CONCAT is set on and kernel does not support this, it
should instead just report EOPNOTSUPP (see the second patch in this
email). I'm also updating the error code for the object maps since
there might be a need to introduce new objects in the future.

Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 1323 bytes --]

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 30f2a87270dc..4565456c0ef4 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -276,6 +276,7 @@ enum nft_rule_compat_attributes {
  * @NFT_SET_TIMEOUT: set uses timeouts
  * @NFT_SET_EVAL: set can be updated from the evaluation path
  * @NFT_SET_OBJECT: set contains stateful objects
+ * @NFT_SET_CONCAT: set contains a concatenation
  */
 enum nft_set_flags {
 	NFT_SET_ANONYMOUS		= 0x1,
@@ -285,6 +286,7 @@ enum nft_set_flags {
 	NFT_SET_TIMEOUT			= 0x10,
 	NFT_SET_EVAL			= 0x20,
 	NFT_SET_OBJECT			= 0x40,
+	NFT_SET_CONCAT			= 0x80,
 };
 
 /**
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d0ab5ffa1e2c..235762b91dd4 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3961,7 +3961,7 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
 		if (flags & ~(NFT_SET_ANONYMOUS | NFT_SET_CONSTANT |
 			      NFT_SET_INTERVAL | NFT_SET_TIMEOUT |
 			      NFT_SET_MAP | NFT_SET_EVAL |
-			      NFT_SET_OBJECT))
+			      NFT_SET_OBJECT | NFT_SET_CONCAT))
 			return -EINVAL;
 		/* Only one of these operations is supported */
 		if ((flags & (NFT_SET_MAP | NFT_SET_OBJECT)) ==
-- 
2.11.0


[-- Attachment #3: y.patch --]
[-- Type: text/x-diff, Size: 925 bytes --]

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d0ab5ffa1e2c..63e38ac1c0f8 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -3962,7 +3962,7 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
 			      NFT_SET_INTERVAL | NFT_SET_TIMEOUT |
 			      NFT_SET_MAP | NFT_SET_EVAL |
 			      NFT_SET_OBJECT | NFT_SET_CONCAT))
-			return -EINVAL;
+			return -EOPNOTSUPP;
 		/* Only one of these operations is supported */
 		if ((flags & (NFT_SET_MAP | NFT_SET_OBJECT)) ==
 			     (NFT_SET_MAP | NFT_SET_OBJECT))
@@ -4000,7 +4000,7 @@ static int nf_tables_newset(struct net *net, struct sock *nlsk,
 		objtype = ntohl(nla_get_be32(nla[NFTA_SET_OBJ_TYPE]));
 		if (objtype == NFT_OBJECT_UNSPEC ||
 		    objtype > NFT_OBJECT_MAX)
-			return -EINVAL;
+			return -EOPNOTSUPP;
 	} else if (flags & NFT_SET_OBJECT)
 		return -EINVAL;
 	else
-- 
2.11.0