[PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Florian Westphal]

bitmask types default to flagcmp now, thus do not suppress OP_EQ.  Else,

rule filter output tcp flags syn
rule filter output tcp flags == syn

are both displayed as 'flags syn'.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/expression.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/expression.c b/src/expression.c
index 1313925..fa14d99 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -514,13 +514,21 @@ static void binop_arg_print(const struct expr *op, const struct expr *arg)
 		printf(")");
 }
 
+static bool must_print_eq_op(const struct expr *expr)
+{
+	if (expr->right->dtype->basetype != NULL &&
+	    expr->right->dtype->basetype->type == TYPE_BITMASK)
+		return true;
+
+	return expr->left->ops->type == EXPR_BINOP;
+}
+
 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 ||
-	     expr->left->ops->type == EXPR_BINOP))
+	    (expr->op != OP_EQ || must_print_eq_op(expr)))
 		printf(" %s ", expr_op_symbols[expr->op]);
 	else
 		printf(" ");
-- 
1.8.1.5

Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Pablo Neira Ayuso]

On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote:
> bitmask types default to flagcmp now, thus do not suppress OP_EQ.  Else,
> 
> rule filter output tcp flags syn
> rule filter output tcp flags == syn
> 
> are both displayed as 'flags syn'.

I believe that in other selectors:

        selector == value
        selector value

are equivalent.

I think it's not just that we have to fix the printing, but make it
consistent.

> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  src/expression.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/expression.c b/src/expression.c
> index 1313925..fa14d99 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -514,13 +514,21 @@ static void binop_arg_print(const struct expr *op, const struct expr *arg)
>  		printf(")");
>  }
>  
> +static bool must_print_eq_op(const struct expr *expr)
> +{
> +	if (expr->right->dtype->basetype != NULL &&
> +	    expr->right->dtype->basetype->type == TYPE_BITMASK)
> +		return true;
> +
> +	return expr->left->ops->type == EXPR_BINOP;
> +}
> +
>  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 ||
> -	     expr->left->ops->type == EXPR_BINOP))
> +	    (expr->op != OP_EQ || must_print_eq_op(expr)))
>  		printf(" %s ", expr_op_symbols[expr->op]);
>  	else
>  		printf(" ");
> -- 
> 1.8.1.5
> 
> --
> 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

Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote:
> > bitmask types default to flagcmp now, thus do not suppress OP_EQ.  Else,
> > 
> > rule filter output tcp flags syn
> > rule filter output tcp flags == syn
> > 
> > are both displayed as 'flags syn'.
> 
> I believe that in other selectors:
> 
>         selector == value
>         selector value
> 
> are equivalent.

Yes, thats true.

> I think it's not just that we have to fix the printing, but make it
> consistent.

Not sure, this was changed recently, see
6bad82aba5d304c7a2dd1b19fe57464dca327f4a
(evaluate: use flagcmp for single RHS bitmask expression).

Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Pablo Neira Ayuso]

On Fri, Apr 04, 2014 at 10:33:28AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote:
> > > bitmask types default to flagcmp now, thus do not suppress OP_EQ.  Else,
> > > 
> > > rule filter output tcp flags syn
> > > rule filter output tcp flags == syn
> > > 
> > > are both displayed as 'flags syn'.
> > 
> > I believe that in other selectors:
> > 
> >         selector == value
> >         selector value
> > 
> > are equivalent.
> 
> Yes, thats true.
> 
> > I think it's not just that we have to fix the printing, but make it
> > consistent.
> 
> Not sure, this was changed recently, see
> 6bad82aba5d304c7a2dd1b19fe57464dca327f4a
> (evaluate: use flagcmp for single RHS bitmask expression).

That change is fine, I think we only have to fix tcp flags == syn to
make it equivalent to tcp flags syn. I don't find a good reason why
the should behave in a different way.

Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Patrick McHardy]

On Fri, Apr 04, 2014 at 10:17:23AM +0200, Pablo Neira Ayuso wrote:
> On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote:
> > bitmask types default to flagcmp now, thus do not suppress OP_EQ.  Else,
> > 
> > rule filter output tcp flags syn
> > rule filter output tcp flags == syn
> > 
> > are both displayed as 'flags syn'.
> 
> I believe that in other selectors:
> 
>         selector == value
>         selector value
> 
> are equivalent.

No, it really depends on the base type. For bitmasks the equality relation
should be printed explicitly.

> I think it's not just that we have to fix the printing, but make it
> consistent.

What do you propose to change?

Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Patrick McHardy]

On Fri, Apr 04, 2014 at 11:44:33AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 04, 2014 at 10:33:28AM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote:
> > > > bitmask types default to flagcmp now, thus do not suppress OP_EQ.  Else,
> > > > 
> > > > rule filter output tcp flags syn
> > > > rule filter output tcp flags == syn
> > > > 
> > > > are both displayed as 'flags syn'.
> > > 
> > > I believe that in other selectors:
> > > 
> > >         selector == value
> > >         selector value
> > > 
> > > are equivalent.
> > 
> > Yes, thats true.
> > 
> > > I think it's not just that we have to fix the printing, but make it
> > > consistent.
> > 
> > Not sure, this was changed recently, see
> > 6bad82aba5d304c7a2dd1b19fe57464dca327f4a
> > (evaluate: use flagcmp for single RHS bitmask expression).
> 
> That change is fine, I think we only have to fix tcp flags == syn to
> make it equivalent to tcp flags syn. I don't find a good reason why
> the should behave in a different way.

Because the implicit op for bitmasks is to test for any of the given bits.
tcp flags syn really should match on syn and syn/ack.

If an equality relation is explicitly specified by the user, it also needs
to be printed. Florian's change is all we need from what I can tell.

Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Pablo Neira Ayuso]

On Fri, Apr 04, 2014 at 02:09:48PM +0200, Patrick McHardy wrote:
> On Fri, Apr 04, 2014 at 11:44:33AM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Apr 04, 2014 at 10:33:28AM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote:
> > > > > bitmask types default to flagcmp now, thus do not suppress OP_EQ.  Else,
> > > > > 
> > > > > rule filter output tcp flags syn
> > > > > rule filter output tcp flags == syn
> > > > > 
> > > > > are both displayed as 'flags syn'.
> > > > 
> > > > I believe that in other selectors:
> > > > 
> > > >         selector == value
> > > >         selector value
> > > > 
> > > > are equivalent.
> > > 
> > > Yes, thats true.
> > > 
> > > > I think it's not just that we have to fix the printing, but make it
> > > > consistent.
> > > 
> > > Not sure, this was changed recently, see
> > > 6bad82aba5d304c7a2dd1b19fe57464dca327f4a
> > > (evaluate: use flagcmp for single RHS bitmask expression).
> > 
> > That change is fine, I think we only have to fix tcp flags == syn to
> > make it equivalent to tcp flags syn. I don't find a good reason why
> > the should behave in a different way.
> 
> Because the implicit op for bitmasks is to test for any of the given bits.
> tcp flags syn really should match on syn and syn/ack.

That's fine with me. The problem that I see is the inconsistent
interpretation depending on if the value is a flag or not.

> If an equality relation is explicitly specified by the user, it also needs
> to be printed. Florian's change is all we need from what I can tell.

Then we have to document that in some cases key == value and key
value are equivalent, and when it comes to flags it is not, which is
still rare to me.

nft --debug=netlink add rule filter output tcp flags == syn ip filter output
  [ payload load 1b @ network header + 9 => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ cmp eq reg 1 0x00000002 ]

nft --debug=netlink add rule filter output tcp flags syn ip filter output
  [ payload load 1b @ network header + 9 => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ bitwise reg 1 = (reg=1 & 0x00000002 ) ^ 0x00000000 ]
  [ cmp neq reg 1 0x00000000 ]

Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Patrick McHardy]

On Fri, Apr 04, 2014 at 04:04:30PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 04, 2014 at 02:09:48PM +0200, Patrick McHardy wrote:
> > On Fri, Apr 04, 2014 at 11:44:33AM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Apr 04, 2014 at 10:33:28AM +0200, Florian Westphal wrote:
> > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > On Mon, Mar 17, 2014 at 11:08:38PM +0100, Florian Westphal wrote:
> > > > > > bitmask types default to flagcmp now, thus do not suppress OP_EQ.  Else,
> > > > > > 
> > > > > > rule filter output tcp flags syn
> > > > > > rule filter output tcp flags == syn
> > > > > > 
> > > > > > are both displayed as 'flags syn'.
> > > > > 
> > > > > I believe that in other selectors:
> > > > > 
> > > > >         selector == value
> > > > >         selector value
> > > > > 
> > > > > are equivalent.
> > > > 
> > > > Yes, thats true.
> > > > 
> > > > > I think it's not just that we have to fix the printing, but make it
> > > > > consistent.
> > > > 
> > > > Not sure, this was changed recently, see
> > > > 6bad82aba5d304c7a2dd1b19fe57464dca327f4a
> > > > (evaluate: use flagcmp for single RHS bitmask expression).
> > > 
> > > That change is fine, I think we only have to fix tcp flags == syn to
> > > make it equivalent to tcp flags syn. I don't find a good reason why
> > > the should behave in a different way.
> > 
> > Because the implicit op for bitmasks is to test for any of the given bits.
> > tcp flags syn really should match on syn and syn/ack.
> 
> That's fine with me. The problem that I see is the inconsistent
> interpretation depending on if the value is a flag or not.

I don't see it as inconsistent, its just that the implicit op can mean
different things. Its basically meant to "do the right thing", which
IMO is the current behaviour. For me tcp flags syn also matching on syn/ack
is what I'd expect.

> > If an equality relation is explicitly specified by the user, it also needs
> > to be printed. Florian's change is all we need from what I can tell.
> 
> Then we have to document that in some cases key == value and key
> value are equivalent, and when it comes to flags it is not, which is
> still rare to me.

Sure. I think I already have it in my documentation. Its basically simply
documenting what the implicit op means in which context.

Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Pablo Neira Ayuso]

On Fri, Apr 04, 2014 at 04:24:40PM +0200, Patrick McHardy wrote:
> > > If an equality relation is explicitly specified by the user, it also needs
> > > to be printed. Florian's change is all we need from what I can tell.
> > 
> > Then we have to document that in some cases key == value and key
> > value are equivalent, and when it comes to flags it is not, which is
> > still rare to me.
> 
> Sure. I think I already have it in my documentation. Its basically simply
> documenting what the implicit op means in which context.

OK, please go ahead push it, thanks.

Re: [PATCH nft] expr: do not suppress OP_EQ when RHS is bitmask type

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Apr 04, 2014 at 04:24:40PM +0200, Patrick McHardy wrote:
> > > > If an equality relation is explicitly specified by the user, it also needs
> > > > to be printed. Florian's change is all we need from what I can tell.
> > > 
> > > Then we have to document that in some cases key == value and key
> > > value are equivalent, and when it comes to flags it is not, which is
> > > still rare to me.
> > 
> > Sure. I think I already have it in my documentation. Its basically simply
> > documenting what the implicit op means in which context.
> 
> OK, please go ahead push it, thanks.

done.