| Message ID | 20160622154945.GA12610@sonyv |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
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 ] -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(-)