[PATCH] segtree: bail out on concatenations

[PATCH] segtree: bail out on concatenations

[Pablo Neira Ayuso]

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

Reported-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/segtree.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/segtree.c b/src/segtree.c
index 8d79332d8578..85310f62c429 100644
--- a/src/segtree.c
+++ b/src/segtree.c
@@ -419,6 +419,17 @@ static int set_to_segtree(struct list_head *msgs, struct set *set,
 	unsigned int n;
 	int err;
 
+	/* Probe for the first element to check for concatenations, this code
+	 * does not support for intervals and concatenations.
+	 */
+	if (init) {
+		i = list_first_entry(&init->expressions, struct expr, list);
+		if (i->key->etype == EXPR_CONCAT) {
+			errno = EOPNOTSUPP;
+			return -1;
+		}
+	}
+
 	/* We are updating an existing set with new elements, check if the new
 	 * interval overlaps with any of the existing ones.
 	 */
-- 
2.11.0

Re: [PATCH] segtree: bail out on concatenations

[Stefano Brivio]

Hi,

On Thu,  2 Apr 2020 23:49:41 +0200
Pablo Neira Ayuso <pablo@netfilter.org> 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
> 
> Reported-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

I know you both reported this to me, sorry, I still have to polish up
the actual fix before posting it. I'm not very familiar with this code
yet, and it's taking ages.

It might be a few more days before I get to it, so I guess this patch
might make sense for the moment being.

-- 
Stefano

Re: [PATCH] segtree: bail out on concatenations

[Pablo Neira Ayuso]

Hi,

On Fri, Apr 03, 2020 at 02:54:53AM +0200, Stefano Brivio wrote:
> Hi,
> 
> On Thu,  2 Apr 2020 23:49:41 +0200
> Pablo Neira Ayuso <pablo@netfilter.org> 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
> > 
> > Reported-by: Florian Westphal <fw@strlen.de>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> I know you both reported this to me, sorry, I still have to polish up
> the actual fix before posting it. I'm not very familiar with this code
> yet, and it's taking ages.
> 
> It might be a few more days before I get to it, so I guess this patch
> might make sense for the moment being.

I think this one might not be worth to look further. This only happens
with old kernel and new nft binary.

After adding the set, for instance:

table x {
        set y {
                type ipv4_addr . ipv4_addr
                flags interval
        }
}

Old kernels accept this because the kernel has no concept of
concatenation, it's just a number of bytes. Then, the flag interval
selects the rbtree in the kernel.

Then, when listing back the set to userspace, the old kernel reports
no concatenation description, so nft userspace enters the segtree path
to deal with this concatenation.

I think the check in this patch is fine to report users that this
kernel is old and that adding interval concatenation is not supported
by this kernel.

Not related to this patch, Phil reported this one is still broken:

        ip daddr . tcp dport { 10.0.0.0/8 . 10-23, 192.168.1.1-192.168.3.8 . 80-443 } accept

Thanks.

Re: [PATCH] segtree: bail out on concatenations

[Stefano Brivio]

On Fri, 3 Apr 2020 12:39:54 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> Hi,
> 
> On Fri, Apr 03, 2020 at 02:54:53AM +0200, Stefano Brivio wrote:
> > Hi,
> > 
> > On Thu,  2 Apr 2020 23:49:41 +0200
> > Pablo Neira Ayuso <pablo@netfilter.org> 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
> > > 
> > > Reported-by: Florian Westphal <fw@strlen.de>
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>  
> > 
> > I know you both reported this to me, sorry, I still have to polish up
> > the actual fix before posting it. I'm not very familiar with this code
> > yet, and it's taking ages.
> > 
> > It might be a few more days before I get to it, so I guess this patch
> > might make sense for the moment being.  
> 
> I think this one might not be worth to look further. This only happens
> with old kernel and new nft binary.
>
> [...]
>
> Not related to this patch, Phil reported this one is still broken:
> 
>         ip daddr . tcp dport { 10.0.0.0/8 . 10-23, 192.168.1.1-192.168.3.8 . 80-443 } accept

Grrr, yes, I mixed up the two problems, and it was you and Phil, not
Florian, reporting this.

This is what my message really was about, sorry for the confusion.

-- 
Stefano

Re: [PATCH] segtree: bail out on concatenations

[Pablo Neira Ayuso]

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

Re: [PATCH] segtree: bail out on concatenations

[Pablo Neira Ayuso]

On Fri, Apr 03, 2020 at 02:03:51PM +0200, Pablo Neira Ayuso wrote:
> 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.

... with this flag. *sigh*

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

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

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

Re: [PATCH] segtree: bail out on concatenations

[Stefano Brivio]

On Fri, 3 Apr 2020 14:03:51 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

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

First off, sorry, it didn't occur to me to run new tests on older
kernels. :/

I can't quickly run that on some older kernel right now. For my
understanding, where is it crashing?

-- 
Stefano

Re: [PATCH] segtree: bail out on concatenations

[Pablo Neira Ayuso]

On Fri, Apr 03, 2020 at 02:27:05PM +0200, Stefano Brivio wrote:
> On Fri, 3 Apr 2020 14:03:51 +0200
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> > 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
> >         }
> > }
> 
> First off, sorry, it didn't occur to me to run new tests on older
> kernels. :/
> 
> I can't quickly run that on some older kernel right now. For my
> understanding, where is it crashing?

When listing via

        nft list ruleset

The segtree does not know how to handle this concatenation.

The only way I found to prevent this error is to bail out when adding
the set, ie. old kernel checks that NFT_SET_CONCAT is not supported,
hence it bails out.

I'm going to prepare patches to submit this to nf.git.

Re: [PATCH] segtree: bail out on concatenations

[Stefano Brivio]

On Fri, 3 Apr 2020 14:50:29 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Fri, Apr 03, 2020 at 02:27:05PM +0200, Stefano Brivio wrote:
> > On Fri, 3 Apr 2020 14:03:51 +0200
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >   
> > > 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
> > >         }
> > > }  
> > 
> > First off, sorry, it didn't occur to me to run new tests on older
> > kernels. :/
> > 
> > I can't quickly run that on some older kernel right now. For my
> > understanding, where is it crashing?  
> 
> When listing via
> 
>         nft list ruleset
> 
> The segtree does not know how to handle this concatenation.

Ouch, I see.

> The only way I found to prevent this error is to bail out when adding
> the set, ie. old kernel checks that NFT_SET_CONCAT is not supported,
> hence it bails out.

Right, it took me a while to realise you're referring to the flag that
was present until v2 and I dropped in v3 (gold medal goes to truly
yours -- I actually proposed that :/).

> I'm going to prepare patches to submit this to nf.git.

I'm still wondering if we can come up with a userspace-only fix by
understanding kernel support level somehow (not via flags) and refuse
to add that type of set. I'm still checking.

If we let nft add that type of set with rbtree, it's already a bug, so
avoiding a crash on listing is actually worse than the current
situation.

-- 
Stefano

Re: [PATCH] segtree: bail out on concatenations

[Stefano Brivio]

On Fri, 3 Apr 2020 15:33:31 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Fri, 3 Apr 2020 14:50:29 +0200
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> > On Fri, Apr 03, 2020 at 02:27:05PM +0200, Stefano Brivio wrote:  
> > > On Fri, 3 Apr 2020 14:03:51 +0200
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >     
> > > > 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
> > > >         }
> > > > }    
> > > 
> > > First off, sorry, it didn't occur to me to run new tests on older
> > > kernels. :/
> > > 
> > > I can't quickly run that on some older kernel right now. For my
> > > understanding, where is it crashing?    
> > 
> > When listing via
> > 
> >         nft list ruleset
> > 
> > The segtree does not know how to handle this concatenation.  
> 
> Ouch, I see.
> 
> > The only way I found to prevent this error is to bail out when adding
> > the set, ie. old kernel checks that NFT_SET_CONCAT is not supported,
> > hence it bails out.  
> 
> Right, it took me a while to realise you're referring to the flag that
> was present until v2 and I dropped in v3 (gold medal goes to truly
> yours -- I actually proposed that :/).
> 
> > I'm going to prepare patches to submit this to nf.git.  
> 
> I'm still wondering if we can come up with a userspace-only fix by
> understanding kernel support level somehow (not via flags) and refuse
> to add that type of set. I'm still checking.

Nope, sorry, I can't think of anything better than what you already
proposed.

-- 
Stefano