[PATCH nft 1/3] expression: missing != in flagcmp expression print function

[PATCH nft 1/3] expression: missing != in flagcmp expression print function

[Pablo Neira Ayuso]

Missing != when printing the expression.

Fixes: c3d57114f119 ("parser_bison: add shortcut syntax for matching flags without binary operations")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/expression.c            |  7 ++++++-
 tests/py/inet/tcp.t         |  1 +
 tests/py/inet/tcp.t.json    | 25 +++++++++++++++++++++++++
 tests/py/inet/tcp.t.payload |  8 ++++++++
 4 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/src/expression.c b/src/expression.c
index c6be000107f2..4c0874fe9950 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -1358,7 +1358,12 @@ struct expr *set_elem_catchall_expr_alloc(const struct location *loc)
 static void flagcmp_expr_print(const struct expr *expr, struct output_ctx *octx)
 {
 	expr_print(expr->flagcmp.expr, octx);
-	nft_print(octx, " ");
+
+	if (expr->op == OP_NEQ)
+		nft_print(octx, " != ");
+	else
+		nft_print(octx, " ");
+
 	expr_print(expr->flagcmp.value, octx);
 	nft_print(octx, " / ");
 	expr_print(expr->flagcmp.mask, octx);
diff --git a/tests/py/inet/tcp.t b/tests/py/inet/tcp.t
index 532da2776d24..16e15b9f76c1 100644
--- a/tests/py/inet/tcp.t
+++ b/tests/py/inet/tcp.t
@@ -68,6 +68,7 @@ tcp flags cwr;ok
 tcp flags != cwr;ok
 tcp flags == syn;ok
 tcp flags fin,syn / fin,syn;ok
+tcp flags != syn / fin,syn;ok
 tcp flags & (fin | syn | rst | psh | ack | urg | ecn | cwr) == fin | syn | rst | psh | ack | urg | ecn | cwr;ok;tcp flags == 0xff
 tcp flags { syn, syn | ack };ok
 tcp flags & (fin | syn | rst | psh | ack | urg) == { fin, ack, psh | ack, fin | psh | ack };ok
diff --git a/tests/py/inet/tcp.t.json b/tests/py/inet/tcp.t.json
index 8c2a376b2e60..590a3dee5d3f 100644
--- a/tests/py/inet/tcp.t.json
+++ b/tests/py/inet/tcp.t.json
@@ -1496,3 +1496,28 @@
         }
     }
 ]
+
+# tcp flags != syn / fin,syn
+[
+    {
+        "match": {
+            "left": {
+                "&": [
+                    {
+                        "payload": {
+                            "field": "flags",
+                            "protocol": "tcp"
+                        }
+                    },
+                    [
+                        "fin",
+                        "syn"
+                    ]
+                ]
+            },
+            "op": "!=",
+            "right": "syn"
+        }
+    }
+]
+
diff --git a/tests/py/inet/tcp.t.payload b/tests/py/inet/tcp.t.payload
index ee61b1a722d5..7f302080f02a 100644
--- a/tests/py/inet/tcp.t.payload
+++ b/tests/py/inet/tcp.t.payload
@@ -362,6 +362,14 @@ inet test-inet input
   [ bitwise reg 1 = ( reg 1 & 0x00000003 ) ^ 0x00000000 ]
   [ cmp eq reg 1 0x00000003 ]
 
+# tcp flags != syn / fin,syn
+inet test-inet input
+  [ meta load l4proto => reg 1 ]
+  [ cmp eq reg 1 0x00000006 ]
+  [ payload load 1b @ transport header + 13 => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0x00000003 ) ^ 0x00000000 ]
+  [ cmp neq reg 1 0x00000002 ]
+
 # tcp flags & (fin | syn | rst | psh | ack | urg | ecn | cwr) == fin | syn | rst | psh | ack | urg | ecn | cwr
 inet test-inet input
   [ meta load l4proto => reg 1 ]
-- 
2.20.1

[PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Pablo Neira Ayuso]

nft generates incorrect bytecode when combining flag datatype and binary
operations:

  # nft --debug=netlink add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) syn'
ip
  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ bitwise reg 1 = ( reg 1 & 0x00000017 ) ^ 0x00000000 ]
  [ bitwise reg 1 = ( reg 1 & 0x00000002 ) ^ 0x00000000 ]
  [ cmp neq reg 1 0x00000000 ]

Note the double bitwise expression. The last two expressions are not
correct either since it should match on the syn flag, ie. 0x2.

After this patch, netlink bytecode generation looks correct:

 # nft --debug=netlink add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) syn'
ip
  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ bitwise reg 1 = ( reg 1 & 0x00000017 ) ^ 0x00000000 ]
  [ cmp eq reg 1 0x00000002 ]

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/netlink_linearize.c     | 38 ++++++++++++++++-----------
 tests/py/inet/tcp.t         |  2 ++
 tests/py/inet/tcp.t.json    | 52 +++++++++++++++++++++++++++++++++++++
 tests/py/inet/tcp.t.payload | 16 ++++++++++++
 4 files changed, 93 insertions(+), 15 deletions(-)

diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 9ab3ec3ef2ff..eb53ccec1154 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -481,23 +481,31 @@ static void netlink_gen_flagcmp(struct netlink_linearize_ctx *ctx,
 	netlink_gen_raw_data(zero, expr->right->byteorder, len, &nld);
 	netlink_gen_data(expr->right, &nld2);
 
-	nle = alloc_nft_expr("bitwise");
-	netlink_put_register(nle, NFTNL_EXPR_BITWISE_SREG, sreg);
-	netlink_put_register(nle, NFTNL_EXPR_BITWISE_DREG, sreg);
-	nftnl_expr_set_u32(nle, NFTNL_EXPR_BITWISE_LEN, len);
-	nftnl_expr_set(nle, NFTNL_EXPR_BITWISE_MASK, &nld2.value, nld2.len);
-	nftnl_expr_set(nle, NFTNL_EXPR_BITWISE_XOR, &nld.value, nld.len);
-	nft_rule_add_expr(ctx, nle, &expr->location);
-
-	nle = alloc_nft_expr("cmp");
-	netlink_put_register(nle, NFTNL_EXPR_CMP_SREG, sreg);
-	if (expr->op == OP_NEG)
+	if (expr->left->etype == EXPR_BINOP) {
+		nle = alloc_nft_expr("cmp");
+		netlink_put_register(nle, NFTNL_EXPR_CMP_SREG, sreg);
 		nftnl_expr_set_u32(nle, NFTNL_EXPR_CMP_OP, NFT_CMP_EQ);
-	else
-		nftnl_expr_set_u32(nle, NFTNL_EXPR_CMP_OP, NFT_CMP_NEQ);
+		nftnl_expr_set(nle, NFTNL_EXPR_CMP_DATA, nld2.value, nld2.len);
+		nft_rule_add_expr(ctx, nle, &expr->location);
+	} else {
+		nle = alloc_nft_expr("bitwise");
+		netlink_put_register(nle, NFTNL_EXPR_BITWISE_SREG, sreg);
+		netlink_put_register(nle, NFTNL_EXPR_BITWISE_DREG, sreg);
+		nftnl_expr_set_u32(nle, NFTNL_EXPR_BITWISE_LEN, len);
+		nftnl_expr_set(nle, NFTNL_EXPR_BITWISE_MASK, &nld2.value, nld2.len);
+		nftnl_expr_set(nle, NFTNL_EXPR_BITWISE_XOR, &nld.value, nld.len);
+		nft_rule_add_expr(ctx, nle, &expr->location);
 
-	nftnl_expr_set(nle, NFTNL_EXPR_CMP_DATA, nld.value, nld.len);
-	nft_rule_add_expr(ctx, nle, &expr->location);
+		nle = alloc_nft_expr("cmp");
+		netlink_put_register(nle, NFTNL_EXPR_CMP_SREG, sreg);
+		if (expr->op == OP_NEG)
+			nftnl_expr_set_u32(nle, NFTNL_EXPR_CMP_OP, NFT_CMP_EQ);
+		else
+			nftnl_expr_set_u32(nle, NFTNL_EXPR_CMP_OP, NFT_CMP_NEQ);
+
+		nftnl_expr_set(nle, NFTNL_EXPR_CMP_DATA, nld.value, nld.len);
+		nft_rule_add_expr(ctx, nle, &expr->location);
+	}
 
 	mpz_clear(zero);
 	release_register(ctx, expr->left);
diff --git a/tests/py/inet/tcp.t b/tests/py/inet/tcp.t
index 16e15b9f76c1..983564ec5b75 100644
--- a/tests/py/inet/tcp.t
+++ b/tests/py/inet/tcp.t
@@ -69,6 +69,8 @@ tcp flags != cwr;ok
 tcp flags == syn;ok
 tcp flags fin,syn / fin,syn;ok
 tcp flags != syn / fin,syn;ok
+tcp flags & (fin | syn | rst | ack) syn;ok;tcp flags syn / fin,syn,rst,ack
+tcp flags & (fin | syn | rst | ack) != syn;ok;tcp flags != syn / fin,syn,rst,ack
 tcp flags & (fin | syn | rst | psh | ack | urg | ecn | cwr) == fin | syn | rst | psh | ack | urg | ecn | cwr;ok;tcp flags == 0xff
 tcp flags { syn, syn | ack };ok
 tcp flags & (fin | syn | rst | psh | ack | urg) == { fin, ack, psh | ack, fin | psh | ack };ok
diff --git a/tests/py/inet/tcp.t.json b/tests/py/inet/tcp.t.json
index 590a3dee5d3f..033a4f22e0fd 100644
--- a/tests/py/inet/tcp.t.json
+++ b/tests/py/inet/tcp.t.json
@@ -1521,3 +1521,55 @@
     }
 ]
 
+# tcp flags & (fin | syn | rst | ack) syn
+[
+    {
+        "match": {
+            "left": {
+                "&": [
+                    {
+                        "payload": {
+                            "field": "flags",
+                            "protocol": "tcp"
+                        }
+                    },
+                    [
+                        "fin",
+                        "syn",
+                        "rst",
+                        "ack"
+                    ]
+                ]
+            },
+            "op": "==",
+            "right": "syn"
+        }
+    }
+]
+
+# tcp flags & (fin | syn | rst | ack) != syn
+[
+    {
+        "match": {
+            "left": {
+                "&": [
+                    {
+                        "payload": {
+                            "field": "flags",
+                            "protocol": "tcp"
+                        }
+                    },
+                    [
+                        "fin",
+                        "syn",
+                        "rst",
+                        "ack"
+                    ]
+                ]
+            },
+            "op": "!=",
+            "right": "syn"
+        }
+    }
+]
+
diff --git a/tests/py/inet/tcp.t.payload b/tests/py/inet/tcp.t.payload
index 7f302080f02a..eaa7cd099bd6 100644
--- a/tests/py/inet/tcp.t.payload
+++ b/tests/py/inet/tcp.t.payload
@@ -370,6 +370,22 @@ inet test-inet input
   [ bitwise reg 1 = ( reg 1 & 0x00000003 ) ^ 0x00000000 ]
   [ cmp neq reg 1 0x00000002 ]
 
+# tcp flags & (fin | syn | rst | ack) syn
+inet test-inet input
+  [ meta load l4proto => reg 1 ]
+  [ cmp eq reg 1 0x00000006 ]
+  [ payload load 1b @ transport header + 13 => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0x00000017 ) ^ 0x00000000 ]
+  [ cmp eq reg 1 0x00000002 ]
+
+# tcp flags & (fin | syn | rst | ack) != syn
+inet test-inet input
+  [ meta load l4proto => reg 1 ]
+  [ cmp eq reg 1 0x00000006 ]
+  [ payload load 1b @ transport header + 13 => reg 1 ]
+  [ bitwise reg 1 = ( reg 1 & 0x00000017 ) ^ 0x00000000 ]
+  [ cmp neq reg 1 0x00000002 ]
+
 # tcp flags & (fin | syn | rst | psh | ack | urg | ecn | cwr) == fin | syn | rst | psh | ack | urg | ecn | cwr
 inet test-inet input
   [ meta load l4proto => reg 1 ]
-- 
2.20.1

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Tom Yan]

Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'. Does that
mean `tcp flags syn` (was supposed to be and) is now equivalent to
`tcp flags == syn` instead of `tcp flags & syn == syn` / `tcp flags &
syn != 0`?

Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
syn / syn` instead, please note that while nft translates `tcp flags &
syn == syn` to `tcp flags syn / syn`, it does not accept the
translation as input (when the mask is not a comma-separated list):

# nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
Error: syntax error, unexpected newline, expecting comma
add rule meh tcp_flags tcp flags syn / syn
                                          ^

Also, does that mean `tcp flags & (fin | syn | rst | ack) fin,syn,ack`
will now be equivalent to `tcp flags & (fin | syn | rst | ack) = (fin
| syn | ack)` instead of (ultimately) `tcp flags & (fin | syn | ack)
!= 0`? Which means `tcp flags & (fin | syn | ack) != 0` should not be
translated to `tcp flags fin,syn,ack`?


On Tue, 27 Jul 2021 at 23:37, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> nft generates incorrect bytecode when combining flag datatype and binary
> operations:
>
>   # nft --debug=netlink add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) syn'
> ip
>   [ meta load l4proto => reg 1 ]
>   [ cmp eq reg 1 0x00000006 ]
>   [ payload load 1b @ transport header + 13 => reg 1 ]
>   [ bitwise reg 1 = ( reg 1 & 0x00000017 ) ^ 0x00000000 ]
>   [ bitwise reg 1 = ( reg 1 & 0x00000002 ) ^ 0x00000000 ]
>   [ cmp neq reg 1 0x00000000 ]
>
> Note the double bitwise expression. The last two expressions are not
> correct either since it should match on the syn flag, ie. 0x2.
>
> After this patch, netlink bytecode generation looks correct:
>
>  # nft --debug=netlink add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) syn'
> ip
>   [ meta load l4proto => reg 1 ]
>   [ cmp eq reg 1 0x00000006 ]
>   [ payload load 1b @ transport header + 13 => reg 1 ]
>   [ bitwise reg 1 = ( reg 1 & 0x00000017 ) ^ 0x00000000 ]
>   [ cmp eq reg 1 0x00000002 ]
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/netlink_linearize.c     | 38 ++++++++++++++++-----------
>  tests/py/inet/tcp.t         |  2 ++
>  tests/py/inet/tcp.t.json    | 52 +++++++++++++++++++++++++++++++++++++
>  tests/py/inet/tcp.t.payload | 16 ++++++++++++
>  4 files changed, 93 insertions(+), 15 deletions(-)
>
> diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> index 9ab3ec3ef2ff..eb53ccec1154 100644
> --- a/src/netlink_linearize.c
> +++ b/src/netlink_linearize.c
> @@ -481,23 +481,31 @@ static void netlink_gen_flagcmp(struct netlink_linearize_ctx *ctx,
>         netlink_gen_raw_data(zero, expr->right->byteorder, len, &nld);
>         netlink_gen_data(expr->right, &nld2);
>
> -       nle = alloc_nft_expr("bitwise");
> -       netlink_put_register(nle, NFTNL_EXPR_BITWISE_SREG, sreg);
> -       netlink_put_register(nle, NFTNL_EXPR_BITWISE_DREG, sreg);
> -       nftnl_expr_set_u32(nle, NFTNL_EXPR_BITWISE_LEN, len);
> -       nftnl_expr_set(nle, NFTNL_EXPR_BITWISE_MASK, &nld2.value, nld2.len);
> -       nftnl_expr_set(nle, NFTNL_EXPR_BITWISE_XOR, &nld.value, nld.len);
> -       nft_rule_add_expr(ctx, nle, &expr->location);
> -
> -       nle = alloc_nft_expr("cmp");
> -       netlink_put_register(nle, NFTNL_EXPR_CMP_SREG, sreg);
> -       if (expr->op == OP_NEG)
> +       if (expr->left->etype == EXPR_BINOP) {
> +               nle = alloc_nft_expr("cmp");
> +               netlink_put_register(nle, NFTNL_EXPR_CMP_SREG, sreg);
>                 nftnl_expr_set_u32(nle, NFTNL_EXPR_CMP_OP, NFT_CMP_EQ);
> -       else
> -               nftnl_expr_set_u32(nle, NFTNL_EXPR_CMP_OP, NFT_CMP_NEQ);
> +               nftnl_expr_set(nle, NFTNL_EXPR_CMP_DATA, nld2.value, nld2.len);
> +               nft_rule_add_expr(ctx, nle, &expr->location);
> +       } else {
> +               nle = alloc_nft_expr("bitwise");
> +               netlink_put_register(nle, NFTNL_EXPR_BITWISE_SREG, sreg);
> +               netlink_put_register(nle, NFTNL_EXPR_BITWISE_DREG, sreg);
> +               nftnl_expr_set_u32(nle, NFTNL_EXPR_BITWISE_LEN, len);
> +               nftnl_expr_set(nle, NFTNL_EXPR_BITWISE_MASK, &nld2.value, nld2.len);
> +               nftnl_expr_set(nle, NFTNL_EXPR_BITWISE_XOR, &nld.value, nld.len);
> +               nft_rule_add_expr(ctx, nle, &expr->location);
>
> -       nftnl_expr_set(nle, NFTNL_EXPR_CMP_DATA, nld.value, nld.len);
> -       nft_rule_add_expr(ctx, nle, &expr->location);
> +               nle = alloc_nft_expr("cmp");
> +               netlink_put_register(nle, NFTNL_EXPR_CMP_SREG, sreg);
> +               if (expr->op == OP_NEG)
> +                       nftnl_expr_set_u32(nle, NFTNL_EXPR_CMP_OP, NFT_CMP_EQ);
> +               else
> +                       nftnl_expr_set_u32(nle, NFTNL_EXPR_CMP_OP, NFT_CMP_NEQ);
> +
> +               nftnl_expr_set(nle, NFTNL_EXPR_CMP_DATA, nld.value, nld.len);
> +               nft_rule_add_expr(ctx, nle, &expr->location);
> +       }
>
>         mpz_clear(zero);
>         release_register(ctx, expr->left);
> diff --git a/tests/py/inet/tcp.t b/tests/py/inet/tcp.t
> index 16e15b9f76c1..983564ec5b75 100644
> --- a/tests/py/inet/tcp.t
> +++ b/tests/py/inet/tcp.t
> @@ -69,6 +69,8 @@ tcp flags != cwr;ok
>  tcp flags == syn;ok
>  tcp flags fin,syn / fin,syn;ok
>  tcp flags != syn / fin,syn;ok
> +tcp flags & (fin | syn | rst | ack) syn;ok;tcp flags syn / fin,syn,rst,ack
> +tcp flags & (fin | syn | rst | ack) != syn;ok;tcp flags != syn / fin,syn,rst,ack
>  tcp flags & (fin | syn | rst | psh | ack | urg | ecn | cwr) == fin | syn | rst | psh | ack | urg | ecn | cwr;ok;tcp flags == 0xff
>  tcp flags { syn, syn | ack };ok
>  tcp flags & (fin | syn | rst | psh | ack | urg) == { fin, ack, psh | ack, fin | psh | ack };ok
> diff --git a/tests/py/inet/tcp.t.json b/tests/py/inet/tcp.t.json
> index 590a3dee5d3f..033a4f22e0fd 100644
> --- a/tests/py/inet/tcp.t.json
> +++ b/tests/py/inet/tcp.t.json
> @@ -1521,3 +1521,55 @@
>      }
>  ]
>
> +# tcp flags & (fin | syn | rst | ack) syn
> +[
> +    {
> +        "match": {
> +            "left": {
> +                "&": [
> +                    {
> +                        "payload": {
> +                            "field": "flags",
> +                            "protocol": "tcp"
> +                        }
> +                    },
> +                    [
> +                        "fin",
> +                        "syn",
> +                        "rst",
> +                        "ack"
> +                    ]
> +                ]
> +            },
> +            "op": "==",
> +            "right": "syn"
> +        }
> +    }
> +]
> +
> +# tcp flags & (fin | syn | rst | ack) != syn
> +[
> +    {
> +        "match": {
> +            "left": {
> +                "&": [
> +                    {
> +                        "payload": {
> +                            "field": "flags",
> +                            "protocol": "tcp"
> +                        }
> +                    },
> +                    [
> +                        "fin",
> +                        "syn",
> +                        "rst",
> +                        "ack"
> +                    ]
> +                ]
> +            },
> +            "op": "!=",
> +            "right": "syn"
> +        }
> +    }
> +]
> +
> diff --git a/tests/py/inet/tcp.t.payload b/tests/py/inet/tcp.t.payload
> index 7f302080f02a..eaa7cd099bd6 100644
> --- a/tests/py/inet/tcp.t.payload
> +++ b/tests/py/inet/tcp.t.payload
> @@ -370,6 +370,22 @@ inet test-inet input
>    [ bitwise reg 1 = ( reg 1 & 0x00000003 ) ^ 0x00000000 ]
>    [ cmp neq reg 1 0x00000002 ]
>
> +# tcp flags & (fin | syn | rst | ack) syn
> +inet test-inet input
> +  [ meta load l4proto => reg 1 ]
> +  [ cmp eq reg 1 0x00000006 ]
> +  [ payload load 1b @ transport header + 13 => reg 1 ]
> +  [ bitwise reg 1 = ( reg 1 & 0x00000017 ) ^ 0x00000000 ]
> +  [ cmp eq reg 1 0x00000002 ]
> +
> +# tcp flags & (fin | syn | rst | ack) != syn
> +inet test-inet input
> +  [ meta load l4proto => reg 1 ]
> +  [ cmp eq reg 1 0x00000006 ]
> +  [ payload load 1b @ transport header + 13 => reg 1 ]
> +  [ bitwise reg 1 = ( reg 1 & 0x00000017 ) ^ 0x00000000 ]
> +  [ cmp neq reg 1 0x00000002 ]
> +
>  # tcp flags & (fin | syn | rst | psh | ack | urg | ecn | cwr) == fin | syn | rst | psh | ack | urg | ecn | cwr
>  inet test-inet input
>    [ meta load l4proto => reg 1 ]
> --
> 2.20.1
>

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Pablo Neira Ayuso]

On Wed, Jul 28, 2021 at 02:36:18AM +0800, Tom Yan wrote:
> Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
> equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'.

Yes, those two are equivalent.

> Does that mean `tcp flags syn` (was supposed to be and) is now
> equivalent to `tcp flags == syn`

tcp flag syn

is a shortcut to match on the syn bit regarless other bit values, it's
a property of the bitmask datatypes.

tcp flags == syn

is an exact match, it checks that the syn bit is set on.

> instead of `tcp flags & syn == syn` / `tcp flags & syn != 0`?

these two above are equivalent, I just sent a patch to fix the
tcp flags & syn == syn case.

> Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
> syn / syn` instead, please note that while nft translates `tcp flags &
> syn == syn` to `tcp flags syn / syn`, it does not accept the
> translation as input (when the mask is not a comma-separated list):
> 
> # nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
> Error: syntax error, unexpected newline, expecting comma
> add rule meh tcp_flags tcp flags syn / syn
>                                           ^

The most simple way to express this is: tcp flags == syn.

> Also, does that mean `tcp flags & (fin | syn | rst | ack) fin,syn,ack`
> will now be equivalent to `tcp flags & (fin | syn | rst | ack) = (fin
> | syn | ack)`

Yes, those two are equivalent. This is the same example as the one you
have used at the beginning of this email.

> instead of (ultimately) `tcp flags & (fin | syn | ack)  != 0`?

That's equivalent to:

tcp flags fin,syn,ack

A quick summary:

- If you want an exact match:

tcp flags == fin,syn,ack

- If you want to check that those three bits are set on (regardless
  the remaining bits):

tcp flags fin,syn,ack / fin,syn,ack

- If you want to check that any of these three bits is set on:

tcp flags fin,syn,ack

> Which means `tcp flags & (fin | syn | ack) != 0` should not be
> translated to `tcp flags fin,syn,ack`?

tcp flags & (fin | syn | ack) != 0 is checking for any of these three
bits to be set on, this translation is correct.

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Tom Yan]

I'm not sure it's just me or you that are missing something here.

On Wed, 28 Jul 2021 at 05:05, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Wed, Jul 28, 2021 at 02:36:18AM +0800, Tom Yan wrote:
> > Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
> > equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'.
>
> Yes, those two are equivalent.
>
> > Does that mean `tcp flags syn` (was supposed to be and) is now
> > equivalent to `tcp flags == syn`
>
> tcp flag syn
>
> is a shortcut to match on the syn bit regarless other bit values, it's
> a property of the bitmask datatypes.

Don't you think the syntax will be inconsistent then? As a user, it
looks pretty irrational to me: with a mask, just `syn` checks the
exact value of the flags (combined); without a mask, it checks and
checks only whether the specific bit is on.

At least to me I would then expect `tcp flags syn` should be
equivalent / is a shortcut to `tcp flags & (fin | syn | rst | psh |
ack | urg | ecn | cwr) syn` hence `tcp flags & (fin | syn | rst | psh
| ack | urg | ecn | cwr) == syn` hence `tcp flags == syn`.

>
> tcp flags == syn
>
> is an exact match, it checks that the syn bit is set on.
>
> > instead of `tcp flags & syn == syn` / `tcp flags & syn != 0`?
>
> these two above are equivalent, I just sent a patch to fix the
> tcp flags & syn == syn case.
>
> > Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
> > syn / syn` instead, please note that while nft translates `tcp flags &
> > syn == syn` to `tcp flags syn / syn`, it does not accept the
> > translation as input (when the mask is not a comma-separated list):
> >
> > # nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
> > Error: syntax error, unexpected newline, expecting comma
> > add rule meh tcp_flags tcp flags syn / syn
> >                                           ^
>
> The most simple way to express this is: tcp flags == syn.

That does not sound right to me at all. Doesn't `syn / syn` means
"with the mask (the second/"denominator" `syn`) applied on the flags,
we get the value (the first/"nominator" `syn`), which means `tcp flags
& syn == syn` instead of `tcp flags == syn` (which in turn means all
bits but syn are cleared).

>
> > Also, does that mean `tcp flags & (fin | syn | rst | ack) fin,syn,ack`
> > will now be equivalent to `tcp flags & (fin | syn | rst | ack) = (fin
> > | syn | ack)`
>
> Yes, those two are equivalent. This is the same example as the one you
> have used at the beginning of this email.
>
> > instead of (ultimately) `tcp flags & (fin | syn | ack)  != 0`?
>
> That's equivalent to:
>
> tcp flags fin,syn,ack
>
> A quick summary:
>
> - If you want an exact match:
>
> tcp flags == fin,syn,ack
>
> - If you want to check that those three bits are set on (regardless
>   the remaining bits):
>
> tcp flags fin,syn,ack / fin,syn,ack
>
> - If you want to check that any of these three bits is set on:
>
> tcp flags fin,syn,ack
>
> > Which means `tcp flags & (fin | syn | ack) != 0` should not be
> > translated to `tcp flags fin,syn,ack`?
>
> tcp flags & (fin | syn | ack) != 0 is checking for any of these three
> bits to be set on, this translation is correct.

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Pablo Neira Ayuso]

On Thu, Jul 29, 2021 at 09:48:21AM +0800, Tom Yan wrote:
> I'm not sure it's just me or you that are missing something here.
> 
> On Wed, 28 Jul 2021 at 05:05, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > On Wed, Jul 28, 2021 at 02:36:18AM +0800, Tom Yan wrote:
> > > Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
> > > equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'.
> >
> > Yes, those two are equivalent.
> >
> > > Does that mean `tcp flags syn` (was supposed to be and) is now
> > > equivalent to `tcp flags == syn`
> >
> > tcp flag syn
> >
> > is a shortcut to match on the syn bit regarless other bit values, it's
> > a property of the bitmask datatypes.
> 
> Don't you think the syntax will be inconsistent then? As a user, it
> looks pretty irrational to me: with a mask, just `syn` checks the
> exact value of the flags (combined); without a mask, it checks and
> checks only whether the specific bit is on.
> 
> At least to me I would then expect `tcp flags syn` should be
> equivalent / is a shortcut to `tcp flags & (fin | syn | rst | psh |
> ack | urg | ecn | cwr) syn` hence `tcp flags & (fin | syn | rst | psh
> | ack | urg | ecn | cwr) == syn` hence `tcp flags == syn`.

As I said, think of a different use-case:

        ct state new,established

people are _not_ expecting to match on both flags to be set on (that
will actually never happen).

Should ct state and tcp flags use the same syntax (comma-separated
values) but intepret things in a different way? I don't think so.

You can use:

        nft describe ct state

to check for the real datatype behind this selector: it's a bitmask.
For this datatype the implicit operation is not ==.

> > tcp flags == syn
> >
> > is an exact match, it checks that the syn bit is set on.
> >
> > > instead of `tcp flags & syn == syn` / `tcp flags & syn != 0`?
> >
> > these two above are equivalent, I just sent a patch to fix the
> > tcp flags & syn == syn case.
> >
> > > Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
> > > syn / syn` instead, please note that while nft translates `tcp flags &
> > > syn == syn` to `tcp flags syn / syn`, it does not accept the
> > > translation as input (when the mask is not a comma-separated list):
> > >
> > > # nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
> > > Error: syntax error, unexpected newline, expecting comma
> > > add rule meh tcp_flags tcp flags syn / syn
> > >                                           ^
> >
> > The most simple way to express this is: tcp flags == syn.
> 
> That does not sound right to me at all. Doesn't `syn / syn` means
> "with the mask (the second/"denominator" `syn`) applied on the flags,
> we get the value (the first/"nominator" `syn`), which means `tcp flags
> & syn == syn` instead of `tcp flags == syn` (which in turn means all
> bits but syn are cleared).

tcp flags syn / syn makes no sense, it's actually: tcp flags syn.

The goal is to provide a compact syntax for most common operations, so
users do not need to be mingling with explicit binary expressions
(which is considered to be a "more advanced" operation).

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Tom Yan]

I was quite overwhelmed by the whole thing, so I might not have made
my point clear.

While I find e.g. `tcp flags fin,ack,rst` being not the same as `tcp
flags { fin, ack, rst }` confusing, in this case it is still
"reasonable", as we can say that in the former it is a
"comma-separated list" (hence no space), while in the latter it is a
set (hence the spaces).

The real issue here is that a comma-separated list itself can have
totally different meanings. For example,

1. If (just) `fin,rst,ack`, it means "any of the bits set".
2. If `== fin,rst,ack`, it means (fin | rst | ack)
3. If `fin,rst,ack / fin,rst,ack`, it means (fin | rst | ack). *(For
both the value and the mask)*
4. If `== fin,rst,ack / fin,rst,ack`, it means (fin | rst | ack).
*(For both the value and the mask)*

So how could anyone have thought in the first case `fin,rst,ack` does
not have the same meaning as it does in the other cases? That's what I
would call "unreasonable". Also, if in any case e.g. (just) `syn` is
not considered a (single-value) "comma-separated list", it's
"unreasonable" as well.

Or in other words, I don't find a behavior/shortcut like, "if a mask
is not specified explicitly, a mask that is equal to the value is
implied, yet not when we compare the value (e.g. ==)", sensible /
sensical. (Would anyone?)

And you know what, comparing this with the `ct state` is unfair. The
fact that you did so sort of explains why we end up in this...mess.
(Not trying to say it's your fault but rather, how the issue could
have happened.)

In the case of `ct state`, while we use the different bits for the
states, the states themselves are mutually exclusive (AFAIK, e.g. a
packet can't be new and established at the same time). People assume
e.g. `related,established` to be *practically* equivalent to `{
related, established }` not because they would think like, "generally
a comma-separated list should mean any of states", but either because
they know in advance a packet can only be of either, or, they assume
such similar syntax should mean the same thing. (The truth is, `ct
state related,established` is NOT *logically* the same as `ct state {
related, established }`; the former will be true for a packet if its
state can be / is related | established.)

On Thu, 29 Jul 2021 at 15:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Thu, Jul 29, 2021 at 09:48:21AM +0800, Tom Yan wrote:
> > I'm not sure it's just me or you that are missing something here.
> >
> > On Wed, 28 Jul 2021 at 05:05, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >
> > > On Wed, Jul 28, 2021 at 02:36:18AM +0800, Tom Yan wrote:
> > > > Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
> > > > equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'.
> > >
> > > Yes, those two are equivalent.
> > >
> > > > Does that mean `tcp flags syn` (was supposed to be and) is now
> > > > equivalent to `tcp flags == syn`
> > >
> > > tcp flag syn
> > >
> > > is a shortcut to match on the syn bit regarless other bit values, it's
> > > a property of the bitmask datatypes.
> >
> > Don't you think the syntax will be inconsistent then? As a user, it
> > looks pretty irrational to me: with a mask, just `syn` checks the
> > exact value of the flags (combined); without a mask, it checks and
> > checks only whether the specific bit is on.
> >
> > At least to me I would then expect `tcp flags syn` should be
> > equivalent / is a shortcut to `tcp flags & (fin | syn | rst | psh |
> > ack | urg | ecn | cwr) syn` hence `tcp flags & (fin | syn | rst | psh
> > | ack | urg | ecn | cwr) == syn` hence `tcp flags == syn`.
>
> As I said, think of a different use-case:
>
>         ct state new,established
>
> people are _not_ expecting to match on both flags to be set on (that
> will actually never happen).
>
> Should ct state and tcp flags use the same syntax (comma-separated
> values) but intepret things in a different way? I don't think so.
>
> You can use:
>
>         nft describe ct state
>
> to check for the real datatype behind this selector: it's a bitmask.
> For this datatype the implicit operation is not ==.
>
> > > tcp flags == syn
> > >
> > > is an exact match, it checks that the syn bit is set on.
> > >
> > > > instead of `tcp flags & syn == syn` / `tcp flags & syn != 0`?
> > >
> > > these two above are equivalent, I just sent a patch to fix the
> > > tcp flags & syn == syn case.
> > >
> > > > Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
> > > > syn / syn` instead, please note that while nft translates `tcp flags &
> > > > syn == syn` to `tcp flags syn / syn`, it does not accept the
> > > > translation as input (when the mask is not a comma-separated list):
> > > >
> > > > # nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
> > > > Error: syntax error, unexpected newline, expecting comma
> > > > add rule meh tcp_flags tcp flags syn / syn
> > > >                                           ^
> > >
> > > The most simple way to express this is: tcp flags == syn.
> >
> > That does not sound right to me at all. Doesn't `syn / syn` means
> > "with the mask (the second/"denominator" `syn`) applied on the flags,
> > we get the value (the first/"nominator" `syn`), which means `tcp flags
> > & syn == syn` instead of `tcp flags == syn` (which in turn means all
> > bits but syn are cleared).
>
> tcp flags syn / syn makes no sense, it's actually: tcp flags syn.
>
> The goal is to provide a compact syntax for most common operations, so
> users do not need to be mingling with explicit binary expressions
> (which is considered to be a "more advanced" operation).

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Tom Yan]

Allow me to add one more supplement to what I just wrote, as you/some
might say something like "but allowing `ct state related,established`
to work as expected is intuitive and neat".

Consider this:

# nft --debug=netlink add rule meh tcp_flags 'ct state == new,established'
ip
  [ ct load state => reg 1 ]
  [ cmp eq reg 1 0x0000000a ]

# nft list ruleset
table ip meh {
    chain tcp_flags {
        ct state established | new
    }
}

If `ct state new,established` is interpreted to be the same as `ct
state == new,established`, at least we *will know* that we should use
`ct state { new, established }` instead. But when it is not, we can
only *suppose* it to be *practically* the same as it. (I'm quite sure
many people think like, "nft is just not completed/smart enough yet to
be able to omit the redundant curly braces"...)

On Thu, 29 Jul 2021 at 18:41, Tom Yan <tom.ty89@gmail.com> wrote:
>
> I was quite overwhelmed by the whole thing, so I might not have made
> my point clear.
>
> While I find e.g. `tcp flags fin,ack,rst` being not the same as `tcp
> flags { fin, ack, rst }` confusing, in this case it is still
> "reasonable", as we can say that in the former it is a
> "comma-separated list" (hence no space), while in the latter it is a
> set (hence the spaces).
>
> The real issue here is that a comma-separated list itself can have
> totally different meanings. For example,
>
> 1. If (just) `fin,rst,ack`, it means "any of the bits set".
> 2. If `== fin,rst,ack`, it means (fin | rst | ack)
> 3. If `fin,rst,ack / fin,rst,ack`, it means (fin | rst | ack). *(For
> both the value and the mask)*
> 4. If `== fin,rst,ack / fin,rst,ack`, it means (fin | rst | ack).
> *(For both the value and the mask)*
>
> So how could anyone have thought in the first case `fin,rst,ack` does
> not have the same meaning as it does in the other cases? That's what I
> would call "unreasonable". Also, if in any case e.g. (just) `syn` is
> not considered a (single-value) "comma-separated list", it's
> "unreasonable" as well.
>
> Or in other words, I don't find a behavior/shortcut like, "if a mask
> is not specified explicitly, a mask that is equal to the value is
> implied, yet not when we compare the value (e.g. ==)", sensible /
> sensical. (Would anyone?)
>
> And you know what, comparing this with the `ct state` is unfair. The
> fact that you did so sort of explains why we end up in this...mess.
> (Not trying to say it's your fault but rather, how the issue could
> have happened.)
>
> In the case of `ct state`, while we use the different bits for the
> states, the states themselves are mutually exclusive (AFAIK, e.g. a
> packet can't be new and established at the same time). People assume
> e.g. `related,established` to be *practically* equivalent to `{
> related, established }` not because they would think like, "generally
> a comma-separated list should mean any of states", but either because
> they know in advance a packet can only be of either, or, they assume
> such similar syntax should mean the same thing. (The truth is, `ct
> state related,established` is NOT *logically* the same as `ct state {
> related, established }`; the former will be true for a packet if its
> state can be / is related | established.)
>
> On Thu, 29 Jul 2021 at 15:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > On Thu, Jul 29, 2021 at 09:48:21AM +0800, Tom Yan wrote:
> > > I'm not sure it's just me or you that are missing something here.
> > >
> > > On Wed, 28 Jul 2021 at 05:05, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >
> > > > On Wed, Jul 28, 2021 at 02:36:18AM +0800, Tom Yan wrote:
> > > > > Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
> > > > > equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'.
> > > >
> > > > Yes, those two are equivalent.
> > > >
> > > > > Does that mean `tcp flags syn` (was supposed to be and) is now
> > > > > equivalent to `tcp flags == syn`
> > > >
> > > > tcp flag syn
> > > >
> > > > is a shortcut to match on the syn bit regarless other bit values, it's
> > > > a property of the bitmask datatypes.
> > >
> > > Don't you think the syntax will be inconsistent then? As a user, it
> > > looks pretty irrational to me: with a mask, just `syn` checks the
> > > exact value of the flags (combined); without a mask, it checks and
> > > checks only whether the specific bit is on.
> > >
> > > At least to me I would then expect `tcp flags syn` should be
> > > equivalent / is a shortcut to `tcp flags & (fin | syn | rst | psh |
> > > ack | urg | ecn | cwr) syn` hence `tcp flags & (fin | syn | rst | psh
> > > | ack | urg | ecn | cwr) == syn` hence `tcp flags == syn`.
> >
> > As I said, think of a different use-case:
> >
> >         ct state new,established
> >
> > people are _not_ expecting to match on both flags to be set on (that
> > will actually never happen).
> >
> > Should ct state and tcp flags use the same syntax (comma-separated
> > values) but intepret things in a different way? I don't think so.
> >
> > You can use:
> >
> >         nft describe ct state
> >
> > to check for the real datatype behind this selector: it's a bitmask.
> > For this datatype the implicit operation is not ==.
> >
> > > > tcp flags == syn
> > > >
> > > > is an exact match, it checks that the syn bit is set on.
> > > >
> > > > > instead of `tcp flags & syn == syn` / `tcp flags & syn != 0`?
> > > >
> > > > these two above are equivalent, I just sent a patch to fix the
> > > > tcp flags & syn == syn case.
> > > >
> > > > > Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
> > > > > syn / syn` instead, please note that while nft translates `tcp flags &
> > > > > syn == syn` to `tcp flags syn / syn`, it does not accept the
> > > > > translation as input (when the mask is not a comma-separated list):
> > > > >
> > > > > # nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
> > > > > Error: syntax error, unexpected newline, expecting comma
> > > > > add rule meh tcp_flags tcp flags syn / syn
> > > > >                                           ^
> > > >
> > > > The most simple way to express this is: tcp flags == syn.
> > >
> > > That does not sound right to me at all. Doesn't `syn / syn` means
> > > "with the mask (the second/"denominator" `syn`) applied on the flags,
> > > we get the value (the first/"nominator" `syn`), which means `tcp flags
> > > & syn == syn` instead of `tcp flags == syn` (which in turn means all
> > > bits but syn are cleared).
> >
> > tcp flags syn / syn makes no sense, it's actually: tcp flags syn.
> >
> > The goal is to provide a compact syntax for most common operations, so
> > users do not need to be mingling with explicit binary expressions
> > (which is considered to be a "more advanced" operation).

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Tom Yan]

I've written sort of a specification / the grammar of how things
should be. See if it helps you see my point and if you can find any
problem in it:

spec/grammar for comma-separated list

definition: string representation of *one or more* bits, separated by comma
example: syn fin,rst,ack
meaning (of the comma): bitwise or

When a list is matched against, the value of the list is compared
against. Therefore the following are equivalent:

tcp flags syn
tcp flags == syn

And these are equivalent:

tcp flags fin,rst,ack
tcp flags == (fin | rst | ack)

The behavior of the matching / the meaning of a list *remains the
same* when a mask is applied, hence these are also equivalent:

tcp flags & syn syn
tcp flags & syn == syn

And so are these:

tcp flags & (fin | rst | ack) fin,rst,ack
tcp flags & (fin | rst | ack) == fin,rst,ack
tcp flags & (fin | rst | ack) == (fin | rst | ack)
tcp flags & (fin | rst | ack) (fin | rst | ack)

They can be represented in an alternative "value_in_list_form /
mask_in_list_form":

tcp flags syn / syn
tcp flags fin,rst,ack / fin,rst,ack

The behavior of the matching / the meaning of a list *remains the
same* when the operator is !=. So these are equivalent:

tcp flags != syn / syn
tcp flags & syn != syn

So are these:

tcp flags != fin,rst,ack / fin,rst,ack
tcp flags & (fin | rst | ack) != (fin | rst | ack)
tcp flags & (fin | rst | ack) != fin,rst,ack

Note that != is not the same as !. What the latter means is, the
specified list is used as the mask and the output will be compared
with 0. So these are equivalent:

tcp flags ! syn
tcp flags & syn == 0

So are these:

tcp flags ! fin,rst,ack
tcp flags & (fin | rst | ack) == 0

Note that ! cannot be used on / with a mask. Therefore:

tcp flags ! syn / syn
tcp flags syn / ! syn
tcp flags & ! syn syn
tcp flags & syn ! syn
...

tcp flags ! fin,rst,ack / fin,rst,ack
tcp flags fin,rst,ack / ! fin,rst,ack
tcp flags & ! (fin, rst, ack) fin,rst,ack
tcp flags & (fin, rst, ack) ! fin,rst,ack
...

are all invalid.

Also, there is no simplified / shortcut form for:

tcp flags & syn != 0
tcp flags & (fin | rst | ack) != 0

Although the first one is *inheritly* (i.e. by nature) equivalent to:

tcp flags & syn == syn
tcp flags & syn syn
tcp flags syn / syn

On Thu, 29 Jul 2021 at 18:58, Tom Yan <tom.ty89@gmail.com> wrote:
>
> Allow me to add one more supplement to what I just wrote, as you/some
> might say something like "but allowing `ct state related,established`
> to work as expected is intuitive and neat".
>
> Consider this:
>
> # nft --debug=netlink add rule meh tcp_flags 'ct state == new,established'
> ip
>   [ ct load state => reg 1 ]
>   [ cmp eq reg 1 0x0000000a ]
>
> # nft list ruleset
> table ip meh {
>     chain tcp_flags {
>         ct state established | new
>     }
> }
>
> If `ct state new,established` is interpreted to be the same as `ct
> state == new,established`, at least we *will know* that we should use
> `ct state { new, established }` instead. But when it is not, we can
> only *suppose* it to be *practically* the same as it. (I'm quite sure
> many people think like, "nft is just not completed/smart enough yet to
> be able to omit the redundant curly braces"...)
>
> On Thu, 29 Jul 2021 at 18:41, Tom Yan <tom.ty89@gmail.com> wrote:
> >
> > I was quite overwhelmed by the whole thing, so I might not have made
> > my point clear.
> >
> > While I find e.g. `tcp flags fin,ack,rst` being not the same as `tcp
> > flags { fin, ack, rst }` confusing, in this case it is still
> > "reasonable", as we can say that in the former it is a
> > "comma-separated list" (hence no space), while in the latter it is a
> > set (hence the spaces).
> >
> > The real issue here is that a comma-separated list itself can have
> > totally different meanings. For example,
> >
> > 1. If (just) `fin,rst,ack`, it means "any of the bits set".
> > 2. If `== fin,rst,ack`, it means (fin | rst | ack)
> > 3. If `fin,rst,ack / fin,rst,ack`, it means (fin | rst | ack). *(For
> > both the value and the mask)*
> > 4. If `== fin,rst,ack / fin,rst,ack`, it means (fin | rst | ack).
> > *(For both the value and the mask)*
> >
> > So how could anyone have thought in the first case `fin,rst,ack` does
> > not have the same meaning as it does in the other cases? That's what I
> > would call "unreasonable". Also, if in any case e.g. (just) `syn` is
> > not considered a (single-value) "comma-separated list", it's
> > "unreasonable" as well.
> >
> > Or in other words, I don't find a behavior/shortcut like, "if a mask
> > is not specified explicitly, a mask that is equal to the value is
> > implied, yet not when we compare the value (e.g. ==)", sensible /
> > sensical. (Would anyone?)
> >
> > And you know what, comparing this with the `ct state` is unfair. The
> > fact that you did so sort of explains why we end up in this...mess.
> > (Not trying to say it's your fault but rather, how the issue could
> > have happened.)
> >
> > In the case of `ct state`, while we use the different bits for the
> > states, the states themselves are mutually exclusive (AFAIK, e.g. a
> > packet can't be new and established at the same time). People assume
> > e.g. `related,established` to be *practically* equivalent to `{
> > related, established }` not because they would think like, "generally
> > a comma-separated list should mean any of states", but either because
> > they know in advance a packet can only be of either, or, they assume
> > such similar syntax should mean the same thing. (The truth is, `ct
> > state related,established` is NOT *logically* the same as `ct state {
> > related, established }`; the former will be true for a packet if its
> > state can be / is related | established.)
> >
> > On Thu, 29 Jul 2021 at 15:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >
> > > On Thu, Jul 29, 2021 at 09:48:21AM +0800, Tom Yan wrote:
> > > > I'm not sure it's just me or you that are missing something here.
> > > >
> > > > On Wed, 28 Jul 2021 at 05:05, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > >
> > > > > On Wed, Jul 28, 2021 at 02:36:18AM +0800, Tom Yan wrote:
> > > > > > Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
> > > > > > equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'.
> > > > >
> > > > > Yes, those two are equivalent.
> > > > >
> > > > > > Does that mean `tcp flags syn` (was supposed to be and) is now
> > > > > > equivalent to `tcp flags == syn`
> > > > >
> > > > > tcp flag syn
> > > > >
> > > > > is a shortcut to match on the syn bit regarless other bit values, it's
> > > > > a property of the bitmask datatypes.
> > > >
> > > > Don't you think the syntax will be inconsistent then? As a user, it
> > > > looks pretty irrational to me: with a mask, just `syn` checks the
> > > > exact value of the flags (combined); without a mask, it checks and
> > > > checks only whether the specific bit is on.
> > > >
> > > > At least to me I would then expect `tcp flags syn` should be
> > > > equivalent / is a shortcut to `tcp flags & (fin | syn | rst | psh |
> > > > ack | urg | ecn | cwr) syn` hence `tcp flags & (fin | syn | rst | psh
> > > > | ack | urg | ecn | cwr) == syn` hence `tcp flags == syn`.
> > >
> > > As I said, think of a different use-case:
> > >
> > >         ct state new,established
> > >
> > > people are _not_ expecting to match on both flags to be set on (that
> > > will actually never happen).
> > >
> > > Should ct state and tcp flags use the same syntax (comma-separated
> > > values) but intepret things in a different way? I don't think so.
> > >
> > > You can use:
> > >
> > >         nft describe ct state
> > >
> > > to check for the real datatype behind this selector: it's a bitmask.
> > > For this datatype the implicit operation is not ==.
> > >
> > > > > tcp flags == syn
> > > > >
> > > > > is an exact match, it checks that the syn bit is set on.
> > > > >
> > > > > > instead of `tcp flags & syn == syn` / `tcp flags & syn != 0`?
> > > > >
> > > > > these two above are equivalent, I just sent a patch to fix the
> > > > > tcp flags & syn == syn case.
> > > > >
> > > > > > Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
> > > > > > syn / syn` instead, please note that while nft translates `tcp flags &
> > > > > > syn == syn` to `tcp flags syn / syn`, it does not accept the
> > > > > > translation as input (when the mask is not a comma-separated list):
> > > > > >
> > > > > > # nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
> > > > > > Error: syntax error, unexpected newline, expecting comma
> > > > > > add rule meh tcp_flags tcp flags syn / syn
> > > > > >                                           ^
> > > > >
> > > > > The most simple way to express this is: tcp flags == syn.
> > > >
> > > > That does not sound right to me at all. Doesn't `syn / syn` means
> > > > "with the mask (the second/"denominator" `syn`) applied on the flags,
> > > > we get the value (the first/"nominator" `syn`), which means `tcp flags
> > > > & syn == syn` instead of `tcp flags == syn` (which in turn means all
> > > > bits but syn are cleared).
> > >
> > > tcp flags syn / syn makes no sense, it's actually: tcp flags syn.
> > >
> > > The goal is to provide a compact syntax for most common operations, so
> > > users do not need to be mingling with explicit binary expressions
> > > (which is considered to be a "more advanced" operation).

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Tom Yan]

To put the point of this spec into words: what's really nasty and
wrong here is that we do not use an operator when we want to perform
an operation on the comma-separated list, but instead, we make it
imply an operation *conditionally* and *implicitly*.

If you look at the spec again, every comma-separated list refers to
only a numeric value, but not whether this value should be used as a
mask or introduce another numeric value (0) for comparison.

For example, there's a reason it's fine to use `! syn` / `!
fin,rst,ack` to refer to `& syn == 0` / `& fin,rst,ack == 0`: the `!`
indicated the operation (and it's fine that it also introduces another
value, since the value never changes: it's always 0).

While I don't see a point in introducing an operator to refer to `&
list != 0`, if you insist on having a shortcut for that, with e.g. `@
syn` / `@ fin,rst,ack` at least things will be "reasonable".

Can you even document it like this with how "shortcuting" is
"beautifully" abused:

    [ == ] list: flags has the same value as list
        != list: flags has different value from list
         ! list: flags has none of the bit(s) in list set (a.k.a.
flags & list == 0)
list_a / list_b: with flags masked with list_b, flags has the same
value as list_a (a.k.a. flags & list_b == list_a)
         @ list: flags has at least one of the bits in list set
(a.k.a. flags & list != 0)

There's something you can't just "shortcut". When you do it wrong, it
does no one any good, but only makes things *seem* neat. Just `tcp
flags syn / syn` is perfectly sensical. It might look bad, but it
doesn't mean that you can do it wrong.

Regards,
Tom

On Thu, 29 Jul 2021 at 23:16, Tom Yan <tom.ty89@gmail.com> wrote:
>
> I've written sort of a specification / the grammar of how things
> should be. See if it helps you see my point and if you can find any
> problem in it:
>
> spec/grammar for comma-separated list
>
> definition: string representation of *one or more* bits, separated by comma
> example: syn fin,rst,ack
> meaning (of the comma): bitwise or
>
> When a list is matched against, the value of the list is compared
> against. Therefore the following are equivalent:
>
> tcp flags syn
> tcp flags == syn
>
> And these are equivalent:
>
> tcp flags fin,rst,ack
> tcp flags == (fin | rst | ack)
>
> The behavior of the matching / the meaning of a list *remains the
> same* when a mask is applied, hence these are also equivalent:
>
> tcp flags & syn syn
> tcp flags & syn == syn
>
> And so are these:
>
> tcp flags & (fin | rst | ack) fin,rst,ack
> tcp flags & (fin | rst | ack) == fin,rst,ack
> tcp flags & (fin | rst | ack) == (fin | rst | ack)
> tcp flags & (fin | rst | ack) (fin | rst | ack)
>
> They can be represented in an alternative "value_in_list_form /
> mask_in_list_form":
>
> tcp flags syn / syn
> tcp flags fin,rst,ack / fin,rst,ack
>
> The behavior of the matching / the meaning of a list *remains the
> same* when the operator is !=. So these are equivalent:
>
> tcp flags != syn / syn
> tcp flags & syn != syn
>
> So are these:
>
> tcp flags != fin,rst,ack / fin,rst,ack
> tcp flags & (fin | rst | ack) != (fin | rst | ack)
> tcp flags & (fin | rst | ack) != fin,rst,ack
>
> Note that != is not the same as !. What the latter means is, the
> specified list is used as the mask and the output will be compared
> with 0. So these are equivalent:
>
> tcp flags ! syn
> tcp flags & syn == 0
>
> So are these:
>
> tcp flags ! fin,rst,ack
> tcp flags & (fin | rst | ack) == 0
>
> Note that ! cannot be used on / with a mask. Therefore:
>
> tcp flags ! syn / syn
> tcp flags syn / ! syn
> tcp flags & ! syn syn
> tcp flags & syn ! syn
> ...
>
> tcp flags ! fin,rst,ack / fin,rst,ack
> tcp flags fin,rst,ack / ! fin,rst,ack
> tcp flags & ! (fin, rst, ack) fin,rst,ack
> tcp flags & (fin, rst, ack) ! fin,rst,ack
> ...
>
> are all invalid.
>
> Also, there is no simplified / shortcut form for:
>
> tcp flags & syn != 0
> tcp flags & (fin | rst | ack) != 0
>
> Although the first one is *inheritly* (i.e. by nature) equivalent to:
>
> tcp flags & syn == syn
> tcp flags & syn syn
> tcp flags syn / syn
>
> On Thu, 29 Jul 2021 at 18:58, Tom Yan <tom.ty89@gmail.com> wrote:
> >
> > Allow me to add one more supplement to what I just wrote, as you/some
> > might say something like "but allowing `ct state related,established`
> > to work as expected is intuitive and neat".
> >
> > Consider this:
> >
> > # nft --debug=netlink add rule meh tcp_flags 'ct state == new,established'
> > ip
> >   [ ct load state => reg 1 ]
> >   [ cmp eq reg 1 0x0000000a ]
> >
> > # nft list ruleset
> > table ip meh {
> >     chain tcp_flags {
> >         ct state established | new
> >     }
> > }
> >
> > If `ct state new,established` is interpreted to be the same as `ct
> > state == new,established`, at least we *will know* that we should use
> > `ct state { new, established }` instead. But when it is not, we can
> > only *suppose* it to be *practically* the same as it. (I'm quite sure
> > many people think like, "nft is just not completed/smart enough yet to
> > be able to omit the redundant curly braces"...)
> >
> > On Thu, 29 Jul 2021 at 18:41, Tom Yan <tom.ty89@gmail.com> wrote:
> > >
> > > I was quite overwhelmed by the whole thing, so I might not have made
> > > my point clear.
> > >
> > > While I find e.g. `tcp flags fin,ack,rst` being not the same as `tcp
> > > flags { fin, ack, rst }` confusing, in this case it is still
> > > "reasonable", as we can say that in the former it is a
> > > "comma-separated list" (hence no space), while in the latter it is a
> > > set (hence the spaces).
> > >
> > > The real issue here is that a comma-separated list itself can have
> > > totally different meanings. For example,
> > >
> > > 1. If (just) `fin,rst,ack`, it means "any of the bits set".
> > > 2. If `== fin,rst,ack`, it means (fin | rst | ack)
> > > 3. If `fin,rst,ack / fin,rst,ack`, it means (fin | rst | ack). *(For
> > > both the value and the mask)*
> > > 4. If `== fin,rst,ack / fin,rst,ack`, it means (fin | rst | ack).
> > > *(For both the value and the mask)*
> > >
> > > So how could anyone have thought in the first case `fin,rst,ack` does
> > > not have the same meaning as it does in the other cases? That's what I
> > > would call "unreasonable". Also, if in any case e.g. (just) `syn` is
> > > not considered a (single-value) "comma-separated list", it's
> > > "unreasonable" as well.
> > >
> > > Or in other words, I don't find a behavior/shortcut like, "if a mask
> > > is not specified explicitly, a mask that is equal to the value is
> > > implied, yet not when we compare the value (e.g. ==)", sensible /
> > > sensical. (Would anyone?)
> > >
> > > And you know what, comparing this with the `ct state` is unfair. The
> > > fact that you did so sort of explains why we end up in this...mess.
> > > (Not trying to say it's your fault but rather, how the issue could
> > > have happened.)
> > >
> > > In the case of `ct state`, while we use the different bits for the
> > > states, the states themselves are mutually exclusive (AFAIK, e.g. a
> > > packet can't be new and established at the same time). People assume
> > > e.g. `related,established` to be *practically* equivalent to `{
> > > related, established }` not because they would think like, "generally
> > > a comma-separated list should mean any of states", but either because
> > > they know in advance a packet can only be of either, or, they assume
> > > such similar syntax should mean the same thing. (The truth is, `ct
> > > state related,established` is NOT *logically* the same as `ct state {
> > > related, established }`; the former will be true for a packet if its
> > > state can be / is related | established.)
> > >
> > > On Thu, 29 Jul 2021 at 15:03, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > >
> > > > On Thu, Jul 29, 2021 at 09:48:21AM +0800, Tom Yan wrote:
> > > > > I'm not sure it's just me or you that are missing something here.
> > > > >
> > > > > On Wed, 28 Jul 2021 at 05:05, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > >
> > > > > > On Wed, Jul 28, 2021 at 02:36:18AM +0800, Tom Yan wrote:
> > > > > > > Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
> > > > > > > equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'.
> > > > > >
> > > > > > Yes, those two are equivalent.
> > > > > >
> > > > > > > Does that mean `tcp flags syn` (was supposed to be and) is now
> > > > > > > equivalent to `tcp flags == syn`
> > > > > >
> > > > > > tcp flag syn
> > > > > >
> > > > > > is a shortcut to match on the syn bit regarless other bit values, it's
> > > > > > a property of the bitmask datatypes.
> > > > >
> > > > > Don't you think the syntax will be inconsistent then? As a user, it
> > > > > looks pretty irrational to me: with a mask, just `syn` checks the
> > > > > exact value of the flags (combined); without a mask, it checks and
> > > > > checks only whether the specific bit is on.
> > > > >
> > > > > At least to me I would then expect `tcp flags syn` should be
> > > > > equivalent / is a shortcut to `tcp flags & (fin | syn | rst | psh |
> > > > > ack | urg | ecn | cwr) syn` hence `tcp flags & (fin | syn | rst | psh
> > > > > | ack | urg | ecn | cwr) == syn` hence `tcp flags == syn`.
> > > >
> > > > As I said, think of a different use-case:
> > > >
> > > >         ct state new,established
> > > >
> > > > people are _not_ expecting to match on both flags to be set on (that
> > > > will actually never happen).
> > > >
> > > > Should ct state and tcp flags use the same syntax (comma-separated
> > > > values) but intepret things in a different way? I don't think so.
> > > >
> > > > You can use:
> > > >
> > > >         nft describe ct state
> > > >
> > > > to check for the real datatype behind this selector: it's a bitmask.
> > > > For this datatype the implicit operation is not ==.
> > > >
> > > > > > tcp flags == syn
> > > > > >
> > > > > > is an exact match, it checks that the syn bit is set on.
> > > > > >
> > > > > > > instead of `tcp flags & syn == syn` / `tcp flags & syn != 0`?
> > > > > >
> > > > > > these two above are equivalent, I just sent a patch to fix the
> > > > > > tcp flags & syn == syn case.
> > > > > >
> > > > > > > Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
> > > > > > > syn / syn` instead, please note that while nft translates `tcp flags &
> > > > > > > syn == syn` to `tcp flags syn / syn`, it does not accept the
> > > > > > > translation as input (when the mask is not a comma-separated list):
> > > > > > >
> > > > > > > # nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
> > > > > > > Error: syntax error, unexpected newline, expecting comma
> > > > > > > add rule meh tcp_flags tcp flags syn / syn
> > > > > > >                                           ^
> > > > > >
> > > > > > The most simple way to express this is: tcp flags == syn.
> > > > >
> > > > > That does not sound right to me at all. Doesn't `syn / syn` means
> > > > > "with the mask (the second/"denominator" `syn`) applied on the flags,
> > > > > we get the value (the first/"nominator" `syn`), which means `tcp flags
> > > > > & syn == syn` instead of `tcp flags == syn` (which in turn means all
> > > > > bits but syn are cleared).
> > > >
> > > > tcp flags syn / syn makes no sense, it's actually: tcp flags syn.
> > > >
> > > > The goal is to provide a compact syntax for most common operations, so
> > > > users do not need to be mingling with explicit binary expressions
> > > > (which is considered to be a "more advanced" operation).

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Tom Yan]

On Wed, 28 Jul 2021 at 05:05, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Wed, Jul 28, 2021 at 02:36:18AM +0800, Tom Yan wrote:
> > Hmm, that means `tcp flags & (fin | syn | rst | ack) syn` is now
> > equivalent to 'tcp flags & (fin | syn | rst | ack) == syn'.
>
> Yes, those two are equivalent.
>
> > Does that mean `tcp flags syn` (was supposed to be and) is now
> > equivalent to `tcp flags == syn`
>
> tcp flag syn
>
> is a shortcut to match on the syn bit regarless other bit values, it's
> a property of the bitmask datatypes.
>
> tcp flags == syn
>
> is an exact match, it checks that the syn bit is set on.
>
> > instead of `tcp flags & syn == syn` / `tcp flags & syn != 0`?
>
> these two above are equivalent, I just sent a patch to fix the
> tcp flags & syn == syn case.
>
> > Suppose `tcp flags & syn != 0` should then be translated to `tcp flags
> > syn / syn` instead, please note that while nft translates `tcp flags &
> > syn == syn` to `tcp flags syn / syn`, it does not accept the
> > translation as input (when the mask is not a comma-separated list):
> >
> > # nft --debug=netlink add rule meh tcp_flags 'tcp flags syn / syn'
> > Error: syntax error, unexpected newline, expecting comma
> > add rule meh tcp_flags tcp flags syn / syn
> >                                           ^
>
> The most simple way to express this is: tcp flags == syn.
>
> > Also, does that mean `tcp flags & (fin | syn | rst | ack) fin,syn,ack`
> > will now be equivalent to `tcp flags & (fin | syn | rst | ack) = (fin
> > | syn | ack)`
>
> Yes, those two are equivalent. This is the same example as the one you
> have used at the beginning of this email.
>
> > instead of (ultimately) `tcp flags & (fin | syn | ack)  != 0`?
>
> That's equivalent to:
>
> tcp flags fin,syn,ack
>
> A quick summary:
>
> - If you want an exact match:
>
> tcp flags == fin,syn,ack
>
> - If you want to check that those three bits are set on (regardless
>   the remaining bits):
>
> tcp flags fin,syn,ack / fin,syn,ack
>
> - If you want to check that any of these three bits is set on:
>
> tcp flags fin,syn,ack

This is exactly what I find absurd btw. IMHO it's much better if the
latter just means `tcp flags == (fin | syn | ack)`. I'd rather we keep
`tcp flags & (fin | syn | ack) != 0` and so "unsimplified" or accept
something like `tcp flags { fin / fin, syn / syn, ack / ack }`, if
necessary at all. I think being "obvious" / "unambiguous" /
"straight-forward" enough is much more important than being (too)
"neat".

>
> > Which means `tcp flags & (fin | syn | ack) != 0` should not be
> > translated to `tcp flags fin,syn,ack`?
>
> tcp flags & (fin | syn | ack) != 0 is checking for any of these three
> bits to be set on, this translation is correct.

Re: [PATCH nft 2/3] netlink_linearize: incorrect netlink bytecode with binary operation and flags

[Pablo Neira Ayuso]

On Thu, Jul 29, 2021 at 10:57:35AM +0800, Tom Yan wrote:
> On Wed, 28 Jul 2021 at 05:05, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
[...]
> > A quick summary:
> >
> > - If you want an exact match:
> >
> > tcp flags == fin,syn,ack
> >
> > - If you want to check that those three bits are set on (regardless
> >   the remaining bits):
> >
> > tcp flags fin,syn,ack / fin,syn,ack
> >
> > - If you want to check that any of these three bits is set on:
> >
> > tcp flags fin,syn,ack
> 
> This is exactly what I find absurd btw. IMHO it's much better if the
> latter just means `tcp flags == (fin | syn | ack)`.

Look at this from a different angle, ie. ct state

        ct state new,established

ct state also has a bitmask datatype, and people are not expecting
here to match to new AND established.

> I'd rather we keep `tcp flags & (fin | syn | ack) != 0` and so
> "unsimplified" or accept something like `tcp flags { fin / fin, syn
> / syn, ack / ack }`

The curly brace notation implies the use of sets. Sets only allow for
exact matches, therefore

tcp flags { fin, syn, ack}

is actually making exact matches on the tcp flags.

[PATCH nft 3/3] evaluate: disallow negation with binary operation

[Pablo Neira Ayuso]

The negation was introduced to provide a simple shortcut. Extend
e6c32b2fa0b8 ("src: add negation match on singleton bitmask value") to
disallow negation with binary operations too.

 # nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) ! syn'
 Error: cannot combine negation with binary expression
 add rule meh tcp_flags tcp flags & (fin | syn | rst | ack) ! syn
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^   ~~~

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/evaluate.c      | 16 ++++++++++------
 tests/py/inet/tcp.t |  1 +
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 4609576b2a61..8b5f51cee01c 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2016,12 +2016,16 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 		/* fall through */
 	case OP_NEQ:
 	case OP_NEG:
-		if (rel->op == OP_NEG &&
-		    (right->etype != EXPR_VALUE ||
-		     right->dtype->basetype == NULL ||
-		     right->dtype->basetype->type != TYPE_BITMASK))
-			return expr_binary_error(ctx->msgs, left, right,
-						 "negation can only be used with singleton bitmask values");
+		if (rel->op == OP_NEG) {
+			if (left->etype == EXPR_BINOP)
+				return expr_binary_error(ctx->msgs, left, right,
+							 "cannot combine negation with binary expression");
+			if (right->etype != EXPR_VALUE ||
+			    right->dtype->basetype == NULL ||
+			    right->dtype->basetype->type != TYPE_BITMASK)
+				return expr_binary_error(ctx->msgs, left, right,
+							 "negation can only be used with singleton bitmask values");
+		}
 
 		switch (right->etype) {
 		case EXPR_RANGE:
diff --git a/tests/py/inet/tcp.t b/tests/py/inet/tcp.t
index 983564ec5b75..13b84215bd86 100644
--- a/tests/py/inet/tcp.t
+++ b/tests/py/inet/tcp.t
@@ -75,6 +75,7 @@ tcp flags & (fin | syn | rst | psh | ack | urg | ecn | cwr) == fin | syn | rst |
 tcp flags { syn, syn | ack };ok
 tcp flags & (fin | syn | rst | psh | ack | urg) == { fin, ack, psh | ack, fin | psh | ack };ok
 tcp flags ! fin,rst;ok
+tcp flags & (fin | syn | rst | ack) ! syn;fail
 
 tcp window 22222;ok
 tcp window 22;ok
-- 
2.20.1