[PATCH nft] evaluate: disallow anonymous set with empty elements

[PATCH nft] evaluate: disallow anonymous set with empty elements

[Pablo Neira Ayuso]

Restrict this, the brackets have explicit semantics since they tell the
kernel to represent this value as a set, which is too costly. Set for
one single element are overkill.

 # nft add rule x y ct state { established } counter
 Error: anonymous set with single element makes no sense, remove brackets wrapping this value
 add rule x y ct state { established } counter
                       ^^^^^^^^^^^^^^^

Instead, the preferred way to express this is:

 # nft add rule x y ct state established counter

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
I know this may break stuff outthere, but probably it's still early to
fix this. If we keep allowing this and transparently turn this into a
value, people will likely never understand the bracket semantics.
Brackets are not just syntaxic sugar.

 src/evaluate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/evaluate.c b/src/evaluate.c
index 3a3f2468c826..aa42c69127a2 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1253,8 +1253,11 @@ static int expr_evaluate_set_elem(struct eval_ctx *ctx, struct expr **expr)
 static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr)
 {
 	struct expr *set = *expr, *i, *next;
+	unsigned int count = 0;
 
 	list_for_each_entry_safe(i, next, &set->expressions, list) {
+		count++;
+
 		if (list_member_evaluate(ctx, &i) < 0)
 			return -1;
 
@@ -1288,6 +1291,11 @@ static int expr_evaluate_set(struct eval_ctx *ctx, struct expr **expr)
 			set->set_flags |= NFT_SET_INTERVAL;
 	}
 
+	if (set->flags & NFT_SET_ANONYMOUS &&
+	    count == 1)
+		return expr_error(ctx->msgs, set,
+				  "anonymous set with one single element makes no sense, remove brackets wrapping this value");
+
 	set->set_flags |= NFT_SET_CONSTANT;
 
 	set->dtype = ctx->ectx.dtype;
-- 
2.11.0

Re: [PATCH nft] evaluate: disallow anonymous set with empty elements

[Pablo Neira Ayuso]

On Tue, Apr 09, 2019 at 12:59:36PM +0200, Pablo Neira Ayuso wrote:
> Restrict this, the brackets have explicit semantics since they tell the
> kernel to represent this value as a set, which is too costly. Set for
> one single element are overkill.
> 
>  # nft add rule x y ct state { established } counter
>  Error: anonymous set with single element makes no sense, remove brackets wrapping this value
>  add rule x y ct state { established } counter
>                        ^^^^^^^^^^^^^^^
> 
> Instead, the preferred way to express this is:
> 
>  # nft add rule x y ct state established counter
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> I know this may break stuff outthere, but probably it's still early to
> fix this. If we keep allowing this and transparently turn this into a
> value, people will likely never understand the bracket semantics.
> Brackets are not just syntaxic sugar.

This patch is broken, will send v2, and I need to adapt tests after
this change. It's a bit of work but it is doable.

We also need a way not to fail on "define" (definition) that allows a
set with one single element, since this can be used from literal sets.
Will require a bit more code.

Re: [PATCH nft] evaluate: disallow anonymous set with empty elements

[Phil Sutter]

Hi Pablo,

On Tue, Apr 09, 2019 at 12:59:36PM +0200, Pablo Neira Ayuso wrote:
> Restrict this, the brackets have explicit semantics since they tell the
> kernel to represent this value as a set, which is too costly. Set for
> one single element are overkill.
> 
>  # nft add rule x y ct state { established } counter
>  Error: anonymous set with single element makes no sense, remove brackets wrapping this value
>  add rule x y ct state { established } counter
>                        ^^^^^^^^^^^^^^^
> 
> Instead, the preferred way to express this is:
> 
>  # nft add rule x y ct state established counter
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> I know this may break stuff outthere, but probably it's still early to
> fix this. If we keep allowing this and transparently turn this into a
> value, people will likely never understand the bracket semantics.
> Brackets are not just syntaxic sugar.

Your point makes sense, understanding that within a rule curly braces
are not a block delimiter but a set definition is key to getting along
with nft syntax.

OTOH I like how we radically optimize anonymous sets. This allows to
have rather "dumb" scripts and get by without a performance penalty.

Could we maybe find a middle ground where nft still does these
optimizations but prints warnings so users are notified? We might even
introduce -W flag to customize behaviour (-W all (default), -W error
(strict mode), -W none (suppress any non-fatal output on stderr)).

Just an idea, not sure if feasible.

Cheers, Phil

Re: [PATCH nft] evaluate: disallow anonymous set with empty elements

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Could we maybe find a middle ground where nft still does these
> optimizations but prints warnings so users are notified? We might even
> introduce -W flag to customize behaviour (-W all (default), -W error
> (strict mode), -W none (suppress any non-fatal output on stderr)).

I like this proposal.

One of the broken tproxy test cases (it prints warning) does this:

ip daddr 0.0.0.0/0

.. and that is always true and could be removed.
Different "problem" of course, but it shows that there is ample
opportunity for pruning irrelevant expressions.

And breaking scripts every time we decide that something is
"silly" is a bad decision, imo.

I suspect users will complain about { 1.2.3.4 } being illegal
"just because".

Re: [PATCH nft] evaluate: disallow anonymous set with empty elements

[Pablo Neira Ayuso]

On Tue, Apr 09, 2019 at 04:03:26PM +0200, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > Could we maybe find a middle ground where nft still does these
> > optimizations but prints warnings so users are notified? We might even
> > introduce -W flag to customize behaviour (-W all (default), -W error
> > (strict mode), -W none (suppress any non-fatal output on stderr)).
> 
> I like this proposal.
> 
> One of the broken tproxy test cases (it prints warning) does this:
> 
> ip daddr 0.0.0.0/0

Yes, sorry, that's my fault.

> .. and that is always true and could be removed.
> Different "problem" of course, but it shows that there is ample
> opportunity for pruning irrelevant expressions.
> 
> And breaking scripts every time we decide that something is
> "silly" is a bad decision, imo.

Agreed, this case is slightly bit corner case as they should _not_ be
doing enclosing single element in brackets in their scripts. But I get
your point, better adopt a more conservative approach ;-)

> I suspect users will complain about { 1.2.3.4 } being illegal
> "just because".

I'll explore the warning idea, it can be an initial step before we can
fully disallow this, so users don't complain about sudden breakage :-)

Thanks for your feedback!

Re: [PATCH nft] evaluate: disallow anonymous set with empty elements

[Phil Sutter]

Hi,

On Wed, Apr 10, 2019 at 01:19:25AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Apr 09, 2019 at 04:03:26PM +0200, Florian Westphal wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > Could we maybe find a middle ground where nft still does these
> > > optimizations but prints warnings so users are notified? We might even
> > > introduce -W flag to customize behaviour (-W all (default), -W error
> > > (strict mode), -W none (suppress any non-fatal output on stderr)).
> > 
> > I like this proposal.
> > 
> > One of the broken tproxy test cases (it prints warning) does this:
> > 
> > ip daddr 0.0.0.0/0
> 
> Yes, sorry, that's my fault.
> 
> > .. and that is always true and could be removed.
> > Different "problem" of course, but it shows that there is ample
> > opportunity for pruning irrelevant expressions.
> > 
> > And breaking scripts every time we decide that something is
> > "silly" is a bad decision, imo.
> 
> Agreed, this case is slightly bit corner case as they should _not_ be
> doing enclosing single element in brackets in their scripts. But I get
> your point, better adopt a more conservative approach ;-)
> 
> > I suspect users will complain about { 1.2.3.4 } being illegal
> > "just because".
> 
> I'll explore the warning idea, it can be an initial step before we can
> fully disallow this, so users don't complain about sudden breakage :-)

What I have in mind is "dumb" scripts collecting addresses and adding a
rule matching them in an anonymous set. The case of just a single address
needs additional code, not just an adjustment of the existing one. This
is not so much a matter of bad design or missing education but one of
effort and feasibility.

Cheers, Phil