* [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges @ 2020-05-24 13:00 Stefano Brivio 2020-05-24 13:00 ` [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets Stefano Brivio ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Stefano Brivio @ 2020-05-24 13:00 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Phil Sutter, netfilter-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets 2020-05-24 13:00 [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges Stefano Brivio @ 2020-05-24 13:00 ` Stefano Brivio 2020-05-25 15:46 ` Phil Sutter 2020-05-26 16:54 ` Pablo Neira Ayuso 2020-05-24 13:00 ` [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets Stefano Brivio 2020-05-25 15:45 ` [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges Phil Sutter 2 siblings, 2 replies; 14+ messages in thread From: Stefano Brivio @ 2020-05-24 13:00 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Phil Sutter, netfilter-devel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets 2020-05-24 13:00 ` [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets Stefano Brivio @ 2020-05-25 15:46 ` Phil Sutter 2020-05-26 16:54 ` Pablo Neira Ayuso 1 sibling, 0 replies; 14+ messages in thread From: Phil Sutter @ 2020-05-25 15:46 UTC (permalink / raw) To: Stefano Brivio; +Cc: Pablo Neira Ayuso, netfilter-devel 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets 2020-05-24 13:00 ` [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets Stefano Brivio 2020-05-25 15:46 ` Phil Sutter @ 2020-05-26 16:54 ` Pablo Neira Ayuso 2020-05-26 17:17 ` Stefano Brivio 1 sibling, 1 reply; 14+ messages in thread From: Pablo Neira Ayuso @ 2020-05-26 16:54 UTC (permalink / raw) To: Stefano Brivio; +Cc: Phil Sutter, netfilter-devel 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(). ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets 2020-05-26 16:54 ` Pablo Neira Ayuso @ 2020-05-26 17:17 ` Stefano Brivio 2020-05-26 17:34 ` Pablo Neira Ayuso 0 siblings, 1 reply; 14+ messages in thread From: Stefano Brivio @ 2020-05-26 17:17 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Phil Sutter, netfilter-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets 2020-05-26 17:17 ` Stefano Brivio @ 2020-05-26 17:34 ` Pablo Neira Ayuso 2020-05-26 18:01 ` Stefano Brivio 0 siblings, 1 reply; 14+ messages in thread From: Pablo Neira Ayuso @ 2020-05-26 17:34 UTC (permalink / raw) To: Stefano Brivio; +Cc: Phil Sutter, netfilter-devel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets 2020-05-26 17:34 ` Pablo Neira Ayuso @ 2020-05-26 18:01 ` Stefano Brivio 2020-05-26 22:04 ` Pablo Neira Ayuso 0 siblings, 1 reply; 14+ messages in thread From: Stefano Brivio @ 2020-05-26 18:01 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Phil Sutter, netfilter-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets 2020-05-26 18:01 ` Stefano Brivio @ 2020-05-26 22:04 ` Pablo Neira Ayuso 0 siblings, 0 replies; 14+ messages in thread From: Pablo Neira Ayuso @ 2020-05-26 22:04 UTC (permalink / raw) To: Stefano Brivio; +Cc: Phil Sutter, netfilter-devel 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets 2020-05-24 13:00 [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges Stefano Brivio 2020-05-24 13:00 ` [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets Stefano Brivio @ 2020-05-24 13:00 ` Stefano Brivio 2020-05-25 15:48 ` Phil Sutter 2020-05-25 15:45 ` [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges Phil Sutter 2 siblings, 1 reply; 14+ messages in thread From: Stefano Brivio @ 2020-05-24 13:00 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Phil Sutter, netfilter-devel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets 2020-05-24 13:00 ` [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets Stefano Brivio @ 2020-05-25 15:48 ` Phil Sutter 2020-05-25 23:12 ` Stefano Brivio 0 siblings, 1 reply; 14+ messages in thread From: Phil Sutter @ 2020-05-25 15:48 UTC (permalink / raw) To: Stefano Brivio; +Cc: Pablo Neira Ayuso, netfilter-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets 2020-05-25 15:48 ` Phil Sutter @ 2020-05-25 23:12 ` Stefano Brivio 2020-05-26 13:39 ` Phil Sutter 0 siblings, 1 reply; 14+ messages in thread From: Stefano Brivio @ 2020-05-25 23:12 UTC (permalink / raw) To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets 2020-05-25 23:12 ` Stefano Brivio @ 2020-05-26 13:39 ` Phil Sutter 2020-05-26 17:17 ` Stefano Brivio 0 siblings, 1 reply; 14+ messages in thread From: Phil Sutter @ 2020-05-26 13:39 UTC (permalink / raw) To: Stefano Brivio; +Cc: Pablo Neira Ayuso, netfilter-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets 2020-05-26 13:39 ` Phil Sutter @ 2020-05-26 17:17 ` Stefano Brivio 0 siblings, 0 replies; 14+ messages in thread From: Stefano Brivio @ 2020-05-26 17:17 UTC (permalink / raw) To: Phil Sutter; +Cc: Pablo Neira Ayuso, netfilter-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges 2020-05-24 13:00 [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges Stefano Brivio 2020-05-24 13:00 ` [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets Stefano Brivio 2020-05-24 13:00 ` [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets Stefano Brivio @ 2020-05-25 15:45 ` Phil Sutter 2 siblings, 0 replies; 14+ messages in thread From: Phil Sutter @ 2020-05-25 15:45 UTC (permalink / raw) To: Stefano Brivio; +Cc: Pablo Neira Ayuso, netfilter-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-05-26 22:05 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-24 13:00 [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges Stefano Brivio 2020-05-24 13:00 ` [PATCH nft 1/2] evaluate: Perform set evaluation on implicitly declared (anonymous) sets Stefano Brivio 2020-05-25 15:46 ` Phil Sutter 2020-05-26 16:54 ` Pablo Neira Ayuso 2020-05-26 17:17 ` Stefano Brivio 2020-05-26 17:34 ` Pablo Neira Ayuso 2020-05-26 18:01 ` Stefano Brivio 2020-05-26 22:04 ` Pablo Neira Ayuso 2020-05-24 13:00 ` [PATCH nft 2/2] tests: shell: Introduce test for concatenated ranges in anonymous sets Stefano Brivio 2020-05-25 15:48 ` Phil Sutter 2020-05-25 23:12 ` Stefano Brivio 2020-05-26 13:39 ` Phil Sutter 2020-05-26 17:17 ` Stefano Brivio 2020-05-25 15:45 ` [PATCH nft 0/2] Fix evaluation of anonymous sets with concatenated ranges Phil Sutter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).