[PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges

[PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges

[Stefano Brivio]

As reported by both Pablo and Phil, trying to add an anonymous set
containing a concatenated range would fail:

  # nft add rule x y ip saddr . tcp dport { 192.168.2.1 . 20-30 } accept
  BUG: invalid range expression type concat
  nft: expression.c:1160: range_expr_value_low: Assertion `0' failed.
  Aborted

  # nft add rule t c ip daddr . tcp dport '{ 10.0.0.0/8 . 10-23, 192.168.1.1-192.168.3.8 . 80-443 } accept'
  BUG: invalid range expression type concat
  nft: expression.c:1296: range_expr_value_low: Assertion `0' failed.

Patch 1/2 fixes this, and 2/2 adds a simple test for it.

Stefano Brivio (2):
  evaluate: Perform set evaluation on implicitly declared (anonymous)
    sets
  tests: shell: Introduce test for concatenated ranges in anonymous sets

 src/evaluate.c                                             | 5 ++++-
 tests/shell/testcases/sets/0048anonymous_set_concat_0      | 7 +++++++
 .../testcases/sets/dumps/0048anonymous_set_concat_0.nft    | 6 ++++++
 3 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100755 tests/shell/testcases/sets/0048anonymous_set_concat_0
 create mode 100644 tests/shell/testcases/sets/dumps/0048anonymous_set_concat_0.nft

-- 
2.26.2

[PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets

[Stefano Brivio]

If a set is implicitly declared, set_evaluate() is not called as a
result of cmd_evaluate_add(), because we're adding in fact something
else (e.g. a rule). Expression-wise, evaluation still happens as the
implicit set expression is eventually found in the tree and handled
by expr_evaluate_set(), but context-wise evaluation (set_evaluate())
is skipped, and this might be relevant instead.

This is visible in the reported case of an anonymous set including
concatenated ranges:

  # nft add rule t c ip saddr . tcp dport { 192.0.2.1 . 20-30 } accept
  BUG: invalid range expression type concat
  nft: expression.c:1160: range_expr_value_low: Assertion `0' failed.
  Aborted

because we reach do_add_set() without properly evaluated flags and
set description, and eventually end up in expr_to_intervals(), which
can't handle that expression.

Explicitly call set_evaluate() as we add anonymous sets into the
context, and instruct the same function to skip expression-wise set
evaluation if the set is anonymous, as that happens later anyway as
part of the general tree evaluation.

Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
Reported-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 src/evaluate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 506f2c6a257e..ee019bc98480 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -76,6 +76,7 @@ static void key_fix_dtype_byteorder(struct expr *key)
 	datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
 }
 
+static int set_evaluate(struct eval_ctx *ctx, struct set *set);
 static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
 					     const char *name,
 					     struct expr *key,
@@ -107,6 +108,8 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
 		list_add_tail(&cmd->list, &ctx->cmd->list);
 	}
 
+	set_evaluate(ctx, set);
+
 	return set_ref_expr_alloc(&expr->location, set);
 }
 
@@ -3547,7 +3550,7 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 	}
 
 	ctx->set = set;
-	if (set->init != NULL) {
+	if (!set_is_anonymous(set->flags) && set->init != NULL) {
 		__expr_set_context(&ctx->ectx, set->key->dtype,
 				   set->key->byteorder, set->key->len, 0);
 		if (expr_evaluate(ctx, &set->init) < 0)
-- 
2.26.2

[PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets

[Stefano Brivio]

Add a simple anonymous set including a concatenated range and check
it's inserted correctly. This is roughly based on the existing
0025_anonymous_set_0 test case.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tests/shell/testcases/sets/0048anonymous_set_concat_0      | 7 +++++++
 .../testcases/sets/dumps/0048anonymous_set_concat_0.nft    | 6 ++++++
 2 files changed, 13 insertions(+)
 create mode 100755 tests/shell/testcases/sets/0048anonymous_set_concat_0
 create mode 100644 tests/shell/testcases/sets/dumps/0048anonymous_set_concat_0.nft

diff --git a/tests/shell/testcases/sets/0048anonymous_set_concat_0 b/tests/shell/testcases/sets/0048anonymous_set_concat_0
new file mode 100755
index 000000000000..fab61231d0c0
--- /dev/null
+++ b/tests/shell/testcases/sets/0048anonymous_set_concat_0
@@ -0,0 +1,7 @@
+#!/bin/sh -e
+#
+# 0048anonymous_sets_concat_0 - Anonymous sets with concatenated ranges
+
+${NFT} add table t
+${NFT} add chain t c '{ type filter hook forward priority 0 ; }'
+${NFT} add rule t c 'ip daddr . tcp dport { 192.0.2.1 . 49152-65535 }'
diff --git a/tests/shell/testcases/sets/dumps/0048anonymous_set_concat_0.nft b/tests/shell/testcases/sets/dumps/0048anonymous_set_concat_0.nft
new file mode 100644
index 000000000000..c54ffae9d6d2
--- /dev/null
+++ b/tests/shell/testcases/sets/dumps/0048anonymous_set_concat_0.nft
@@ -0,0 +1,6 @@
+table ip t {
+	chain c {
+		type filter hook forward priority filter; policy accept;
+		ip daddr . tcp dport { 192.0.2.1 . 49152-65535 }
+	}
+}
-- 
2.26.2

Re: [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges

[Phil Sutter]

Hi Stefano,

On Sun, May 24, 2020 at 03:00:25PM +0200, Stefano Brivio wrote:
> As reported by both Pablo and Phil, trying to add an anonymous set
> containing a concatenated range would fail:

Thanks for getting back at this. You may consider enabling the
corresponding line in tests/py/inet/sets.t (leading dashes disable a
test in that suite).

Cheers, Phil

Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets

[Phil Sutter]

On Sun, May 24, 2020 at 03:00:26PM +0200, Stefano Brivio wrote:
> If a set is implicitly declared, set_evaluate() is not called as a
> result of cmd_evaluate_add(), because we're adding in fact something
> else (e.g. a rule). Expression-wise, evaluation still happens as the
> implicit set expression is eventually found in the tree and handled
> by expr_evaluate_set(), but context-wise evaluation (set_evaluate())
> is skipped, and this might be relevant instead.
> 
> This is visible in the reported case of an anonymous set including
> concatenated ranges:
> 
>   # nft add rule t c ip saddr . tcp dport { 192.0.2.1 . 20-30 } accept
>   BUG: invalid range expression type concat
>   nft: expression.c:1160: range_expr_value_low: Assertion `0' failed.
>   Aborted
> 
> because we reach do_add_set() without properly evaluated flags and
> set description, and eventually end up in expr_to_intervals(), which
> can't handle that expression.
> 
> Explicitly call set_evaluate() as we add anonymous sets into the
> context, and instruct the same function to skip expression-wise set
> evaluation if the set is anonymous, as that happens later anyway as
> part of the general tree evaluation.
> 
> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Reported-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Acked-by: Phil Sutter <phil@nwl.cc>

Re: [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets

[Phil Sutter]

On Sun, May 24, 2020 at 03:00:27PM +0200, Stefano Brivio wrote:
> Add a simple anonymous set including a concatenated range and check
> it's inserted correctly. This is roughly based on the existing
> 0025_anonymous_set_0 test case.

I think this is pretty much redundant to what tests/py/inet/sets.t tests
if you simply enable the anonymous set rule I added in commit
64b9aa3803dd1 ("tests/py: Add tests involving concatenated ranges").

Cheers, Phil

Re: [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets

[Stefano Brivio]

On Mon, 25 May 2020 17:48:34 +0200
Phil Sutter <phil@nwl.cc> wrote:

> On Sun, May 24, 2020 at 03:00:27PM +0200, Stefano Brivio wrote:
> > Add a simple anonymous set including a concatenated range and check
> > it's inserted correctly. This is roughly based on the existing
> > 0025_anonymous_set_0 test case.  
> 
> I think this is pretty much redundant to what tests/py/inet/sets.t tests
> if you simply enable the anonymous set rule I added in commit
> 64b9aa3803dd1 ("tests/py: Add tests involving concatenated ranges").

Nice, I wasn't aware of that one. Anyway, this isn't really redundant
as it also checks that sets are reported back correctly (which I
expected to break, even if it didn't) by comparing with the dump file,
instead of just checking netlink messages.

So I'd actually suggest that we keep this and I'd send another patch
(should I repost this series? A separate patch?) to enable the rule you
added for py tests.

-- 
Stefano

Re: [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets

[Phil Sutter]

Hi,

On Tue, May 26, 2020 at 01:12:47AM +0200, Stefano Brivio wrote:
> On Mon, 25 May 2020 17:48:34 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > On Sun, May 24, 2020 at 03:00:27PM +0200, Stefano Brivio wrote:
> > > Add a simple anonymous set including a concatenated range and check
> > > it's inserted correctly. This is roughly based on the existing
> > > 0025_anonymous_set_0 test case.  
> > 
> > I think this is pretty much redundant to what tests/py/inet/sets.t tests
> > if you simply enable the anonymous set rule I added in commit
> > 64b9aa3803dd1 ("tests/py: Add tests involving concatenated ranges").
> 
> Nice, I wasn't aware of that one. Anyway, this isn't really redundant
> as it also checks that sets are reported back correctly (which I
> expected to break, even if it didn't) by comparing with the dump file,
> instead of just checking netlink messages.
> 
> So I'd actually suggest that we keep this and I'd send another patch
> (should I repost this series? A separate patch?) to enable the rule you
> added for py tests.

But nft-test.py does check ruleset listing, that's what the optional
third part of a rule line is for. The syntax is roughly:

| <rule>;(fail|ok[;<rule_out>])

It allows us to cover for asymmetric rule listings. A simple example
from any/ct.t is:

| ct mark or 0x23 == 0x11;ok;ct mark | 0x00000023 == 0x00000011

So nft reports mark values with leading zeroes (don't ask me why ;).

Am I missing some extra your test does?

Cheers, Phil

Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets

[Pablo Neira Ayuso]

On Sun, May 24, 2020 at 03:00:26PM +0200, Stefano Brivio wrote:
> If a set is implicitly declared, set_evaluate() is not called as a
> result of cmd_evaluate_add(), because we're adding in fact something
> else (e.g. a rule). Expression-wise, evaluation still happens as the
> implicit set expression is eventually found in the tree and handled
> by expr_evaluate_set(), but context-wise evaluation (set_evaluate())
> is skipped, and this might be relevant instead.
> 
> This is visible in the reported case of an anonymous set including
> concatenated ranges:
> 
>   # nft add rule t c ip saddr . tcp dport { 192.0.2.1 . 20-30 } accept
>   BUG: invalid range expression type concat
>   nft: expression.c:1160: range_expr_value_low: Assertion `0' failed.
>   Aborted
> 
> because we reach do_add_set() without properly evaluated flags and
> set description, and eventually end up in expr_to_intervals(), which
> can't handle that expression.
> 
> Explicitly call set_evaluate() as we add anonymous sets into the
> context, and instruct the same function to skip expression-wise set
> evaluation if the set is anonymous, as that happens later anyway as
> part of the general tree evaluation.
> 
> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Reported-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  src/evaluate.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 506f2c6a257e..ee019bc98480 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -76,6 +76,7 @@ static void key_fix_dtype_byteorder(struct expr *key)
>  	datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
>  }
>  
> +static int set_evaluate(struct eval_ctx *ctx, struct set *set);
>  static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
>  					     const char *name,
>  					     struct expr *key,
> @@ -107,6 +108,8 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
>  		list_add_tail(&cmd->list, &ctx->cmd->list);
>  	}
>  
> +	set_evaluate(ctx, set);

Hm, set_evaluate() populates the cache with the anonymous set in this
case, see set_lookup() + sed_add_hash().

Re: [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets

[Stefano Brivio]

On Tue, 26 May 2020 15:39:52 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Hi,
> 
> On Tue, May 26, 2020 at 01:12:47AM +0200, Stefano Brivio wrote:
> > On Mon, 25 May 2020 17:48:34 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> >   
> > > On Sun, May 24, 2020 at 03:00:27PM +0200, Stefano Brivio wrote:  
> > > > Add a simple anonymous set including a concatenated range and check
> > > > it's inserted correctly. This is roughly based on the existing
> > > > 0025_anonymous_set_0 test case.    
> > > 
> > > I think this is pretty much redundant to what tests/py/inet/sets.t tests
> > > if you simply enable the anonymous set rule I added in commit
> > > 64b9aa3803dd1 ("tests/py: Add tests involving concatenated ranges").  
> > 
> > Nice, I wasn't aware of that one. Anyway, this isn't really redundant
> > as it also checks that sets are reported back correctly (which I
> > expected to break, even if it didn't) by comparing with the dump file,
> > instead of just checking netlink messages.
> > 
> > So I'd actually suggest that we keep this and I'd send another patch
> > (should I repost this series? A separate patch?) to enable the rule you
> > added for py tests.  
> 
> But nft-test.py does check ruleset listing, that's what the optional
> third part of a rule line is for. The syntax is roughly:
> 
> | <rule>;(fail|ok[;<rule_out>])
> 
> It allows us to cover for asymmetric rule listings.

Oh, sorry, I didn't realise that... the README actually mentions it
(section C), Line 5, Part 3 of example), but I skipped that part.

> A simple example from any/ct.t is:
> 
> | ct mark or 0x23 == 0x11;ok;ct mark | 0x00000023 == 0x00000011
> 
> So nft reports mark values with leading zeroes (don't ask me why ;).

I guess it's actually neater that way for 32-bit fields :)

> Am I missing some extra your test does?

No, nothing. I'll replace this patch by one that simply enables the
case you already added.

-- 
Stefano

Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets

[Stefano Brivio]

On Tue, 26 May 2020 18:54:16 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Sun, May 24, 2020 at 03:00:26PM +0200, Stefano Brivio wrote:
> > If a set is implicitly declared, set_evaluate() is not called as a
> > result of cmd_evaluate_add(), because we're adding in fact something
> > else (e.g. a rule). Expression-wise, evaluation still happens as the
> > implicit set expression is eventually found in the tree and handled
> > by expr_evaluate_set(), but context-wise evaluation (set_evaluate())
> > is skipped, and this might be relevant instead.
> > 
> > This is visible in the reported case of an anonymous set including
> > concatenated ranges:
> > 
> >   # nft add rule t c ip saddr . tcp dport { 192.0.2.1 . 20-30 } accept
> >   BUG: invalid range expression type concat
> >   nft: expression.c:1160: range_expr_value_low: Assertion `0' failed.
> >   Aborted
> > 
> > because we reach do_add_set() without properly evaluated flags and
> > set description, and eventually end up in expr_to_intervals(), which
> > can't handle that expression.
> > 
> > Explicitly call set_evaluate() as we add anonymous sets into the
> > context, and instruct the same function to skip expression-wise set
> > evaluation if the set is anonymous, as that happens later anyway as
> > part of the general tree evaluation.
> > 
> > Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > Reported-by: Phil Sutter <phil@nwl.cc>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  src/evaluate.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index 506f2c6a257e..ee019bc98480 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -76,6 +76,7 @@ static void key_fix_dtype_byteorder(struct expr *key)
> >  	datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
> >  }
> >  
> > +static int set_evaluate(struct eval_ctx *ctx, struct set *set);
> >  static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
> >  					     const char *name,
> >  					     struct expr *key,
> > @@ -107,6 +108,8 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
> >  		list_add_tail(&cmd->list, &ctx->cmd->list);
> >  	}
> >  
> > +	set_evaluate(ctx, set);  
> 
> Hm, set_evaluate() populates the cache with the anonymous set in this
> case, see set_lookup() + sed_add_hash().

While checking what parts of set_evaluate() we should skip for anonymous
sets, I thought it made sense to keep that, simply because I didn't see
any value in making that a special case. Is the __set* stuff polluting?
Any other bad consequence I missed? Or you would skip that just because
it's useless?

-- 
Stefano

Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets

[Pablo Neira Ayuso]

On Tue, May 26, 2020 at 07:17:25PM +0200, Stefano Brivio wrote:
> On Tue, 26 May 2020 18:54:16 +0200
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> > On Sun, May 24, 2020 at 03:00:26PM +0200, Stefano Brivio wrote:
> > > If a set is implicitly declared, set_evaluate() is not called as a
> > > result of cmd_evaluate_add(), because we're adding in fact something
> > > else (e.g. a rule). Expression-wise, evaluation still happens as the
> > > implicit set expression is eventually found in the tree and handled
> > > by expr_evaluate_set(), but context-wise evaluation (set_evaluate())
> > > is skipped, and this might be relevant instead.
> > > 
> > > This is visible in the reported case of an anonymous set including
> > > concatenated ranges:
> > > 
> > >   # nft add rule t c ip saddr . tcp dport { 192.0.2.1 . 20-30 } accept
> > >   BUG: invalid range expression type concat
> > >   nft: expression.c:1160: range_expr_value_low: Assertion `0' failed.
> > >   Aborted
> > > 
> > > because we reach do_add_set() without properly evaluated flags and
> > > set description, and eventually end up in expr_to_intervals(), which
> > > can't handle that expression.
> > > 
> > > Explicitly call set_evaluate() as we add anonymous sets into the
> > > context, and instruct the same function to skip expression-wise set
> > > evaluation if the set is anonymous, as that happens later anyway as
> > > part of the general tree evaluation.
> > > 
> > > Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > Reported-by: Phil Sutter <phil@nwl.cc>
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > >  src/evaluate.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/evaluate.c b/src/evaluate.c
> > > index 506f2c6a257e..ee019bc98480 100644
> > > --- a/src/evaluate.c
> > > +++ b/src/evaluate.c
> > > @@ -76,6 +76,7 @@ static void key_fix_dtype_byteorder(struct expr *key)
> > >  	datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
> > >  }
> > >  
> > > +static int set_evaluate(struct eval_ctx *ctx, struct set *set);
> > >  static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
> > >  					     const char *name,
> > >  					     struct expr *key,
> > > @@ -107,6 +108,8 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
> > >  		list_add_tail(&cmd->list, &ctx->cmd->list);
> > >  	}
> > >  
> > > +	set_evaluate(ctx, set);  
> > 
> > Hm, set_evaluate() populates the cache with the anonymous set in this
> > case, see set_lookup() + sed_add_hash().
> 
> While checking what parts of set_evaluate() we should skip for anonymous
> sets, I thought it made sense to keep that, simply because I didn't see
> any value in making that a special case. Is the __set* stuff polluting?

Yes, it's just adding a __set%d to the cache.

> Any other bad consequence I missed? Or you would skip that just because
> it's useless?

I did not find any command that triggers any problem right now. I just
think we should not add an anonymous set to the cache.

BTW, are not set->desc.field_len and set->key->field_len duplicated
fields? Same thing with field_count.

Probably it should be possible to simplify this by using
set->key->field* instead? So set_evaluate() is not required to
transfer the fields.

Thanks.

Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets

[Stefano Brivio]

On Tue, 26 May 2020 19:34:19 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Tue, May 26, 2020 at 07:17:25PM +0200, Stefano Brivio wrote:
> > On Tue, 26 May 2020 18:54:16 +0200
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >   
> > > On Sun, May 24, 2020 at 03:00:26PM +0200, Stefano Brivio wrote:  
> > > > If a set is implicitly declared, set_evaluate() is not called as a
> > > > result of cmd_evaluate_add(), because we're adding in fact something
> > > > else (e.g. a rule). Expression-wise, evaluation still happens as the
> > > > implicit set expression is eventually found in the tree and handled
> > > > by expr_evaluate_set(), but context-wise evaluation (set_evaluate())
> > > > is skipped, and this might be relevant instead.
> > > > 
> > > > This is visible in the reported case of an anonymous set including
> > > > concatenated ranges:
> > > > 
> > > >   # nft add rule t c ip saddr . tcp dport { 192.0.2.1 . 20-30 } accept
> > > >   BUG: invalid range expression type concat
> > > >   nft: expression.c:1160: range_expr_value_low: Assertion `0' failed.
> > > >   Aborted
> > > > 
> > > > because we reach do_add_set() without properly evaluated flags and
> > > > set description, and eventually end up in expr_to_intervals(), which
> > > > can't handle that expression.
> > > > 
> > > > Explicitly call set_evaluate() as we add anonymous sets into the
> > > > context, and instruct the same function to skip expression-wise set
> > > > evaluation if the set is anonymous, as that happens later anyway as
> > > > part of the general tree evaluation.
> > > > 
> > > > Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > Reported-by: Phil Sutter <phil@nwl.cc>
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > > ---
> > > >  src/evaluate.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/evaluate.c b/src/evaluate.c
> > > > index 506f2c6a257e..ee019bc98480 100644
> > > > --- a/src/evaluate.c
> > > > +++ b/src/evaluate.c
> > > > @@ -76,6 +76,7 @@ static void key_fix_dtype_byteorder(struct expr *key)
> > > >  	datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
> > > >  }
> > > >  
> > > > +static int set_evaluate(struct eval_ctx *ctx, struct set *set);
> > > >  static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
> > > >  					     const char *name,
> > > >  					     struct expr *key,
> > > > @@ -107,6 +108,8 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
> > > >  		list_add_tail(&cmd->list, &ctx->cmd->list);
> > > >  	}
> > > >  
> > > > +	set_evaluate(ctx, set);    
> > > 
> > > Hm, set_evaluate() populates the cache with the anonymous set in this
> > > case, see set_lookup() + sed_add_hash().  
> > 
> > While checking what parts of set_evaluate() we should skip for anonymous
> > sets, I thought it made sense to keep that, simply because I didn't see
> > any value in making that a special case. Is the __set* stuff polluting?  
> 
> Yes, it's just adding a __set%d to the cache.
> 
> > Any other bad consequence I missed? Or you would skip that just because
> > it's useless?  
> 
> I did not find any command that triggers any problem right now. I just
> think we should not add an anonymous set to the cache.

Okay, I see.

> BTW, are not set->desc.field_len and set->key->field_len duplicated
> fields? Same thing with field_count.

Yes, they are, but:

> Probably it should be possible to simplify this by using
> set->key->field* instead? So set_evaluate() is not required to
> transfer the fields.

...even if we use those, we still need to call expr_evaluate_concat()
(with the same logic as implemented by set_evaluate()) to fill the
set->key fields in.

Conceptually, I think that set_evaluate() should apply just in the same
way no matter how sets are created, minus expression evaluation and
caching. Taking selecting bits out looks a bit fragile/inconsistent to
me. Maybe I'm biased by the fact it was relatively complicated for me
to narrow down this particular issue.

-- 
Stefano

Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets

[Pablo Neira Ayuso]

On Tue, May 26, 2020 at 08:01:21PM +0200, Stefano Brivio wrote:
> On Tue, 26 May 2020 19:34:19 +0200
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> > On Tue, May 26, 2020 at 07:17:25PM +0200, Stefano Brivio wrote:
> > > On Tue, 26 May 2020 18:54:16 +0200
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >   
> > > > On Sun, May 24, 2020 at 03:00:26PM +0200, Stefano Brivio wrote:  
> > > > > If a set is implicitly declared, set_evaluate() is not called as a
> > > > > result of cmd_evaluate_add(), because we're adding in fact something
> > > > > else (e.g. a rule). Expression-wise, evaluation still happens as the
> > > > > implicit set expression is eventually found in the tree and handled
> > > > > by expr_evaluate_set(), but context-wise evaluation (set_evaluate())
> > > > > is skipped, and this might be relevant instead.
> > > > > 
> > > > > This is visible in the reported case of an anonymous set including
> > > > > concatenated ranges:
> > > > > 
> > > > >   # nft add rule t c ip saddr . tcp dport { 192.0.2.1 . 20-30 } accept
> > > > >   BUG: invalid range expression type concat
> > > > >   nft: expression.c:1160: range_expr_value_low: Assertion `0' failed.
> > > > >   Aborted
> > > > > 
> > > > > because we reach do_add_set() without properly evaluated flags and
> > > > > set description, and eventually end up in expr_to_intervals(), which
> > > > > can't handle that expression.
> > > > > 
> > > > > Explicitly call set_evaluate() as we add anonymous sets into the
> > > > > context, and instruct the same function to skip expression-wise set
> > > > > evaluation if the set is anonymous, as that happens later anyway as
> > > > > part of the general tree evaluation.
> > > > > 
> > > > > Reported-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > > > Reported-by: Phil Sutter <phil@nwl.cc>
> > > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > > > ---
> > > > >  src/evaluate.c | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/src/evaluate.c b/src/evaluate.c
> > > > > index 506f2c6a257e..ee019bc98480 100644
> > > > > --- a/src/evaluate.c
> > > > > +++ b/src/evaluate.c
> > > > > @@ -76,6 +76,7 @@ static void key_fix_dtype_byteorder(struct expr *key)
> > > > >  	datatype_set(key, set_datatype_alloc(dtype, key->byteorder));
> > > > >  }
> > > > >  
> > > > > +static int set_evaluate(struct eval_ctx *ctx, struct set *set);
> > > > >  static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
> > > > >  					     const char *name,
> > > > >  					     struct expr *key,
> > > > > @@ -107,6 +108,8 @@ static struct expr *implicit_set_declaration(struct eval_ctx *ctx,
> > > > >  		list_add_tail(&cmd->list, &ctx->cmd->list);
> > > > >  	}
> > > > >  
> > > > > +	set_evaluate(ctx, set);    
> > > > 
> > > > Hm, set_evaluate() populates the cache with the anonymous set in this
> > > > case, see set_lookup() + sed_add_hash().  
> > > 
> > > While checking what parts of set_evaluate() we should skip for anonymous
> > > sets, I thought it made sense to keep that, simply because I didn't see
> > > any value in making that a special case. Is the __set* stuff polluting?  
> > 
> > Yes, it's just adding a __set%d to the cache.
> > 
> > > Any other bad consequence I missed? Or you would skip that just because
> > > it's useless?  
> > 
> > I did not find any command that triggers any problem right now. I just
> > think we should not add an anonymous set to the cache.
> 
> Okay, I see.
> 
> > BTW, are not set->desc.field_len and set->key->field_len duplicated
> > fields? Same thing with field_count.
> 
> Yes, they are, but:
> 
> > Probably it should be possible to simplify this by using
> > set->key->field* instead? So set_evaluate() is not required to
> > transfer the fields.
> 
> ...even if we use those, we still need to call expr_evaluate_concat()
> (with the same logic as implemented by set_evaluate()) to fill the
> set->key fields in.
> 
> Conceptually, I think that set_evaluate() should apply just in the same
> way no matter how sets are created, minus expression evaluation and
> caching. Taking selecting bits out looks a bit fragile/inconsistent to
> me. Maybe I'm biased by the fact it was relatively complicated for me
> to narrow down this particular issue.

OK, let's just not add this anonymous set to the cache.

There is also a test line in tests/py that needs to be turned on that
Phil mentioned (no need for new tests/shell file).

There is a memleak kicking in a few tests after this patch. Turn on
ASAN and run tests/shell/ to catch it.

So re-spin and send v2, thanks.