[PATCH] nft: Add support for inverted bitwise value list

[PATCH] nft: Add support for inverted bitwise value list

[Laura Garcia Liebana]

Add support for inverted state and status bitwise value list required in the
ct match.

Before this patch, nft didn't support the rule:

$ nft add rule ip filter INPUT ct state != new,related counter accept
<cmdline>:1:41-41: Error: syntax error, unexpected comma, expecting end of file or newline or semicolon
add rule ip filter INPUT ct state != new,related counter accept
                                        ^

This patch includes in the parser the ability to understand a list of
bitwise values.

nft --debug=netlink add rule ip filter INPUT ct state != new,related,established,untracked counter accept
ip filter INPUT
  [ ct load state => reg 1 ]
  [ cmp neq reg 1 0x0000004e ]
  [ counter pkts 0 bytes 0 ]
  [ immediate reg 0 accept ]

In addition, this patch prints the correct rule syntax.

table ip filter {
	chain INPUT {
		ct state != established,related,new,untracked counter packets 0 bytes 0 accept
	}
}

And some tests included:

ct state != new,related;ok
ct status != expected,seen-reply;ok

Signed-off-by: Laura Garcia Liebana <nevola@gmail.com>
---
 src/expression.c   | 12 +++++++++++-
 src/parser_bison.y |  1 +
 tests/py/any/ct.t  |  2 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/expression.c b/src/expression.c
index a10af5d..2ba4d83 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -524,13 +524,23 @@ static bool must_print_eq_op(const struct expr *expr)
 	return expr->left->ops->type == EXPR_BINOP;
 }
 
+static void binop_expr_print_symbol(const struct expr *expr)
+{
+	if (expr->op == OP_OR &&
+	    (expr->right->dtype->type == TYPE_CT_STATE ||
+	    expr->right->dtype->type == TYPE_CT_STATUS))
+		printf(",");
+	else
+		printf(" %s ", expr_op_symbols[expr->op]);
+}
+
 static void binop_expr_print(const struct expr *expr)
 {
 	binop_arg_print(expr, expr->left);
 
 	if (expr_op_symbols[expr->op] &&
 	    (expr->op != OP_EQ || must_print_eq_op(expr)))
-		printf(" %s ", expr_op_symbols[expr->op]);
+		binop_expr_print_symbol(expr);
 	else
 		printf(" ");
 
diff --git a/src/parser_bison.y b/src/parser_bison.y
index d7cba23..02d23b9 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -2097,6 +2097,7 @@ list_rhs_expr		:	basic_rhs_expr		COMMA		basic_rhs_expr
 rhs_expr		:	concat_rhs_expr		{ $$ = $1; }
 			|	multiton_rhs_expr	{ $$ = $1; }
 			|	set_expr		{ $$ = $1; }
+			|       list_rhs_expr           { $$ = $1; }
 			;
 
 shift_rhs_expr		:	primary_rhs_expr
diff --git a/tests/py/any/ct.t b/tests/py/any/ct.t
index 4d13213..4d0273f 100644
--- a/tests/py/any/ct.t
+++ b/tests/py/any/ct.t
@@ -6,6 +6,7 @@
 
 ct state new,established, related, untracked;ok;ct state established,related,new,untracked
 ct state != related;ok
+ct state != new,related;ok
 ct state {new,established, related, untracked};ok
 - ct state != {new,established, related, untracked};ok
 ct state invalid drop;ok
@@ -25,6 +26,7 @@ ct status expected;ok
 ct status != expected;ok
 ct status seen-reply;ok
 ct status != seen-reply;ok
+ct status != expected,seen-reply;ok
 ct status {expected, seen-reply, assured, confirmed, dying};ok
 ct status expected,seen-reply,assured,confirmed,snat,dnat,dying;ok
 ct status snat;ok
-- 
2.7.0

Re: [PATCH] nft: Add support for inverted bitwise value list

[Florian Westphal]

Laura Garcia Liebana <nevola@gmail.com> wrote:
> Add support for inverted state and status bitwise value list required in the
> ct match.
> 
> Before this patch, nft didn't support the rule:
> 
> $ nft add rule ip filter INPUT ct state != new,related counter accept
> <cmdline>:1:41-41: Error: syntax error, unexpected comma, expecting end of file or newline or semicolon
> add rule ip filter INPUT ct state != new,related counter accept
                                         ^
I don't like nft foo,bar syntax since

'state new,related' looks a lot like 'state { new, related }' but its not the same...

Maybe we should use 'state new|related' instead for flag type too?

[ Maybe better discuss it at nfws ]

Re: [PATCH] nft: Add support for inverted bitwise value list

[Jan Engelhardt]

On Wednesday 2016-06-22 18:14, Florian Westphal wrote:
>Laura Garcia Liebana <nevola@gmail.com> wrote:
>> Add support for inverted state and status bitwise value list required in the
>> ct match.
>> 
>> Before this patch, nft didn't support the rule:
>> 
>> $ nft add rule ip filter INPUT ct state != new,related counter accept
>> <cmdline>:1:41-41: Error: syntax error, unexpected comma, expecting end of file or newline or semicolon
>> add rule ip filter INPUT ct state != new,related counter accept
>                                         ^
>I don't like nft foo,bar syntax since
>
>'state new,related' looks a lot like 'state { new, related }' but its not the same...

What is the difference? More specifically, why is there a difference?
That appears to be a bad pitfall for users. (And as such, choosing
different symbols like the pipe symbol does not cure the issue of 
confusion.)

Re: [PATCH] nft: Add support for inverted bitwise value list

[Florian Westphal]

Jan Engelhardt <jengelh@inai.de> wrote:
> 
> On Wednesday 2016-06-22 18:14, Florian Westphal wrote:
> >Laura Garcia Liebana <nevola@gmail.com> wrote:
> >> Add support for inverted state and status bitwise value list required in the
> >> ct match.
> >> 
> >> Before this patch, nft didn't support the rule:
> >> 
> >> $ nft add rule ip filter INPUT ct state != new,related counter accept
> >> <cmdline>:1:41-41: Error: syntax error, unexpected comma, expecting end of file or newline or semicolon
> >> add rule ip filter INPUT ct state != new,related counter accept
> >                                         ^
> >I don't like nft foo,bar syntax since
> >
> >'state new,related' looks a lot like 'state { new, related }' but its not the same...
> 
> What is the difference? More specifically, why is there a difference?

state { new, related  } asks nft to perform a lookup in an anonymous set with the
key values new and related and check if there is a matching key.

> That appears to be a bad pitfall for users. (And as such, choosing
> different symbols like the pipe symbol does not cure the issue of 
> confusion.)

The | already works, and in fact 'state new|related' is displayed as
'state new, related'.

state { new | related  } would ask nft to perform a lookup in anonymous
set, but that set would have just one value, namely the result of '4|8': 12.

Re: [PATCH] nft: Add support for inverted bitwise value list

[Pablo Neira Ayuso]

On Wed, Jun 22, 2016 at 05:49:48PM +0200, Laura Garcia Liebana wrote:
> Add support for inverted state and status bitwise value list required in the
> ct match.
> 
> Before this patch, nft didn't support the rule:
> 
> $ nft add rule ip filter INPUT ct state != new,related counter accept
> <cmdline>:1:41-41: Error: syntax error, unexpected comma, expecting end of file or newline or semicolon
> add rule ip filter INPUT ct state != new,related counter accept
>                                         ^
> 
> This patch includes in the parser the ability to understand a list of
> bitwise values.
> 
> nft --debug=netlink add rule ip filter INPUT ct state != new,related,established,untracked counter accept
> ip filter INPUT
>   [ ct load state => reg 1 ]
>   [ cmp neq reg 1 0x0000004e ]
>   [ counter pkts 0 bytes 0 ]
>   [ immediate reg 0 accept ]

This bytecode looks incorrect.

nft --debug=netlink add rule ip filter INPUT ct state new,related,established,untracked
ip filter INPUT 
  [ ct load state => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x0000004e ) ^ 0x00000000 ]
  [ cmp neq reg 1 0x00000000 ]

so I think the right bytecode should look like:

nft --debug=netlink add rule ip filter INPUT ct state new,related,established,untracked
ip filter INPUT 
  [ ct load state => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x0000004e ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000000 ] 

I guess something is missing from the expr_evaluate_relational(), I
can see:

        if (rel->op == OP_IMPLICIT) {
                switch (right->ops->type) {
                ...
                case EXPR_LIST:
                        rel->op = OP_FLAGCMP;

I guess rel->op is OP_NEQ for your case above, that's why it is
generating the wrong code.

Note that from netlink_linearize.c, it is netlink_gen_flagcmp() that
generates the bitwise + cmp when we see OP_FLAGCMP.

Instead of this, I would kill the OP_FLAGCMP and transform the left
hand side of the tree to get a bitwise from evaluate.c, so this looks
like:

        relational (OP_NEQ)
                / \
               /   \
              /     \
         bitwise   value
            /  \
           /    \
     ct state   mask

Then, we can kill netlink_gen_flagcmp() too since the
netlink_linearize.c will generate the right bytecode for us based on
that tree.