[PATCH 0/2 nft] mptcp: add mptcp subtype mnemonics

[PATCH 0/2 nft] mptcp: add mptcp subtype mnemonics

[Florian Westphal]

This allows use of mnemonics, e.g.

tcp option mptcp subtype mp-capable

The new datatype is phony: on kernel-side its represented as
TYPE_INTEGER.
It can only be used a set key via the 'typeof' expression.

This avoids bloating the (finite) list of data types just to
handle the extra symbol table.

 include/datatype.h                                 |  5 +++-
 src/expression.c                                   | 34 ++++++++++++++++++++++
 src/tcpopt.c                                       | 30 ++++++++++++++++++-
 tests/py/any/tcpopt.t                              |  6 ++--
 tests/py/any/tcpopt.t.json                         |  2 +-
 tests/py/any/tcpopt.t.json.output                  | 31 ++++++++++++++++++++
 tests/py/any/tcpopt.t.payload                      | 12 ++++----
 tests/shell/testcases/sets/dumps/typeof_sets_0.nft |  9 ++++++
 tests/shell/testcases/sets/typeof_sets_0           |  9 ++++++
 9 files changed, 126 insertions(+), 12 deletions(-)

[PATCH nft 1/2] tcpopt: add phony mptcp suboption datatype

[Florian Westphal]

Because the number of unique 'enum datatypes' is limited by ABI
contraints this adds a new mptcp suboption type as integer alias.

This means that input side will work fine, but output won't, e.g.

tcp option mptcp subtype mp-capable

works, but will be shown as 'subtype 0'.

A followup patch will augment delinearization to infer the correct
type.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/datatype.h                            |  5 ++-
 src/tcpopt.c                                  | 30 +++++++++++++++++-
 tests/py/any/tcpopt.t                         |  4 +--
 tests/py/any/tcpopt.t.json                    |  2 +-
 tests/py/any/tcpopt.t.json.output             | 31 +++++++++++++++++++
 tests/py/any/tcpopt.t.payload                 |  2 +-
 .../testcases/sets/dumps/typeof_sets_0.nft    |  9 ++++++
 tests/shell/testcases/sets/typeof_sets_0      |  9 ++++++
 8 files changed, 86 insertions(+), 6 deletions(-)

diff --git a/include/datatype.h b/include/datatype.h
index f5bb9dc4d937..70a0a8ac8f5d 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -254,7 +254,6 @@ extern const struct datatype verdict_type;
 extern const struct datatype nfproto_type;
 extern const struct datatype bitmask_type;
 extern const struct datatype integer_type;
-extern const struct datatype xinteger_type;
 extern const struct datatype string_type;
 extern const struct datatype lladdr_type;
 extern const struct datatype ipaddr_type;
@@ -276,6 +275,10 @@ extern const struct datatype priority_type;
 extern const struct datatype policy_type;
 extern const struct datatype cgroupv2_type;
 
+/* TYPE_INTEGER aliases: */
+extern const struct datatype xinteger_type;
+extern const struct datatype mptcpopt_subtype;
+
 void inet_service_type_print(const struct expr *expr, struct output_ctx *octx);
 
 extern const struct datatype *concat_type_alloc(uint32_t type);
diff --git a/src/tcpopt.c b/src/tcpopt.c
index c3e07d7889ab..9f2f082ec333 100644
--- a/src/tcpopt.c
+++ b/src/tcpopt.c
@@ -109,6 +109,31 @@ static const struct exthdr_desc tcpopt_md5sig = {
 	},
 };
 
+static const struct symbol_table mptcp_subtype_tbl = {
+	.base		= BASE_DECIMAL,
+	.symbols	= {
+		SYMBOL("mp-capable",	0),
+		SYMBOL("mp-join",	1),
+		SYMBOL("dss",		2),
+		SYMBOL("add-addr",	3),
+		SYMBOL("remove-addr",	4),
+		SYMBOL("mp-prio",	5),
+		SYMBOL("mp-fail",	6),
+		SYMBOL("mp-fastclose",	7),
+		SYMBOL("mp-tcprst",	8),
+		SYMBOL_LIST_END
+	},
+};
+
+/* alias of integer_type to parse mptcp subtypes */
+const struct datatype mptcpopt_subtype = {
+	.type		= TYPE_INTEGER,
+	.name		= "integer",
+	.desc		= "mptcp option subtype",
+	.size		= 4,
+	.basetype	= &integer_type,
+	.sym_tbl	= &mptcp_subtype_tbl,
+};
 
 static const struct exthdr_desc tcpopt_mptcp = {
 	.name		= "mptcp",
@@ -116,7 +141,10 @@ static const struct exthdr_desc tcpopt_mptcp = {
 	.templates	= {
 		[TCPOPT_MPTCP_KIND]	= PHT("kind",   0,   8),
 		[TCPOPT_MPTCP_LENGTH]	= PHT("length", 8,  8),
-		[TCPOPT_MPTCP_SUBTYPE]  = PHT("subtype", 16, 4),
+		[TCPOPT_MPTCP_SUBTYPE]  = PROTO_HDR_TEMPLATE("subtype",
+							     &mptcpopt_subtype,
+							     BYTEORDER_BIG_ENDIAN,
+							     16, 4),
 	},
 };
 #undef PHT
diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
index 3d4be2a274df..a5ac1e86e207 100644
--- a/tests/py/any/tcpopt.t
+++ b/tests/py/any/tcpopt.t
@@ -51,6 +51,6 @@ tcp option md5sig exists;ok
 tcp option fastopen exists;ok
 tcp option mptcp exists;ok
 
-tcp option mptcp subtype 0;ok
-tcp option mptcp subtype 1;ok
+tcp option mptcp subtype mp-capable;ok
+tcp option mptcp subtype 1;ok;tcp option mptcp subtype mp-join
 tcp option mptcp subtype { 0, 2};ok
diff --git a/tests/py/any/tcpopt.t.json b/tests/py/any/tcpopt.t.json
index 5cc6f8f42446..dc1e7a279c4a 100644
--- a/tests/py/any/tcpopt.t.json
+++ b/tests/py/any/tcpopt.t.json
@@ -533,7 +533,7 @@
     }
 ]
 
-# tcp option mptcp subtype 0
+# tcp option mptcp subtype mp-capable
 [
     {
         "match": {
diff --git a/tests/py/any/tcpopt.t.json.output b/tests/py/any/tcpopt.t.json.output
index ad0d25f4d56c..fd17cc2ace34 100644
--- a/tests/py/any/tcpopt.t.json.output
+++ b/tests/py/any/tcpopt.t.json.output
@@ -30,3 +30,34 @@
     }
 ]
 
+# tcp option mptcp subtype mp-capable
+[
+    {
+        "match": {
+            "left": {
+                "tcp option": {
+                    "field": "subtype",
+                    "name": "mptcp"
+                }
+            },
+            "op": "==",
+            "right": "mp-capable"
+        }
+    }
+]
+
+# tcp option mptcp subtype 1
+[
+    {
+        "match": {
+            "left": {
+                "tcp option": {
+                    "field": "subtype",
+                    "name": "mptcp"
+                }
+            },
+            "op": "==",
+            "right": "mp-join"
+        }
+    }
+]
diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
index 121cc97fac09..269dee0aedb6 100644
--- a/tests/py/any/tcpopt.t.payload
+++ b/tests/py/any/tcpopt.t.payload
@@ -168,7 +168,7 @@ inet
   [ exthdr load tcpopt 1b @ 30 + 0 present => reg 1 ]
   [ cmp eq reg 1 0x00000001 ]
 
-# tcp option mptcp subtype 0
+# tcp option mptcp subtype mp-capable
 inet
   [ exthdr load tcpopt 1b @ 30 + 2 => reg 1 ]
   [ bitwise reg 1 = ( reg 1 & 0x000000f0 ) ^ 0x00000000 ]
diff --git a/tests/shell/testcases/sets/dumps/typeof_sets_0.nft b/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
index e397a6345462..52dc583b139c 100644
--- a/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
+++ b/tests/shell/testcases/sets/dumps/typeof_sets_0.nft
@@ -45,6 +45,11 @@ table inet t {
 			     15 }
 	}
 
+	set s10 {
+		typeof tcp option mptcp subtype
+		elements = { mp-join, dss }
+	}
+
 	chain c1 {
 		osf name @s1 accept
 	}
@@ -76,4 +81,8 @@ table inet t {
 	chain c9 {
 		ip hdrlength @s9 accept
 	}
+
+	chain c10 {
+		tcp option mptcp subtype @s10 accept
+	}
 }
diff --git a/tests/shell/testcases/sets/typeof_sets_0 b/tests/shell/testcases/sets/typeof_sets_0
index be906cdcc842..5a40fe4f20c2 100755
--- a/tests/shell/testcases/sets/typeof_sets_0
+++ b/tests/shell/testcases/sets/typeof_sets_0
@@ -50,6 +50,11 @@ EXPECTED="table inet t {
 		elements = { 0, 1, 2, 3, 4, 15 }
 	}
 
+	set s10 {
+		typeof tcp option mptcp subtype
+		elements = { mp-join, dss }
+	}
+
 	chain c1 {
 		osf name @s1 accept
 	}
@@ -81,6 +86,10 @@ EXPECTED="table inet t {
 	chain c9 {
 		ip hdrlength @s9 accept
 	}
+
+	chain c10 {
+		tcp option mptcp subtype @s10 accept
+	}
 }"
 
 set -e
-- 
2.32.0

[PATCH nft 2/2] src: propagate key datatype for anonymous sets

[Florian Westphal]

set s10 {
  typeof tcp option mptcp subtype
  elements = { mp-join, dss }
}

is listed correctly: typeof provides the 'mptcpopt_subtype'
datatype, so listing will print the elements with their sybolic types.

In anon case this doesn't work:
tcp option mptcp subtype { mp-join, dss }

gets shown as 'tcp option mptcp subtype { 1,  2}' because the anon
set has integer type.

This change propagates the datatype to the individual members
of the anon set.

After this change, multiple existing data types such as
TYPE_ICMP_TYPE could be replaced by integer-type aliases, but those
data types are already exposed to userspace via the 'set type'
directive so doing this may break existing set definitions.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/expression.c              | 34 ++++++++++++++++++++++++++++++++++
 tests/py/any/tcpopt.t         |  2 +-
 tests/py/any/tcpopt.t.payload | 10 +++++-----
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/src/expression.c b/src/expression.c
index f1cca8845376..9de70c6cc1a4 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -1249,6 +1249,31 @@ static void set_ref_expr_destroy(struct expr *expr)
 	set_free(expr->set);
 }
 
+static void set_ref_expr_set_type(const struct expr *expr,
+				  const struct datatype *dtype,
+				  enum byteorder byteorder)
+{
+	const struct set *s = expr->set;
+
+	/* normal sets already have a precise datatype that is given in
+	 * the set definition via type foo.
+	 *
+	 * Anon sets do not have this, and need to rely on type info
+	 * generated at rule creation time.
+	 *
+	 * For most cases, the type info is correct.
+	 * In some cases however, the kernel only stores TYPE_INTEGER.
+	 *
+	 * This happens with expressions that only use an integer alias
+	 * type, such as mptcp_suboption.
+	 *
+	 * In this case nft would print '1', '2', etc. instead of symbolic
+	 * names because the base type lacks ->sym_tbl information.
+	 */
+	if (s->init && set_is_anonymous(s->flags))
+		expr_set_type(s->init, dtype, byteorder);
+}
+
 static const struct expr_ops set_ref_expr_ops = {
 	.type		= EXPR_SET_REF,
 	.name		= "set reference",
@@ -1256,6 +1281,7 @@ static const struct expr_ops set_ref_expr_ops = {
 	.json		= set_ref_expr_json,
 	.clone		= set_ref_expr_clone,
 	.destroy	= set_ref_expr_destroy,
+	.set_type	= set_ref_expr_set_type,
 };
 
 struct expr *set_ref_expr_alloc(const struct location *loc, struct set *set)
@@ -1310,6 +1336,13 @@ static void set_elem_expr_clone(struct expr *new, const struct expr *expr)
 	init_list_head(&new->stmt_list);
 }
 
+static void set_elem_expr_set_type(const struct expr *expr,
+				   const struct datatype *dtype,
+				   enum byteorder byteorder)
+{
+       expr_set_type(expr->key, dtype, byteorder);
+}
+
 static const struct expr_ops set_elem_expr_ops = {
 	.type		= EXPR_SET_ELEM,
 	.name		= "set element",
@@ -1317,6 +1350,7 @@ static const struct expr_ops set_elem_expr_ops = {
 	.print		= set_elem_expr_print,
 	.json		= set_elem_expr_json,
 	.destroy	= set_elem_expr_destroy,
+	.set_type	= set_elem_expr_set_type,
 };
 
 struct expr *set_elem_expr_alloc(const struct location *loc, struct expr *key)
diff --git a/tests/py/any/tcpopt.t b/tests/py/any/tcpopt.t
index a5ac1e86e207..f57532c3016d 100644
--- a/tests/py/any/tcpopt.t
+++ b/tests/py/any/tcpopt.t
@@ -53,4 +53,4 @@ tcp option mptcp exists;ok
 
 tcp option mptcp subtype mp-capable;ok
 tcp option mptcp subtype 1;ok;tcp option mptcp subtype mp-join
-tcp option mptcp subtype { 0, 2};ok
+tcp option mptcp subtype { mp-capable, mp-join, remove-addr, mp-prio, mp-fail, mp-fastclose, mp-tcprst };ok
diff --git a/tests/py/any/tcpopt.t.payload b/tests/py/any/tcpopt.t.payload
index 269dee0aedb6..5a3102f9b459 100644
--- a/tests/py/any/tcpopt.t.payload
+++ b/tests/py/any/tcpopt.t.payload
@@ -180,11 +180,11 @@ inet
   [ bitwise reg 1 = ( reg 1 & 0x000000f0 ) ^ 0x00000000 ]
   [ cmp eq reg 1 0x00000010 ]
 
-# tcp option mptcp subtype { 0, 2}
-__set%d test-inet 3 size 2
-__set%d test-inet 0
-	element 00000000  : 0 [end]	element 00000020  : 0 [end]
-inet
+# tcp option mptcp subtype { mp-capable, mp-join, remove-addr, mp-prio, mp-fail, mp-fastclose, mp-tcprst }
+__set%d test-ip4 3 size 7
+__set%d test-ip4 0
+	element 00000000  : 0 [end]	element 00000010  : 0 [end]	element 00000040  : 0 [end]	element 00000050  : 0 [end]	element 00000060  : 0 [end]	element 00000070  : 0 [end]	element 00000080  : 0 [end]
+ip test-ip4 input
   [ exthdr load tcpopt 1b @ 30 + 2 => reg 1 ]
   [ bitwise reg 1 = ( reg 1 & 0x000000f0 ) ^ 0x00000000 ]
   [ lookup reg 1 set __set%d ]
-- 
2.32.0

Re: [PATCH nft 2/2] src: propagate key datatype for anonymous sets

[Pablo Neira Ayuso]

On Thu, Dec 09, 2021 at 04:11:31PM +0100, Florian Westphal wrote:
> set s10 {
>   typeof tcp option mptcp subtype
>   elements = { mp-join, dss }
> }
> 
> is listed correctly: typeof provides the 'mptcpopt_subtype'
> datatype, so listing will print the elements with their sybolic types.
> 
> In anon case this doesn't work:
> tcp option mptcp subtype { mp-join, dss }
> 
> gets shown as 'tcp option mptcp subtype { 1,  2}' because the anon
> set has integer type.
> 
> This change propagates the datatype to the individual members
> of the anon set.
> 
> After this change, multiple existing data types such as
> TYPE_ICMP_TYPE could be replaced by integer-type aliases, but those
> data types are already exposed to userspace via the 'set type'
> directive so doing this may break existing set definitions.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  src/expression.c              | 34 ++++++++++++++++++++++++++++++++++
>  tests/py/any/tcpopt.t         |  2 +-
>  tests/py/any/tcpopt.t.payload | 10 +++++-----
>  3 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/src/expression.c b/src/expression.c
> index f1cca8845376..9de70c6cc1a4 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -1249,6 +1249,31 @@ static void set_ref_expr_destroy(struct expr *expr)
>  	set_free(expr->set);
>  }
>  
> +static void set_ref_expr_set_type(const struct expr *expr,
> +				  const struct datatype *dtype,
> +				  enum byteorder byteorder)
> +{
> +	const struct set *s = expr->set;
> +
> +	/* normal sets already have a precise datatype that is given in
> +	 * the set definition via type foo.
> +	 *
> +	 * Anon sets do not have this, and need to rely on type info
> +	 * generated at rule creation time.
> +	 *
> +	 * For most cases, the type info is correct.
> +	 * In some cases however, the kernel only stores TYPE_INTEGER.
> +	 *
> +	 * This happens with expressions that only use an integer alias
> +	 * type, such as mptcp_suboption.
> +	 *
> +	 * In this case nft would print '1', '2', etc. instead of symbolic
> +	 * names because the base type lacks ->sym_tbl information.
> +	 */
> +	if (s->init && set_is_anonymous(s->flags))
> +		expr_set_type(s->init, dtype, byteorder);

Will this work with concatenations?

Re: [PATCH nft 2/2] src: propagate key datatype for anonymous sets

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Dec 09, 2021 at 04:11:31PM +0100, Florian Westphal wrote:
> > set s10 {
> >   typeof tcp option mptcp subtype
> >   elements = { mp-join, dss }
> > }
> > 
> > is listed correctly: typeof provides the 'mptcpopt_subtype'
> > datatype, so listing will print the elements with their sybolic types.
> > 
> > In anon case this doesn't work:
> > tcp option mptcp subtype { mp-join, dss }
> > 
> > gets shown as 'tcp option mptcp subtype { 1,  2}' because the anon
> > set has integer type.
> > 
> > This change propagates the datatype to the individual members
> > of the anon set.
> > 
> > After this change, multiple existing data types such as
> > TYPE_ICMP_TYPE could be replaced by integer-type aliases, but those
> > data types are already exposed to userspace via the 'set type'
> > directive so doing this may break existing set definitions.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  src/expression.c              | 34 ++++++++++++++++++++++++++++++++++
> >  tests/py/any/tcpopt.t         |  2 +-
> >  tests/py/any/tcpopt.t.payload | 10 +++++-----
> >  3 files changed, 40 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/expression.c b/src/expression.c
> > index f1cca8845376..9de70c6cc1a4 100644
> > --- a/src/expression.c
> > +++ b/src/expression.c
> > @@ -1249,6 +1249,31 @@ static void set_ref_expr_destroy(struct expr *expr)
> >  	set_free(expr->set);
> >  }
> >  
> > +static void set_ref_expr_set_type(const struct expr *expr,
> > +				  const struct datatype *dtype,
> > +				  enum byteorder byteorder)
> > +{
> > +	const struct set *s = expr->set;
> > +
> > +	/* normal sets already have a precise datatype that is given in
> > +	 * the set definition via type foo.
> > +	 *
> > +	 * Anon sets do not have this, and need to rely on type info
> > +	 * generated at rule creation time.
> > +	 *
> > +	 * For most cases, the type info is correct.
> > +	 * In some cases however, the kernel only stores TYPE_INTEGER.
> > +	 *
> > +	 * This happens with expressions that only use an integer alias
> > +	 * type, such as mptcp_suboption.
> > +	 *
> > +	 * In this case nft would print '1', '2', etc. instead of symbolic
> > +	 * names because the base type lacks ->sym_tbl information.
> > +	 */
> > +	if (s->init && set_is_anonymous(s->flags))
> > +		expr_set_type(s->init, dtype, byteorder);
> 
> Will this work with concatenations?

No.  But concatenations won't work for any of these, e.g.

ip version . ip daddr
tcp option mptcp subtype . tcp dport

... and so on because integer type (unspecific length) can't be
used with concat types so far.

So I think the problem is related but not added in this patch.

However, I think I need to withdraw this propsed patch for a different
reason.

If we'd try to expose mptcp option matching for specific sub fields,
the symbol expression path might not be ideal, for example consider:

   mptcp option subtype mp-join address-id gt 4 drop

(illustrative example with made-up syntax).

From parser perspective it might be better to make this

MPTCP OPTION SUBTYPE mptcp_option_fields : { ...


mptcp_option_fields : MP_JOIN mptcp_option_mpjoin_fields ...
		    | MP_CAPABLE mptcp_option_capable_fields ...


rather than

mptcp option subtype STRING STRING
... where parser calls helpers to 'translate' $4 and $5 to
the desired needed offset-length values (plus dependencies
to avoid bogus match on different suboption).

What do you think?